Problems working with allowEmpty & required #7

weierophinney · 2015-07-23T21:27:22Z

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 · 2015-06-16T01:19:08Z

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 · 2015-07-18T22:58:44Z

@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 · 2015-07-18T23:03:42Z

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 · 2015-07-19T00:23:41Z

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 · 2015-07-19T00:44:31Z

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 · 2015-07-19T08:08:03Z

@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 · 2015-07-20T08:32:30Z

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 · 2015-07-20T13:10:18Z

@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 · 2015-07-20T13:52:06Z

For the record, zendframework/zendframework#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 · 2015-07-20T15:16:09Z

@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 · 2015-07-20T15:24:38Z

@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 · 2015-07-21T18:46:23Z

@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 · 2015-07-21T18:51:10Z

@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!

Per the issue, this patch ensures that if an input is marked as required **and** marked to allow empty that the following works as expected: - If the input name is present in the data set, but a value that evaluates to empty, it will validate as true. - If the input name **is not** present in the data set, it will validate as false.

- Mainly to ensure tests will pass!

weierophinney · 2015-07-23T21:32:17Z

src/BaseInputFilter.php

@@ -219,7 +218,7 @@ public function isValid($context = null)
        }

        $inputs = $this->validationGroup ?: array_keys($this->inputs);
-        return $this->validateInputs($inputs, $data, $context);
+        return $this->validateInputs($inputs, $this->data, $context);


This turned out to be the key. Using getRawValues() was a poor solution, as it pulls values from the inputs themselves; since these return null when no value is set, there's no clean way to determine if the field was missing in the original data set!

Since the $data set is what was originall passed, and it's only used for its keys in validateInputs(), this provided the key to fixing the issue.

getRawValues usage was exactly what I spotted to be the problem for my usecase and what I didn't understood on the validation workflow side (why don't they rely on given data and take the input filter as default set¿). I thought it was intended to go this way (always having a null default), so I didn't thought of it being a "poor solution". I just always thought that I didn't found the missing puzzle piece to understand it :)

weierophinney · 2015-07-23T21:44:20Z

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 · 2015-07-23T23:45:36Z

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! 🎆

akrabat · 2015-07-24T06:21:38Z

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.

Forward port #7

Close #7

weierophinney · 2015-07-28T16:00:48Z

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

Per report on zendframework#15, the change made in zendframework#7 could lead to an issue if $data is an `ArrayAccess` instance, as `validateInputs()` typehinted on `array`. This patch adds a test for that condition, and removes the typehint.

zendframework/zend-inputfilter#7 introduced a minor change in how required elements are handled in specific cases, and this results in a change in how the "Value is required" validation message is injected into validation failures. This patch makes the test more robust in searching for that message.

Ocramius · 2015-08-17T10:04:58Z

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 · 2015-08-17T10:45:36Z

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.

Per report on zendframework#15, the change made in zendframework#7 could lead to an issue if $data is an `ArrayAccess` instance, as `validateInputs()` typehinted on `array`. This patch adds a test for that condition, and removes the typehint.

Maks3w · 2015-10-08T12:20:18Z

#74 already claims this introduce a BC Break.

weierophinney added the bug label Jul 23, 2015

weierophinney self-assigned this Jul 23, 2015

weierophinney added this to the 2.5.2 milestone Jul 23, 2015

CS fixes per php-cs-fixer

480e4d3

- Mainly to ensure tests will pass!

weierophinney reviewed Jul 23, 2015
View reviewed changes

weierophinney added a commit that referenced this pull request Jul 28, 2015

Created CHANGELOG for #7

c8698fb

weierophinney added a commit that referenced this pull request Jul 28, 2015

Merge branch 'hotfix/7' into develop

a123087

Forward port #7

weierophinney merged commit 480e4d3 into zendframework:master Jul 28, 2015

weierophinney added a commit that referenced this pull request Jul 28, 2015

Merge branch 'hotfix/7'

4d6495f

Close #7

weierophinney mentioned this pull request Aug 11, 2015

Argument 2 passed to Zend\InputFilter\BaseInputFilter::validateInputs() must be of the type array, object given #15

Merged

mwillbanks mentioned this pull request Aug 11, 2015

Unittests failing zendframework/zend-mvc#21

Merged

This was referenced Aug 21, 2015

Fix missing optional fields should not be validated #25

Merged

Delegate to InputInterface::isValid when data not exists #30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problems working with allowEmpty & required #7

Problems working with allowEmpty & required #7

weierophinney commented Jul 23, 2015

Mischosch commented Jun 16, 2015

adamlundrigan commented Jul 18, 2015

Mischosch commented Jul 18, 2015

adamlundrigan commented Jul 19, 2015

Mischosch commented Jul 19, 2015

manuakasam commented Jul 19, 2015

akrabat commented Jul 20, 2015

Mischosch commented Jul 20, 2015

weierophinney commented Jul 20, 2015

Mischosch commented Jul 20, 2015

weierophinney commented Jul 20, 2015

weierophinney commented Jul 21, 2015

Mischosch commented Jul 21, 2015

weierophinney Jul 23, 2015

Mischosch Jul 23, 2015

weierophinney commented Jul 23, 2015

Mischosch commented Jul 23, 2015

akrabat commented Jul 24, 2015

weierophinney commented Jul 28, 2015

Ocramius commented Aug 17, 2015

asgrim commented Aug 17, 2015

Maks3w commented Oct 8, 2015

Problems working with allowEmpty & required #7

Problems working with allowEmpty & required #7

Conversation

weierophinney commented Jul 23, 2015

Mischosch commented Jun 16, 2015

adamlundrigan commented Jul 18, 2015

Mischosch commented Jul 18, 2015

adamlundrigan commented Jul 19, 2015

Mischosch commented Jul 19, 2015

manuakasam commented Jul 19, 2015

akrabat commented Jul 20, 2015

Mischosch commented Jul 20, 2015

weierophinney commented Jul 20, 2015

Mischosch commented Jul 20, 2015

weierophinney commented Jul 20, 2015

weierophinney commented Jul 21, 2015

Mischosch commented Jul 21, 2015

weierophinney Jul 23, 2015

Choose a reason for hiding this comment

Mischosch Jul 23, 2015

Choose a reason for hiding this comment

weierophinney commented Jul 23, 2015

Mischosch commented Jul 23, 2015

akrabat commented Jul 24, 2015

weierophinney commented Jul 28, 2015

Ocramius commented Aug 17, 2015

asgrim commented Aug 17, 2015

Maks3w commented Oct 8, 2015