<form method="post" enctype="multipart/form-data"><input type="file" multiple name="img[]"><input type="submit"></form><?php$loader =require__DIR__ .'/../app/autoload.php';if ($_SERVER['REQUEST_METHOD'] ==='POST') {$request = \Symfony\Component\HttpFoundation\Request::createFromGlobals();var_export($request->files->all()['img']);}

Expected result when I send the form without any files:

array ()

Actual result:

array ( 0 => NULL, )

This causes a problem later when using FileType with multiple option - if no files are sent the form data are[0 => ''] instead of[].

Of course I need to add a test for this.

carsonbot added Status: Needs Review HttpFoundation Bug labels

Sep 14, 2017

Copy link

Member

xabbuh commentedSep 14, 2017

TheFileType should handle this quite well since#20418.

Copy link

ContributorAuthor

enumag commentedSep 14, 2017•
edited
Loading

@xabbuh It does not work because the null is somehow converted to an empty string. I don't know when or why. And regardless I believe this should be fixed in HttpFoundation anyway - some people are using HttpFoundation without Forms.

Should I revert the FileType fix as part of this PR? I don't think it's the right place to fix this issue.

Copy link

Member

xabbuh commentedSep 14, 2017

Not sure about reverting the change inside theFileType. We may discuss that. However, can you create a test covering the change you are doing here?

Copy link

ContributorAuthor

enumag commentedSep 14, 2017

Of course. I wouldn't want you to merge it without a test anyway.

enumag force-pushed thepatch-27 branch 2 times, most recently fromd111996 to748223cCompare

September 14, 2017 09:36

Copy link

ContributorAuthor

enumag commentedSep 14, 2017•
edited
Loading

@xabbuh Test added. For the Forms component I'd like to remove theFileType::buildForm() method. Other changes from#20418 should stay.

@yceruto Since you're the author of#20418, can I ask for your opinion?

xabbuh reviewed

Sep 14, 2017

View reviewed changes

src/Symfony/Component/HttpFoundation/Tests/FileBagTest.php

		publicfunctiontestShouldRemoveEmptyUploadedFilesForMultiUpload()
		{
		$bag =newFileBag(array('file' =>array(
		'name' =>array(''),

Copy link

Member

xabbuhSep 14, 2017

Choose a reason for hiding this comment

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

Is this really the structure that is used when multiple files are submitted?

Copy link

ContributorAuthor

enumagSep 14, 2017

Choose a reason for hiding this comment

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

@xabbuh Yes.

Copy link

ContributorAuthor

enumagSep 14, 2017•
edited
Loading

Choose a reason for hiding this comment

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

To be more precise it's the structure used for<input type="file" name="file[]"> (note the[]), regardless of how many files are actually sent (including 0) and regardless of themultiple attribute.

You can try yourself with the example script I posted above. Just addvar_export($_FILES); somewhere.

Copy link

Member

xabbuhSep 14, 2017

Choose a reason for hiding this comment

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

For the records, that's also explicitly described here:http://php.net/manual/en/features.file-upload.multiple.php

Copy link

Member

ycerutoSep 14, 2017•
edited
Loading

Choose a reason for hiding this comment

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

I can't reproduce the issue, thisFileBag with those values returns:

according to:

symfony/src/Symfony/Component/HttpFoundation/FileBag.php

Lines 86 to 87 in12f1239

	if (UPLOAD_ERR_NO_FILE ==$file['error']) {
	$file =null;

the rest is handled onPRE_SUBMIT event insideFileType ifmultiple = true

Copy link

Member

ycerutoSep 14, 2017

Choose a reason for hiding this comment

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

See test case also:

symfony/src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php

Lines 99 to 110 in77653d1

	publicfunctiontestSubmitNullWhenMultiple()
	{
	$form =$this->factory->create(static::TESTED_TYPE,null,array(
	'multiple' =>true,
	));
	// submitted data when an input file is uploaded without choosing any file
	$form->submit(array(null));

	$this->assertSame(array(),$form->getData());
	$this->assertSame(array(),$form->getNormData());
	$this->assertSame(array(),$form->getViewData());
	}

xabbuh approved these changes

Sep 14, 2017

View reviewed changes

Copy link

Member

xabbuh left a comment

Choose a reason for hiding this comment

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

looks good to me

xabbuh added this to the2.7 milestone

Sep 14, 2017

Copy link

Member

xabbuh commentedSep 14, 2017

@enumag Can you rebase and change the target branch to2.7?

Copy link

ContributorAuthor

enumag commentedSep 14, 2017

@xabbuh Sure. Can I ask why? I mean according toSymfony Release Process maintenance for 2.7 already ended. I don't think this qualifies for a security fix.

Copy link

Member

xabbuh commentedSep 14, 2017

Symfony is an LTS release it receives bugfix until May 2018.

Copy link

ContributorAuthor

enumag commentedSep 14, 2017

Oh right, my mistake. I'll rebase it soon.

enumag changed the base branch from2.8 to2.7

September 14, 2017 12:26

enumag force-pushed thepatch-27 branch from748223c tob6a21f8Compare

September 14, 2017 12:26

[HttpFoundation] Fix file upload multiple with no files

d4f6039

enumag force-pushed thepatch-27 branch fromb6a21f8 tod4f6039Compare

September 14, 2017 12:29

Copy link

ContributorAuthor

enumag commentedSep 14, 2017

@xabbuh All done.

Copy link

Member

yceruto commentedSep 14, 2017•
edited
Loading

Yep, I thinkbuildForm() and related test case can be removed safely so.

Copy link

ContributorAuthor

enumag commentedSep 18, 2017

@xabbuh Should I remove the buildForm here or in another PR? Perhaps it should only be removed in newer symfony versions to prevent regression when new version of Form component is used with old version of HttpFoundation?

Copy link

Member

xabbuh commentedSep 20, 2017

Wecould remove the Form related part, but as you said it may hurt people mixing different versions of the HttpFoundation and Form components. Thus, as it doesn't hurt, I'd vote to keep it as is.

xabbuh added Status: Reviewed and removed Status: Needs Review labels

Sep 20, 2017

Copy link

ContributorAuthor

enumag commentedSep 20, 2017

Alright. I will send a PR to Forms later when there is no possibility of such conflict.

fabpot approved these changes

Sep 30, 2017

View reviewed changes

Copy link

Member

fabpot commentedSep 30, 2017

Thank you@enumag.

fabpot merged commitd4f6039 intosymfony:2.7

Sep 30, 2017

fabpot added a commit that referenced this pull request

Sep 30, 2017

bug#24198[HttpFoundation] Fix file upload multiple with no files (e…

166f64e

…numag)This PR was merged into the 2.7 branch.Discussion----------[HttpFoundation] Fix file upload multiple with no files| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |```php<form method="post" enctype="multipart/form-data"><input type="file" multiple name="img[]"><input type="submit"></form><?php$loader = require __DIR__ . '/../app/autoload.php';if ($_SERVER['REQUEST_METHOD'] === 'POST') {    $request = \Symfony\Component\HttpFoundation\Request::createFromGlobals();    var_export($request->files->all()['img']);}```Expected result when I send the form without any files:```array ()```Actual result:```array ( 0 => NULL, )```This causes a problem later when using FileType with multiple option - if no files are sent the form data are `[0 => '']` instead of `[]`.Of course I need to add a test for this.Commits-------d4f6039 [HttpFoundation] Fix file upload multiple with no files

This was referencedOct 5, 2017

Release v2.7.35#24450

Merged

Release v2.8.28#24451

Merged

Release v3.3.10#24452

Merged