[HttpFoundation] Fix file upload multiple with no files #24198

enumag · 2017-09-14T08:30:24Z

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

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

xabbuh · 2017-09-14T08:33:12Z

The FileType should handle this quite well since #20418.

enumag · 2017-09-14T08:50:55Z

@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.

xabbuh · 2017-09-14T09:07:49Z

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

enumag · 2017-09-14T09:09:39Z

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

enumag · 2017-09-14T09:39:33Z

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

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

xabbuh · 2017-09-14T09:46:36Z

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

+    public function testShouldRemoveEmptyUploadedFilesForMultiUpload()
+    {
+        $bag = new FileBag(array('file' => array(
+            'name' => array(''),


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

@xabbuh Yes.

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 the multiple attribute.

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

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

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

according to:

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

Lines 86 to 87 in 12f1239

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

$file = null;

the rest is handled on PRE_SUBMIT event inside FileType if multiple = true

See test case also:

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

Lines 99 to 110 in 77653d1

public function testSubmitNullWhenMultiple()

{

$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

looks good to me

xabbuh · 2017-09-14T12:00:05Z

@enumag Can you rebase and change the target branch to 2.7?

enumag · 2017-09-14T12:06:00Z

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

xabbuh · 2017-09-14T12:14:00Z

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

enumag · 2017-09-14T12:16:48Z

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

enumag · 2017-09-14T12:30:03Z

@xabbuh All done.

yceruto · 2017-09-14T13:28:05Z

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

enumag · 2017-09-18T07:08:50Z

@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?

xabbuh · 2017-09-20T09:58:08Z

We could 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.

enumag · 2017-09-20T16:48:26Z

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

fabpot · 2017-09-30T14:18:22Z

Thank you @enumag.

…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

carsonbot added Status: Needs Review HttpFoundation Bug labels Sep 14, 2017

enumag force-pushed the patch-27 branch 2 times, most recently from d111996 to 748223c Compare September 14, 2017 09:36

xabbuh reviewed Sep 14, 2017

View reviewed changes

xabbuh approved these changes Sep 14, 2017

View reviewed changes

xabbuh added this to the 2.7 milestone Sep 14, 2017

enumag changed the base branch from 2.8 to 2.7 September 14, 2017 12:26

enumag force-pushed the patch-27 branch from 748223c to b6a21f8 Compare September 14, 2017 12:26

[HttpFoundation] Fix file upload multiple with no files

d4f6039

enumag force-pushed the patch-27 branch from b6a21f8 to d4f6039 Compare September 14, 2017 12:29

xabbuh added Status: Reviewed and removed Status: Needs Review labels Sep 20, 2017

fabpot approved these changes Sep 30, 2017

View reviewed changes

fabpot merged commit d4f6039 into symfony:2.7 Sep 30, 2017

This was referenced Oct 5, 2017

Release v2.7.35 #24450

Merged

Release v2.8.28 #24451

Merged

Release v3.3.10 #24452

Merged

szymach mentioned this pull request Oct 13, 2017

A request to revert changes in removing empty file uploads #24546

Closed

xabbuh mentioned this pull request Nov 2, 2017

File Type is now empty when updating (in 3.3) #24792

Closed

BoShurik mentioned this pull request Jan 1, 2020

[Form][HttpFoundation] Filtering FileBag removes items from CollectionType with FileType #35100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Fix file upload multiple with no files #24198

[HttpFoundation] Fix file upload multiple with no files #24198

enumag commented Sep 14, 2017 •

edited

Loading

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017 •

edited

Loading

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017

enumag commented Sep 14, 2017 •

edited

Loading

xabbuh Sep 14, 2017

enumag Sep 14, 2017

enumag Sep 14, 2017 •

edited

Loading

xabbuh Sep 14, 2017

yceruto Sep 14, 2017 •

edited

Loading

yceruto Sep 14, 2017

xabbuh left a comment

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017

enumag commented Sep 14, 2017

yceruto commented Sep 14, 2017 •

edited

Loading

enumag commented Sep 18, 2017

xabbuh commented Sep 20, 2017

enumag commented Sep 20, 2017

fabpot commented Sep 30, 2017

	public function testSubmitNullWhenMultiple()
	{
	$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());
	}

[HttpFoundation] Fix file upload multiple with no files #24198

[HttpFoundation] Fix file upload multiple with no files #24198

Conversation

enumag commented Sep 14, 2017 • edited Loading

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017 • edited Loading

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017

enumag commented Sep 14, 2017 • edited Loading

xabbuh Sep 14, 2017

Choose a reason for hiding this comment

enumag Sep 14, 2017

Choose a reason for hiding this comment

enumag Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

xabbuh Sep 14, 2017

Choose a reason for hiding this comment

yceruto Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

yceruto Sep 14, 2017

Choose a reason for hiding this comment

xabbuh left a comment

Choose a reason for hiding this comment

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017

xabbuh commented Sep 14, 2017

enumag commented Sep 14, 2017

enumag commented Sep 14, 2017

yceruto commented Sep 14, 2017 • edited Loading

enumag commented Sep 18, 2017

xabbuh commented Sep 20, 2017

enumag commented Sep 20, 2017

fabpot commented Sep 30, 2017

enumag commented Sep 14, 2017 •

edited

Loading

enumag commented Sep 14, 2017 •

edited

Loading

enumag commented Sep 14, 2017 •

edited

Loading

enumag Sep 14, 2017 •

edited

Loading

yceruto Sep 14, 2017 •

edited

Loading

yceruto commented Sep 14, 2017 •

edited

Loading