Problems working with allowEmpty & required

[Mischosch]

Hey there,

I'm trying to get my head around using Zend\InputFilter for validation. I don't use the library with Zend\Form, I try to use it for API backend data validation and filtering. In this setup we have POST data coming from outside and I would like to validate this against business rules.

Let's say, I have the attribute foo, which is required and allow_empty is false and I have bar, which is required, but needs allow_empty true, to be able to set an empty string as value. Doing it this way, isValid returns false, if I do not provide any foo key inside the array I provide with setData call. This is what I expect, everything finet.

But if I provided a valid foo attribute and I do not provide any bar key inside the data array, the isValid call will return true. What I would expect: failing like the first test, because the key bar is completly missing.

I do know, that there were many changes around isRequire and allowEmpty in last two releases. Since then we're struggeling with our defined InputFilters and trying to understand, what changes happend and how the logic behind this is thought to work now. I already have read, that the attribute continueIfEmpty is also having a role inside the InputFilter process and it is working for me, that validation rules get processed if I set it to true.

Digging through the isValid process I recognized, that firing an isValid call is first running through $this->getRawValues(); - after that point, $data array contains bar with NULL value, because the data keys are generated out of defined inputFilter. So setting continueIfEmpty to true and adding an own validator to check for existing key inside my data array does not work, because it does exist inside there, being populated through getRawValues call.

So, is there any way to validate provided input for existiting keys with InpuFilter? Or how can I achieve having a fixed requirement of data keys, that need to be provided, or input gets marked as invalid? I would really to avoid extra checks like

    if (!isset($postValues['bar'])) {
        throw new \InvalidArgumentException('Please provide a value for "bar"');
    }

My tests to show, what my problem is.

    class AllowEmptyTest extends \PHPUnit_Framework_TestCase
    {
        public function testInputFilterBeingRequiredAndDataNotProvided()
        {
            $inputFilter = new \Zend\InputFilter\InputFilter();
            $inputFilter->add([
                'name' => 'foo',
                'required' => true,
                'allow_empty' => false,
            ]);
            $inputFilter->add([
                'name' => 'bar',
                'required' => true,
                'allow_empty' => true,
            ]);

            $data = [
                'foo2' => 'xyz',
                'bar' => ''
            ];

            $inputFilter->setData($data);

            $this->assertFalse($inputFilter->isValid());
        }

        public function testInputFilterBeingRequiredAndAllowEmptyTrue()
        {
            $inputFilter = new \Zend\InputFilter\InputFilter();
            $inputFilter->add([
                'name' => 'foo',
                'required' => true,
                'allow_empty' => false,
            ]);
            $inputFilter->add([
                'name' => 'bar',
                'required' => true,
                'allow_empty' => true,
            ]);

            $data = [
                'foo' => 'xyz'
            ];

            $inputFilter->setData($data);

            $this->assertFalse($inputFilter->isValid());
        }
    }

[Mischosch]

http://validator.particle-php.com/en/latest/required-optional/

Required and optional are special cases within Particle\Validator. Both required and optional will only check on the existence of the key, not the value. You can check on a value being set with "allowEmpty", which is the third parameter to both the required as the optional methods (and false by default).

This feels really clear compared to ZF inputFilter stituation - how about learning from them and introduce "optional" to zend-inputfilters?

[adamlundrigan]

@Mischosch if I understand correctly the required key will do what you're looking for:

<?php
<<<CONFIG
packages:
    - "zendframework/zend-inputfilter: ~2.5"
CONFIG;
// http://melody.sensiolabs.org/

$inputFilter = new \Zend\InputFilter\InputFilter();
$inputFilter->add([
    'name' => 'foo',
    'required' => true,
    'allow_empty' => false,
]);
$inputFilter->add([
    'name' => 'bar',
    // I set required to false to signal that this key can be omitted entirely
    'required' => false,
    'allow_empty' => true,
]);

$data = [
    'foo' => 'xyz',
];

$inputFilter->setData($data);

assert($inputFilter->isValid() === true);

required controls whether the key can be omitted or not. allow_empty controls whether the key's value can be empty() or not.

[Mischosch]

jup, so far the theory - feel free to run the test yourself. allow_empty with true ignores required and makes it not possible to verify the existence of the key.

[adamlundrigan]

Is this the essense of what you're getting at?

<?php
<<<CONFIG
packages:
    - "zendframework/zend-inputfilter: ~2.5"
CONFIG;
// http://melody.sensiolabs.org/

$inputFilter = new \Zend\InputFilter\InputFilter();
$inputFilter->add([
    'name' => 'foo',
    'required' => true,
    'allow_empty' => true,
]);

$data = [ ];
$inputFilter->setData($data);
assert($inputFilter->isValid() === false);  // fails

If the Input is marked required and allow_empty the key can be omitted from the input and the validator still passes. To me that's a bug as my understanding was that required should enforce the existence of the key.

EDIT: I ran the above case against the highest revision of each minor ZF2 version and they all behave in the same way.

[Mischosch]

Had my learning about having my own understanding and running into problems because of that. Better have explicit tests to find out what it is all about. I do believe others would run into the same problem, like you did now.

If it's a bug, not sure, could be feature, too. There is a difference between having a value required, or the existence of a key related to the allowed_empty value.

Not sure who is the package maintainer to ask more directly about this. You are the first commenting on my questions.

I would not start workign on a possible fix for that until opinions on this topic are clear. I do not want to start to work on a PR that gets rejected, because it being a "hot" topic inside the last releases.

[manuakasam]

@Mischosch have you seen #4 ? It sounds like you're running into the same Issue as I did. And I agree, this really isn't the most intuitive situation but reading Matthews response there at least you know the reason why that's the case.

I'm not entirely sure if that's the only reason to fix things (it's certainly not the most beautiful one), but it works as intended now. This pretty much boils down to PHP Arrays and the handling of null|0|'' values.

[akrabat]

I'm really not a fan of the way required, allow_empty and continue_if_empty work and interact with each other.

We get far too many questions about it, too. It would be really nice if we could tidy this up.

[Mischosch]

@manuakasam yes I read all related threads I could find about this topic. But I didn't found a solution to my problem there, only, that there were def some refactorings going on.

@akrabat how do you think about the approach of http://validator.particle-php.com/en/latest/required-optional/ ? It feels really clear by only reading the description, as they divide between key and value. I have read much more about required/allow_empty for Zend\InputFilter and it was a huge amount of trying out the params to see what is happening.

[weierophinney]

For the record, zendframework/zf2#7474 details the exact interactions between required, allow_empty, and continue_if_empty that are tested and supported in the framework. Prior to 2.4, we had not tested each of the interactions, which led to some developers and projects depending on edge cases that were not intended, and which we cannot support long-term due to the fact that they cause conflicts (in essence, if we chose to support the edge case, we would be breaking intended functionality, and we would be inviting conflicting support requests; we've already had to do so with some of the collection support, which has led to breakages from one version to the next!).

[Mischosch]

@weierophinney as I understand your statement: this part is a sensible one and you would prefer keeping the things like they are instead of perhaps breaking (again) existing workflows by adding more otptions (like optional) to it?

[weierophinney]

@Mischosch Adding another flag increases the complexity even more. "Optional" is the opposite of "Required", so, to my mind, I think having it as an additional flag would be even more confusing.

I'll take a look at your question in more detail later today, and see if I can provide a solution using the existing flags; it's very likely possible, but it may require validation groups.

[weierophinney]

@Mischosch I've taken your test cases from above and run them, and what I see is:

• testInputFilterBeingRequiredAndDataNotProvided: returns false currently; assertion passes.
• testInputFilterBeingRequiredAndAllowEmptyTrue: returns true currently; assertion fails.

Based on your narrative, what you're looking for is the following:

• You want to define an element "bar".
• "bar" is required; it must be in the data; however,
• "bar" can be empty when passed.

The combination of required + allow_empty SHOULD be invalidating the data if the key is missing, but I can verify that it is not. I'm going to see if I can figure out a solution to this later today.

[Mischosch]

@weierophinney yes, you understood my point! Thanks a lot for looking again at my question/tests. I do appreciate that you really tried to understand what my problem is and even more, that you try to find a possible solution. If I can help at any point, let me know!

[weierophinney]

I think that we should likely do an LTS release with this fix as well, as the problem and solution fit a number of reports I've seen since the 2.4 release. I'll schedule that for Monday (27 July 2015).

[Mischosch]

Wow, @weierophinney - you're making me so happy. Thanks a lot for your done work. Next time I will try to add a possible solution as PR to my initial request. I think in this case you're def having a better overview of changes affecting other users, the history of the code and the definition of what these parameters should do or not do. (Hopefully my next problem is not inside such a sensible topic like validation.)

I think this is a good thing for the InputFilter library. Makes it much more usable for API validation now and we do not need to refactor our code to another validation library. Thanks again for your time and work! :fireworks:

[akrabat]

I think that we should likely do an LTS release with this fix as well, as the problem and solution fit a number of reports I've seen since the 2.4 release. I'll schedule that for Monday (27 July 2015).

I agree.

[weierophinney]

Released with:

• zend-inputfilter 2.5.2
• zend-inputfilter 2.4.5
• zf2 2.4.5

(For ZF2 2.5 versions, just do a composer update; you'll get the new component version.)

[Ocramius]

This PR broke BC for our project when upgrading from 2.4.4 -> 2.4.5. Still investigating, but there is indeed a problem with it.

I suggest caution when upgrading until we actually find the regression (or verify that it was already fixed in develop)

/cc @asgrim

[asgrim]

Yes indeed, this change breaks our tests when upgrading, but it seems we are relying on incorrect behaviour to run tests.

Despite reading through and agreeing with this particular change (which makes sense), it's still a BC break because previously behaviour was undefined, and projects (such as ours) are relying on this behaviour. I also understand that effort is being put in to clearly define expected behaviour now:

...which led to some developers and projects depending on edge cases that were not intended, and which we cannot support long-term due to the fact that they cause conflicts...

which it seems we're in this boat ;)

We're going to have to lock our composer.json to 2.4.4 until we can now get time to go through and fix our project's broken tests.

[Maks3w]

74 already claims this introduce a BC Break.