ZF-9289: Zend_Filter_Input incorrect generation of validation errors

Description

Zend_Filter_Input has incorrect behavior for handling custom error messages configured via the 'messages' meta-command and applying a 'presence' meta-command on a field validator declaration. This issue extends ZF-8446 and covers additional erroneous behavior.

Using the following sample code:


<?php
// .. Require framework classes ..

$data = array(
  "title" => "",
  "project_id" => "",
  "reporter_id" => "18",
  "assigned_id" => "0",
  "type" => "1",
  "priority" => "2",
  "tags" => "",
  "description" => "",
  "reproduce" => ""
);

$validators = array(
    'title' => array('NotEmpty','messages' => 'Please enter issue title'),
    'description' => array('NotEmpty','messages' => 'Please enter issue description'),
    'reproduce' => array('NotEmpty','presence' => 'optional'),
    'browsers' => array('NotEmpty','presence' => 'optional'),
    'os' => array('NotEmpty','presence' => 'optional'),
    'status' => array('Digits','presence' => 'optional'),
    'type' => 'Digits',
    'priority' => 'Digits',
    'project_id' => array('Digits','messages' => 'Please select project'),
    'reporter_id' => array('Digits','messages' => 'Please select issue reporter'),
    'assigned_id' => array('NotEmpty','presence' => 'optional','allowEmpty' => true)
);

$options = array('presence' => 'required','missingMessage' => "Please fill the required field '%field%'");
$input = new Zend_Filter_Input(null,$validators,$data,$options);

$input -> isValid();
echo '
';
var_dump($input -> getMessages());
echo '
';

The expected output: (works in ZF 1.7.6)


array(3) {
  ["title"]=> array(1) {
    ["isEmpty"]=>
    string(24) "Please enter issue title"
  }
  ["description"]=> array(1) {
    ["isEmpty"]=>
    string(30) "Please enter issue description"
  }
  ["project_id"]=> array(1) {
    ["stringEmpty"]=>
    string(21) "Please select project"
  }
}

The actual output: (1.10 trunk)


array(4) {
  ["title"] => array(1) {
    ["isEmpty"] => string(24) "Please enter issue title"
  }
  ["description"] => array(1) {
    ["isEmpty"] => string(30) "Please enter issue description"
  }
  ["reproduce"] => array(1) {
    ["isEmpty"] => string(30) "Please enter issue description"
  }
  ["project_id"] => array(1) {
    ["isEmpty"] => string(30) "Please enter issue description"
  }
}

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

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".

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

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

I added a related issue, if you go there, you will find a patch that solves the problem with the wrong error message.

I changed it from bug to patch, because you can find a patch in the related issue. If Ralph nor Thomas finds the time to review the patch, I will commit it very soon.

The fix is in svn now.

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.

So if I have something like:


$validators = array(
    'status' => array('Digits','messages' => 'Please select your status')
);

If 'status' has an empty value, I will get the 'NotEmpty' default message - {quote}You must give a non-empty value for field 'status'{quote}

Instead of the error message defined in the 'messages' meta-command. On the other hand if I specify the 'NotEmpty' validator explicitly


$validators = array(
    'status' => array('NotEmpty','Digits','messages' => 'Please select your status')
);

I can now control the error message returned by an empty value. I think it's fair to say this is not the expected behavior (and it's undocumented)

Thanks for your fast feedback Eran.

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.

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.

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.

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.

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.


$data = array('name' => '');

$validators = array(
    'name' => 'NotEmpty'
);

$options = array('allowEmpty' => true);
$filter = new Zend_Filter_Input(null,$validators,$data,$options);

if($filter -> isValid()) {
//Validation passes, even though we have an explicit 'NotEmpty' validator 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.

Attached two patches, with a patch2 extension. Do not worry about the extension.

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

I will commit the resolution as it is in the *.patch3 files.

Thomas, I think my previous fix didn't make it into the 1.11.5 release. How should I go about to get fixes into the next mini release?

I've tested it and clears all of the use-cases I previously had problems with. I really appreciate your help on this, Bart

Check if the fix is within branch. If it is then it will be automatically integrated in the next release.

Additionally I am reopening this issue for integration within ZF2.

Was this merged to the release 1.11 branch?

Also, you can close this if it has been and follow ZF2-27 for integration in 2.x.

-ralph

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.

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.

Merged all fixes for Zend_Filter_Input and unit test into 1.11 branch

Merged into 1.11 branch

Bart: Which svn release?

Fixing assignment as Bart did the work

this specific fix was in svn release 23861 further fixes (different issue) in 23862, which is the version I merged into the 1.11 branch

Added to ZF2 with GH-273 Please note that there are still known issues with the provided patch within ZF1 which have been solved within ZF2 by Matthew