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

Problems working with allowEmpty & required #7

Merged
merged 2 commits into from
Jul 28, 2015
Merged

Problems working with allowEmpty & required #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3575ece
[#7] Ensure Required, AllowEmpty validation combo works
weierophinney Jul 23, 2015
480e4d3
CS fixes per php-cs-fixer
weierophinney Jul 23, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/BaseInputFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,16 +210,15 @@ public function setData($data)
*/
public function isValid($context = null)
{
$data = $this->getRawValues();
if (null === $data) {
if (null === $this->data) {
throw new Exception\RuntimeException(sprintf(
'%s: no data present to validate!',
__METHOD__
));
}

$inputs = $this->validationGroup ?: array_keys($this->inputs);
return $this->validateInputs($inputs, $data, $context);
return $this->validateInputs($inputs, $this->data, $context);
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

}

/**
Expand All @@ -244,7 +243,24 @@ protected function validateInputs(array $inputs, array $data = [], $context = nu
foreach ($inputs as $name) {
$input = $this->inputs[$name];

// make sure we have a value (empty) for validation of context
// If the value is required, but not present in the data set,
// validation fails.
if (!array_key_exists($name, $data)
&& $input instanceof InputInterface
&& $input->isRequired()
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the second part of the solution: if the key for the input is not present, and that input is required, then we should have a validation failure. This ensures that happens.

$input->setErrorMessage('Value is required');
$this->invalidInputs[$name] = $input;

if ($input->breakOnFailure()) {
return false;
}

$valid = false;
continue;
}

// Make sure we have a value (empty) for validation of context
if (!array_key_exists($name, $data)) {
$data[$name] = null;
}
Expand Down
56 changes: 56 additions & 0 deletions test/BaseInputFilterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,8 +641,10 @@ public static function contextDataProvider()
/**
* @dataProvider contextDataProvider()
*/
// @codingStandardsIgnoreStart
public function testValidationMarksInputValidWhenAllowEmptyFlagIsTrueAndContinueIfEmptyIsTrueAndContextValidatesEmptyField($allowEmpty, $blankIsValid, $valid)
{
// @codingStandardsIgnoreEnd
$filter = new InputFilter();

$data = [
Expand Down Expand Up @@ -964,4 +966,58 @@ public function testAllowEmptyTestsFilteredValueAndContinuesIfEmpty()

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

/**
* @group 7
*/
public function testMissingRequiredAllowedEmptyValueShouldMarkInputFilterInvalid()
Copy link
Member Author

Choose a reason for hiding this comment

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

This and the next test case are based on the original issue.

{
$foo = new Input('foo');
$foo->setRequired(true);
$foo->setAllowEmpty(false);

$bar = new Input('bar');
$bar->setRequired(true);
$bar->setAllowEmpty(true);

$filter = new InputFilter();
$filter->add($foo);
$filter->add($bar);

$filter->setData(['foo' => 'xyz']);
$this->assertFalse($filter->isValid(), 'Missing required value should mark input filter as invalid');
}

public function emptyValuesForValidation()
{
return [
'null' => [null],
'empty-string' => [''],
];
}

/**
* @group 7
* @dataProvider emptyValuesForValidation
*/
public function testEmptyValuePassedForRequiredButAllowedEmptyInputShouldMarkInputFilterValid($value)
{
$foo = new Input('foo');
$foo->setRequired(true);
$foo->setAllowEmpty(false);

$bar = new Input('bar');
$bar->setRequired(true);
$bar->setAllowEmpty(true);

$filter = new InputFilter();
$filter->add($foo);
$filter->add($bar);

$filter->setData([
'foo' => 'xyz',
'bar' => $value,
]);
$this->assertTrue($filter->isValid(), 'Empty value should mark input filter as valid');
}
}
12 changes: 6 additions & 6 deletions test/FactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,15 +655,15 @@ public function testSuggestedTypeMayBePluginNameInInputFilterPluginManager()
$pluginManager->setService('bar', new Input('bar'));
$factory->setInputFilterManager($pluginManager);

$input = $factory->createInput(array(
$input = $factory->createInput([
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in this class are due to php-cs-fixer complaining about lack of short array syntax, and were necessary to make the PR pass CI.

'type' => 'bar'
));
]);
$this->assertSame('bar', $input->getName());

$this->setExpectedException('Zend\Filter\Exception\RuntimeException');
$factory->createInput(array(
$factory->createInput([
'type' => 'foo'
));
]);
}

public function testInputFromPluginManagerMayBeFurtherConfiguredWithSpec()
Expand All @@ -674,10 +674,10 @@ public function testInputFromPluginManagerMayBeFurtherConfiguredWithSpec()
$this->assertTrue($barInput->isRequired());
$factory->setInputFilterManager($pluginManager);

$input = $factory->createInput(array(
$input = $factory->createInput([
'type' => 'bar',
'required' => false
));
]);

$this->assertFalse($input->isRequired());
$this->assertSame('bar', $input->getName());
Expand Down