[HttpFoundation] Fix file upload multiple with no files #24198
Conversation
|
The |
|
@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. |
|
Not sure about reverting the change inside the |
|
Of course. I wouldn't want you to merge it without a test anyway. |
d111996 to
748223c
Compare
| public function testShouldRemoveEmptyUploadedFilesForMultiUpload() | ||
| { | ||
| $bag = new FileBag(array('file' => array( | ||
| 'name' => array(''), |
There was a problem hiding this comment.
Is this really the structure that is used when multiple files are submitted?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For the records, that's also explicitly described here: http://php.net/manual/en/features.file-upload.multiple.php
There was a problem hiding this comment.
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
the rest is handled on PRE_SUBMIT event inside FileType if multiple = true
There was a problem hiding this comment.
See test case also:
|
@enumag Can you rebase and change the target branch to |
|
@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. |
|
Symfony is an LTS release it receives bugfix until May 2018. |
|
Oh right, my mistake. I'll rebase it soon. |
|
@xabbuh All done. |
|
Yep, I think |
|
@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? |
|
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. |
|
Alright. I will send a PR to Forms later when there is no possibility of such conflict. |
|
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
Expected result when I send the form without any files:
Actual result:
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.