The last two returned messages are incorrect and an error message is returned for a valid field ('reproduce'), which has a 'presence' => 'optional' modifier (which works for some of the other fields). Zend_Filter_Input message handling has been buggy for several major revisions (since the 1.7 version), might be worth to see where it went wrong.

Comments

Posted by Thomas Weidner (thomas) on 2010-03-15T13:55:12.000+0000

Regarding "reproduce":
The error is correct and not wrong.

You defined that "reproduce" is optional, but when it's available it has to be "NotEmpty"... as the field "reproduce" is available the validator checks if it's not empty... but it is empty, and therefor the error is returned also for the field "reproduce".

Posted by Eran Galperin (erangalp) on 2010-03-15T17:10:36.000+0000

Yes, you are correct, my mistake. However, the error returned is not correct (it uses the 'messages' definition from another field)

Posted by Eran Galperin (erangalp) on 2010-04-28T14:39:49.000+0000

Is there any progress on this? this component still can't be used reliably in current versions of the framework

Thanks Bart.
This fix almost fixes the problem - it does restore the default error message and prevents overwriting. There is another problem that is fleshed out now that this is fixed and might deserve a separate issue report - if you declare a validator in the chain that has hidden dependencies (many validators also apply the 'NotEmpty' validator - 'Digits' for example), the 'messages' meta-command is again ignored and the default of the dependencies is used instead.

I think I disagree with you here. In your first example of your last comment: If you want to yield the message 'Please select your status', you should provide an invalid digit, such as 'a'. In that case, the Digits validator should trigger the custom message that you supplied. If instead, you supply nothing, I think it is reasonable that Zend_Filter_Input resorts to the default message for empty values that are required.

Posted by Eran Galperin (erangalp) on 2011-03-28T12:57:13.000+0000

I would argue that an empty string is still not a digit. This behavior is not expected - it took me a while to figure out I could override it by adding manually the 'NotEmpty' validator, and I imagine others would have the same problem (though it seems that Zend_Filter_Input is not in common use). Ideally, I could configure the 'Digits' validator to avoid the 'NotEmpty' check, but since it is not the case I'd expect it to use my custom error instead of providing a default override for something I didn't require.

It seems like a case of trying to reuse classes where it's not needed - the 'Digits' validator (and others) could have performed the check for an empty string internally and returned the custom error message attached to that validator. Instead it uses the 'NotEmpty' validator which is both unexpected and complicates matters regarding error messages.

I think I get your point to some extend, although I must admit that I myself do not use Zend_Filter_Input in my daily work. By fixing this bug I gained some understanding of the component, but I am not the designer or owner of the component.

What I understand from your last comment is that you would want every validator to check for not empty by itself and not let Zend_Filter_Input interfere with that. The obvious advantages of that approach would be that Zend_Filter_Input could be simpler internally, while becoming more predictable externally at the same time.

Apart from that I personnaly find that intermixing metacommands and validators is not a great feature. It takes us away from the initial Keep It Simple approach that should be typical of Zend Framework components.

However: simplifying this would likely break BC.

I am willing to take a second look at this, but I think you should create a separate issue for that and inform me of that.

Posted by Eran Galperin (erangalp) on 2011-03-29T08:42:36.000+0000

I'm not necessarily saying that each validator should check if it receives a non-empty string - just that if it does, it should be internally. Personally, I don't expect 'Digits' to fail on an empty string - if I want to check for non-empty values, I would add a 'NotEmpty' validator to the chain. The current behavior is not obvious and hard to override unless you know its kinks (like I unfortuntely already do).

I'll just mention that I'm still using an old version of Zend_Filter_Input that works perfectly (I think it's ver. 1.6). Not sure what happened to it since ver. 1.7, but it has become much harder to generate correct error output from it.

Thanks again for your feedback. Could you specify exactly what in this particular case 'perfectly' would mean to you in terms of expected behavior and based on which part of the manual (please add citation) you base the expectation.

I would like to make sure that if I change the behavior of Zend_Filter_Input in any way, this change would meet your expectations and that those expectations are based on documented behavior, rather than on how an older version of the component used to behave.

The old behavior may have been accidental and now that you have become used to it, you regard it as perfect? I am not saying this is the case, I just want to make sure that I really understand your use case and the meaning of expected and perfect in this context. Because I do not frequently use the component I do not yet have a feel for what natural or expected behavior would be in this particular case.

Posted by Eran Galperin (erangalp) on 2011-03-29T10:06:52.000+0000

First I'd like to thank you Bart, for pursuing this issue. I'd given up that it would be fixed since it seems efforts are geared towards ZF 2.0.

I went over the source again (haven't looked at it for a while) to understand exactly what is the problem. I think I have it better defined now -
Zend_Filter_Input has the 'allowEmpty' meta-command set to false by default. This causes a 'NotEmpty' check to be made for each validator by default, even if it's not explicitly defined in the validation chain. This check is made using the 'NotEmpty' validator before the defined validator is run, overriding error messages defined for the validator in question.

This is mentioned briefly in the manual as well (under the ALLOW_EMPTY meta-command), but the way it's described leads you to believe that the 'NotEmpty' check would be made only if no validators are defined for the field in question.

So what I'd expect in this case is one of two things -
* Either the ALLOW_EMPTY meta-command should be set to true by default as not to add validation checks not defined by the developer
* Or the defined validator runs before the 'NotEmpty' check allowing it to output its own error messages. In the case of the 'Digits' validator mentioned previously, it does have an internal check for a non-empty value and would have returned the defined error message (expected behavior). I would add that in my opinion it should return custom error message defined for the validator even if it uses the 'NotEmpty' validator internally.

Posted by Eran Galperin (erangalp) on 2011-03-29T10:40:28.000+0000

Another thing that I wanted to add - part of the problem with the 'allowEmpty' meta-command is that it overrides defined 'NotEmpty' validators in the chain. I'd expect it to apply only if there are no such validators defined.

This is a complicated subject. What is troubling me is that there is still a mix in the expectations you describe, between what is documented and what you find is reasonable.

From a performance perspective, it is wise to run the NotEmpty validators before any others, because if they break the chain, then the more expensive validations do not have to run. It is still easy to add a message for the not empty situation by adding a notempty validator.

I agree that if setting AllowEmpty to true for the overall chain, this should not apply if you explicitly add a NotEmpty validator nor if you explicitly use the metacommand 'presence' => 'required' for a specific validation rule. This should only affect the fact that by default, everything has to be present.

Again, these bugs or improvements should be in separate issues, so that they can be managed seperately, by people that understand the specific issue, this will also help people to add comments where appropriate.

I am reopening the issue, because I looked into to the issue with ALLOW_EMPTY and I agree with Eran, that even if each field is allowed empty by default, we should still disallow them to be empty if a NotEmpty validator is found in the ruleset.

I fixed this issue and will attach patches. The patches will wait for your evaluation for a few days. After this time, they will be committed into svn if no one has complained.

The previous patches still had a problem: the bug would persist if you would use a string 'NotEmpty' instead of a Zend_Validate_NotEmpty instance. I supplied new patches, with a patch3 extension that fix this issue. You no longer need the *.patch2

None of my fixes have been merged it seems. Is this something I should do myself? I had the (wrong?) idea that the component owner would take care of such responsible work, but I am willing to track my fixes back and merge all of them.

Posted by Thomas Weidner (thomas) on 2011-07-13T09:24:57.000+0000

Bart:
Can you please merge your changes to branch, add the svn release as note and then close this issue.
ZF2 will be done by another issue in this case.