Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix BC break in 2.1.5dev - Revert to previous isRequired behavior for file upload inputs #4171

Closed
wants to merge 2 commits into from

4 participants

Christopher Martin Mike Willbanks Matthew Weier O'Phinney Gocha Ossinkine
Christopher Martin

#3983 causes BC breaks for FilePostRedirectGet Plugin, and in existing projects using File Inputs with changed isRequired settings.

To reproduce, install https://github.com/cgmartin/ZF2FileUploadExamples and test the following examples:

  • "Single File Upload with partial validation"
  • "Post-Redirect-Get Plugin Example"
  • "FilePRG Plugin Example with nested Fieldset"

With ZF2.1.4, the examples work correctly.
With ZF2.1.5dev, the examples fail.

Reported via email by @leup (Thanks!)

library/Zend/InputFilter/BaseInputFilter.php
@@ -178,6 +170,14 @@ public function isValid()
&& isset($this->data[$name][0]['error']) && $this->data[$name][0]['error'] === UPLOAD_ERR_NO_FILE)
) {
if ($input instanceof InputInterface) {
+ // - test if input is required
+ if (!$input->isRequired()
+ // Do not mark valid for isRequired setting if value exists and is empty string
+ && !(array_key_exists($name, $this->data) && is_string($this->data[$name]) && strlen($this->data[$name]) === 0)
+ ) {
+ $this->validInputs[$name] = $input;
+ continue;
+ }
Christopher Martin
cgmartin added a note

I preserved what seemed to be the intent of #3983, but @ossinkine please look this over to make sure it's correct.

Looks like the truth.
But the expression strlen($this->data[$name]) === 0 is unnecessary, because the expression is checked before.

Christopher Martin
cgmartin added a note

Thanks, good catch - I will remove that expression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christopher Martin cgmartin commented on the diff
tests/ZendTest/InputFilter/BaseInputFilterTest.php
((6 lines not shown))
{
$filter = new InputFilter();
$foo = new FileInput();
$foo->getValidatorChain()->attach(new Validator\File\UploadFile());
- $foo->setAllowEmpty(true);
+ $foo->setRequired(false);
Christopher Martin
cgmartin added a note

Reverted these tests to match earlier behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christopher Martin cgmartin referenced this pull request in cgmartin/ZF2FileUploadExamples
Closed

BC break in ZF2.1.5dev #7

Mike Willbanks
Collaborator

@ossinkine could you please comment and verify it matches original intent?

Christopher Martin

Added update to remove unnecessary test from @ossinkine 's feedback. Should be good to go.

Matthew Weier O'Phinney weierophinney referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/4171' into develop
Forward port #4171

Conflicts:
	library/Zend/InputFilter/BaseInputFilter.php
aa369ae
Matthew Weier O'Phinney

Interestingly, this was already correct on develop -- due to a huge rewrite I did to the logic a few weeks ago to make it more readable and ensure the test cases were properly represented. :)

Matthew Weier O'Phinney

@cgmartin There was a conflict in behavior between two PRs, and the only way I could sort it out was to do that. :)

Gocha Ossinkine

@weierophinney Can you explain why do you need such validation in BaseInputFilter? Why not do the necessary checks directly in the Input?

Deleted user Unknown referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/4171'
Close #4171
f006917
Deleted user Unknown referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/4171' into develop
Forward port #4171

Conflicts:
	library/Zend/InputFilter/BaseInputFilter.php
4a24eca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
16 library/Zend/InputFilter/BaseInputFilter.php
View
@@ -159,14 +159,6 @@ public function isValid()
$inputs = $this->validationGroup ?: array_keys($this->inputs);
foreach ($inputs as $name) {
$input = $this->inputs[$name];
- if ((!array_key_exists($name, $this->data)
- || (null === $this->data[$name]))
- && $input instanceof InputInterface
- && !$input->isRequired()
- ) {
- $this->validInputs[$name] = $input;
- continue;
- }
if (!array_key_exists($name, $this->data)
|| (null === $this->data[$name])
|| (is_string($this->data[$name]) && strlen($this->data[$name]) === 0)
@@ -178,6 +170,14 @@ public function isValid()
&& isset($this->data[$name][0]['error']) && $this->data[$name][0]['error'] === UPLOAD_ERR_NO_FILE)
) {
if ($input instanceof InputInterface) {
+ // - test if input is required
+ if (!$input->isRequired()
+ // "Not required" should not apply to empty strings (#3983)
+ && !(array_key_exists($name, $this->data) && is_string($this->data[$name]))
+ ) {
+ $this->validInputs[$name] = $input;
+ continue;
+ }
// - test if input allows empty
if ($input->allowEmpty()) {
$this->validInputs[$name] = $input;
12 tests/ZendTest/InputFilter/BaseInputFilterTest.php
View
@@ -450,13 +450,13 @@ public function testValidationSkipsFieldsMarkedNotRequiredWhenNoDataPresent()
$this->assertTrue($filter->isValid());
}
- public function testValidationSkipsFileInputsMarkedAllowEmptyWhenNoFileDataIsPresent()
+ public function testValidationSkipsFileInputsMarkedNotRequiredWhenNoFileDataIsPresent()
{
$filter = new InputFilter();
$foo = new FileInput();
$foo->getValidatorChain()->attach(new Validator\File\UploadFile());
- $foo->setAllowEmpty(true);
+ $foo->setRequired(false);
Christopher Martin
cgmartin added a note

Reverted these tests to match earlier behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$filter->add($foo, 'foo');
@@ -473,16 +473,16 @@ public function testValidationSkipsFileInputsMarkedAllowEmptyWhenNoFileDataIsPre
$this->assertTrue($filter->isValid());
// Negative test
- $foo->setAllowEmpty(false);
+ $foo->setRequired(true);
$filter->setData($data);
$this->assertFalse($filter->isValid());
}
- public function testValidationSkipsFileInputsMarkedAllowEmptyWhenNoMultiFileDataIsPresent()
+ public function testValidationSkipsFileInputsMarkedNotRequiredWhenNoMultiFileDataIsPresent()
{
$filter = new InputFilter();
$foo = new FileInput();
- $foo->setAllowEmpty(true);
+ $foo->setRequired(false);
$filter->add($foo, 'foo');
$data = array(
@@ -498,7 +498,7 @@ public function testValidationSkipsFileInputsMarkedAllowEmptyWhenNoMultiFileData
$this->assertTrue($filter->isValid());
// Negative test
- $foo->setAllowEmpty(false);
+ $foo->setRequired(true);
$filter->setData($data);
$this->assertFalse($filter->isValid());
}
Something went wrong with that request. Please try again.