Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 4 commits into from

3 participants

@davidwindell

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

Ping @weierophinney

@davidwindell davidwindell referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@weierophinney

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
tests/ZendTest/InputFilter/BaseInputFilterTest.php
@@ -564,6 +564,22 @@ public function testValidationMarksInputInvalidWhenRequiredAndAllowEmptyFlagIsFa
$this->assertFalse($filter->isValid());
}
+ public function testValidationMarksInputInvalidWhenNotRequiredAndAllowEmptyFlagIsFalse()
@weierophinney 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.

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

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

@davidwindell

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

@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/4567'
Close #4567
7456196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 30, 2013
  1. @davidwindell
  2. @davidwindell

    Add test case

    davidwindell authored
Commits on Jun 18, 2013
  1. @davidwindell
  2. @davidwindell
This page is out of date. Refresh to see the latest.
View
1  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 Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
) {
$this->validInputs[$name] = $input;
continue;
View
4 library/Zend/InputFilter/Factory.php
@@ -197,8 +197,8 @@ public function createInput($inputSpecification)
break;
case 'required':
$input->setRequired($value);
- if (!isset($inputSpecification['allow_empty'])) {
- $input->setAllowEmpty(!$value);
+ if (isset($inputSpecification['allow_empty'])) {
+ $input->setAllowEmpty($inputSpecification['allow_empty']);
}
break;
case 'allow_empty':
View
1  library/Zend/InputFilter/Input.php
@@ -142,6 +142,7 @@ public function setName($name)
public function setRequired($required)
{
$this->required = (bool) $required;
+ $this->setAllowEmpty(!$required);
return $this;
}
View
16 tests/ZendTest/InputFilter/BaseInputFilterTest.php
@@ -564,6 +564,22 @@ public function testValidationMarksInputInvalidWhenRequiredAndAllowEmptyFlagIsFa
$this->assertFalse($filter->isValid());
}
+ public function testValidationMarksInputInvalidWhenNotRequiredAndAllowEmptyFlagIsFalse()
@weierophinney 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ $filter = new InputFilter();
+
+ $foo = new Input();
+ $foo->setRequired(false);
+ $foo->setAllowEmpty(false);
+
+ $filter->add($foo, 'foo');
+
+ $data = array('foo' => '');
+ $filter->setData($data);
+
+ $this->assertFalse($filter->isValid());
+ }
+
public static function contextDataProvider()
{
return array(
Something went wrong with that request. Please try again.