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

Projects

None yet

4 participants

Contributor
cgmartin commented Apr 3, 2013

#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!)

@cgmartin cgmartin and 1 other commented on an outdated diff Apr 3, 2013
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;
+ }
cgmartin
cgmartin Apr 3, 2013 Contributor

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

ossinkine
ossinkine Apr 6, 2013 Contributor

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

cgmartin
cgmartin Apr 6, 2013 Contributor

Thanks, good catch - I will remove that expression.

@cgmartin cgmartin commented on the diff Apr 3, 2013
tests/ZendTest/InputFilter/BaseInputFilterTest.php
{
$filter = new InputFilter();
$foo = new FileInput();
$foo->getValidatorChain()->attach(new Validator\File\UploadFile());
- $foo->setAllowEmpty(true);
+ $foo->setRequired(false);
cgmartin
cgmartin Apr 3, 2013 Contributor

Reverted these tests to match earlier behavior.

@cgmartin cgmartin referenced this pull request in cgmartin/ZF2FileUploadExamples Apr 3, 2013
Closed

BC break in ZF2.1.5dev #7

Contributor

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

Contributor
cgmartin commented Apr 6, 2013

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

@weierophinney weierophinney added a commit that referenced this pull request Apr 12, 2013
@weierophinney weierophinney Merge branch 'hotfix/4171' into develop
Forward port #4171

Conflicts:
	library/Zend/InputFilter/BaseInputFilter.php
aa369ae
Owner

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

Owner

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

Contributor

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

@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#4171 from cgmartin/fix…
…/file-prg-base-input-filter

Fix BC break in 2.1.5dev - Revert to previous isRequired behavior for file upload inputs
5c3ea85
@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/4171' 03fd3e7
@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/4171' into develop
Forward port zendframework/zendframework#4171

Conflicts:
	library/Zend/InputFilter/BaseInputFilter.php
abd6531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment