[InputFilter][Hotfix] Missing check for allowEmpty() #4567

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

davidwindell commented May 30, 2013

The InputFilter was allowing empty strings to pass through when allowEmpty was set to true since the refactor.

Ping @weierophinney

Owner

weierophinney commented Jun 10, 2013

@davidwindell This creates a regression, it appears: https://travis-ci.org/zendframework/zf2/jobs/7637942#L498 -- 5 new failures/errors.

@weierophinney weierophinney commented on the diff Jun 12, 2013

library/Zend/InputFilter/BaseInputFilter.php
@@ -236,6 +236,7 @@ protected function validateInputs(array $inputs)
&& '' === $this->data[$name]
&& $input instanceof InputInterface
&& !$input->isRequired()
+ && $input->allowEmpty()
@weierophinney

weierophinney Jun 12, 2013

Owner

I think this needs to be OR'd with the previous statement.

@weierophinney weierophinney commented on the diff Jun 12, 2013

tests/ZendTest/InputFilter/BaseInputFilterTest.php
@@ -564,6 +564,22 @@ public function testValidationMarksInputInvalidWhenRequiredAndAllowEmptyFlagIsFa
$this->assertFalse($filter->isValid());
}
+ public function testValidationMarksInputInvalidWhenNotRequiredAndAllowEmptyFlagIsFalse()
@weierophinney

weierophinney Jun 12, 2013

Owner

Actually, on reading this test case title... I think this would be a break.

By default, "allow empty" is false, and is used to determine if a required input is allowed to have an empty value. If you mark an element as not required, the assumption is that we allow an empty value -- this is why the "allow empty" flag defaults to false. If we change that, we break a ton of assumptions -- which you can observe in the CollectionInputFilter tests.

So, the question I have, then is: should calling setRequired(false) automatically set the "allow empty" flag to true? That would allow this to work -- but it also means that order of operations becomes critical -- you would be required to set the "allow empty" flag after setting the required flag if you wanted to set it to false.

Owner

weierophinney commented Jun 12, 2013

@davidwindell I've voiced some concerns above, and for the time being have removed the milestone. I'll set a new milestone once you're able to provide feedback and/or address the concerns.

Thanks in advance!

Contributor

davidwindell commented Jun 18, 2013

@weierophinney looking into this, I believe the correct way to solve is, as you say, to set "allow empty" flag to the reverse of "required" by default. In fact, the factory was already doing this if the user didn't explicitly specify allow_empty.

I've updated the factory too to ensure that regardless of the order of the specification, the allow_empty flag specified by the user (if any) is always respected..

I think this should be good for merge now.

@weierophinney weierophinney added a commit that referenced this pull request Jun 28, 2013

@weierophinney weierophinney Merge pull request #4567 from davidwindell/patch-11
[InputFilter][Hotfix] Missing check for allowEmpty()
5f21c56

@weierophinney weierophinney added a commit that referenced this pull request Jun 28, 2013

@weierophinney weierophinney Merge branch 'hotfix/4567' into develop
Forward port #4567
b7d23cd

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#4567 from davidwindell…
…/patch-11

[InputFilter][Hotfix] Missing check for allowEmpty()
442a68f

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/4567' 2a94048

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/4567' into develop db4f1c3

weierophinney referenced this pull request in zendframework/zend-inputfilter Jun 8, 2015

Closed

BC-Break in 2.4.1? Really annoying NotEmpty injection by default #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment