Possible BC break for zend-inputfilter > V2.5.1

[dstockto]

I think I've found an issue with zend-inputfilter which has prevented me from being able to upgrade successfully (first time it broke in production, second time I added some tests on validators which broke when moving to any version greater than V2.5.1.

I believe the issue was introduced with 3575ece7f53f58cab8086ffc5a88afc0ee4ddc76 -

I've got a few Apigility APIs which have fields that are set to required, allow_empty and continue_if_empty. The intent is that depending on the value of other fields in the API, these fields may or may not be actually required.

As a simplified example, of what used to work, suppose I have an "account_type" field which can be either "state" or "county". If the value is "county", then a "county" field must be provided with a non-empty value. If the value is "state" then the county field must be blank or not provided.

After upgrading to V2.5.2 (or V2.5.5) instead of a successful validation, I get the error message from the NotEmpty validator indicating that the value is required and cannot be empty.

Ideally, I'd like my work as it did after upgrading. Essentially if the account_type is state then I should not need to send in the county field at all. If there's a set of config changes I can make that will allow this to work, please let me know.

Ping @weierophinney and @Maks3w at Matthew's request.

[Maks3w]

We are deprecating continue_if_empty and allow_empty options.

As alternative do the following

  $input = new Input();
  $input->setAllowEmpty(true);         // Disable BC Break logic related to treat `null` values as valid empty value instead *not set*.
  $input->setContinueIfEmpty(true);    // Disable BC Break logic related to treat `null` values as valid empty value instead *not set*.
  $input->getValidatorChain()->attach(
      new Zend\Validator\NotEmpty(),
      true                             // break chain on failure

  );

or

  $inputSpecification = [
    'allow_empty'       => true,
    'continue_if_empty' => true,
    'validators' => [
      [
        'break_chain_on_failure' => true,
        'name'                   => 'Zend\\Validator\\NotEmpty',
      ],
    ],
  ];

If not please provide a Gist with the minimum code necessary for reproduce the issue

[dstockto]

@Maks3w The tests in this gist all pass for V2.5.1. The first data set fails on V2.5.2, V2.5.3, V2.5.4 and V2.5.5. The "CountyValidator" isn't strictly needed to see a difference, but some of the other tests will fail without it.

https://gist.github.com/dstockto/a9e241dcf7d90f59fbaf

The messages from the failure are all:

['result' => [ 'county' => [ 'isEmpty' => "Value is required and can't be empty"]]]

The suggestion to add the NotEmpty validator with 'break_chain_on_failure' doesn't appear to work anyway as I need the validators to continue to run.

[Maks3w]

I confirm the test works on 2.3.9 and 2.5.1

[Maks3w]

I'm not sure if this should be fixed or not.

We have basically two scenarios which could be reduced to just invoke the validator chain always:

• If the input is required call the ValidatorChain even if data was not present.
• If the input is not required call the ValidatorChain even if data was not present.

For to be honest I don't find any scenario where an Input should be validated with unknown data, not matter if required or not.

Also your CountyValidator basically validate the input itself is required.

So the problem can be rewording about how to conditional require an input

For me the expected way of require a field is modifying the required flag instead of hacking with the validation chain.

I opt for do something like this in my Apigility API https://gist.github.com/Maks3w/119477c754e5d4a8377e

I will forward this to @weierophinney so he decide if we should restore previous behavior.

[dstockto]

I understand the sentiment that it's a bug and should be fixed, but at the same time, it is a behavior change and a backward compatibility break at that. If semver is important then it should be fixed back to previous behavior.

I appreciate the workaround, but at the same time, this is a pattern that occurs in a number of places in my application currently and putting validation the resource is definitely not ideal. It means that you could fail on a number of validators initially, then pass those and now receive additional new errors that were not even mentioned previously. It's not a great situation from a usability standpoint for sure.

I am hoping that you, @Maks3w or @weierophinney would consider this a breaking change and reconsider fixing.

Thank you.

[weierophinney]

I think at this point, providing a fix to restore previous behavior will make this component too difficult to maintain; the test matrix currently is ridiculous, and adding one more flag to restore the behavior essentially doubles the number of data sets we need to test, introducing further brittleness to the suite — which is what the changes in the 2.5 series were trying to address.

We're working on a new validation suite of components, which began with zend-datavalidator. I'm thinking those will be the correct place to address the usability and consistency issues going forward.