Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54eec85
Fix missing check for allowEmpty
davidwindell May 30, 2013
6b37c76
Add test case
davidwindell May 30, 2013
35ea125
Make allowEmpty reverse of required
davidwindell Jun 18, 2013
d501083
Update factory to ensure correct order
davidwindell Jun 18, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions library/Zend/InputFilter/BaseInputFilter.php
View file
Open in desktop
Expand Up @@ -236,6 +236,7 @@ protected function validateInputs(array $inputs)
&& '' === $this->data[$name]
&& $input instanceof InputInterface
&& !$input->isRequired()
&& $input->allowEmpty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) {
$this->validInputs[$name] = $input;
continue;
Expand Down
16 changes: 16 additions & 0 deletions tests/ZendTest/InputFilter/BaseInputFilterTest.php
View file
Open in desktop
Expand Up @@ -564,6 +564,22 @@ public function testValidationMarksInputInvalidWhenRequiredAndAllowEmptyFlagIsFa
$this->assertFalse($filter->isValid());
}

public function testValidationMarksInputInvalidWhenNotRequiredAndAllowEmptyFlagIsFalse()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
$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(
Expand Down