-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Added new option to fix a little issue originated from last PR #3844
Conversation
@@ -43,7 +43,8 @@ class Identical extends AbstractValidator | |||
*/ | |||
protected $tokenString; | |||
protected $token; | |||
protected $strict = true; | |||
protected $strict = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you add a space ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To align with the line below!
@bakura10 sent this PR (#3869), to fix a BC break originated from this PR (#3803). If his PR is really necessary, this one isn't. I think this new option is a good idea, if it can't be in this version, it could be added in some next version (from what I see just in ZF 3.0, right!?) So someone take a good look and decide what is the best solution! |
I think we need to merge my PR, yours still introduce a BC that is not necessary. As you said, you need to keep this for ZF 3 :). |
As per the comments, I'll assign this issue to version 3.0.0, that will give us plenty of time to think about it and review it ;) Nonetheless I'd like to thank you for your PR, we have plenty of other issues that you may be willing to work on in the meanwhile! =) |
I think I'm missing something; I don't see anything backwards incompatible in this PR. I think we can merge this for 2.2.0. |
Added new option to fix a little issue originated from last PR Conflicts: tests/ZendTest/Validator/IdenticalTest.php
- Two tests were present on master but removed in develop -- and brought back in during merge. Removed.
The BC break was fixed with @bakura10's PR. When I first introduced the ability to validate in fieldsets, the line 174 was throwing an exeption - that happened in the case a I actually didn't realize this PR could still be used. |
Forgot to close when I merged! |
…dentical-validator Added new option to fix a little issue originated from last PR Conflicts: tests/ZendTest/Validator/IdenticalTest.php
…develop - Two tests were present on master but removed in develop -- and brought back in during merge. Removed.
After I sent this PR, I realised a little problem:
When identical validator is used in a form scope, the form will always provide a context, because of this, it wouldn't be possible to validate a
literal
token, see the example.This would not work because on validation the validator would try to find
'3'
in the context, and then rise an exception when it doesn't find it.To solve this I add a new option,
literal
. Setting this to true (it's false by default) would allow a 'literal' validation even when the context is provided. So the above input filter would look like this:This would tell the validator to skip the look up in the context and just use the string
'3'
._Note: this option has no affect when context is not provided._