New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 30, 2017

Conversation

Projects
None yet
5 participants
@enumag
Contributor

enumag commented Sep 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 14, 2017

Member

The FileType should handle this quite well since #20418.

Member

xabbuh commented Sep 14, 2017

The FileType should handle this quite well since #20418.

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 14, 2017

Contributor

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

Contributor

enumag commented Sep 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 14, 2017

Member

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?

Member

xabbuh commented Sep 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 14, 2017

Contributor

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

Contributor

enumag commented Sep 14, 2017

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

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 14, 2017

Contributor

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

Contributor

enumag commented Sep 14, 2017

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

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

This comment has been minimized.

@xabbuh

xabbuh Sep 14, 2017

Member

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

@xabbuh

xabbuh Sep 14, 2017

Member

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

This comment has been minimized.

@enumag

enumag Sep 14, 2017

Contributor

@xabbuh Yes.

@enumag

enumag Sep 14, 2017

Contributor

@xabbuh Yes.

This comment has been minimized.

@enumag

enumag Sep 14, 2017

Contributor

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.

@enumag

enumag Sep 14, 2017

Contributor

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.

This comment has been minimized.

@xabbuh

xabbuh Sep 14, 2017

Member

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

@xabbuh

xabbuh Sep 14, 2017

Member

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

This comment has been minimized.

@yceruto

yceruto Sep 14, 2017

Contributor

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

file-bag

according to:

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

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

@yceruto

yceruto Sep 14, 2017

Contributor

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

file-bag

according to:

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

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

This comment has been minimized.

@yceruto

yceruto Sep 14, 2017

Contributor

See test case also:

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());
}

@yceruto

yceruto Sep 14, 2017

Contributor

See test case also:

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

xabbuh approved these changes Sep 14, 2017

looks good to me

@xabbuh xabbuh added this to the 2.7 milestone Sep 14, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 14, 2017

Member

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

Member

xabbuh commented Sep 14, 2017

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

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 14, 2017

Contributor

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

Contributor

enumag commented Sep 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 14, 2017

Member

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

Member

xabbuh commented Sep 14, 2017

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

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 14, 2017

Contributor

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

Contributor

enumag commented Sep 14, 2017

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

@enumag enumag changed the base branch from 2.8 to 2.7 Sep 14, 2017

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 14, 2017

Contributor

@xabbuh All done.

Contributor

enumag commented Sep 14, 2017

@xabbuh All done.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 14, 2017

Contributor

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

Contributor

yceruto commented Sep 14, 2017

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

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 18, 2017

Contributor

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

Contributor

enumag commented Sep 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?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 20, 2017

Member

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.

Member

xabbuh commented Sep 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Sep 20, 2017

Contributor

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

Contributor

enumag commented Sep 20, 2017

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

@fabpot

fabpot approved these changes Sep 30, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 30, 2017

Member

Thank you @enumag.

Member

fabpot commented Sep 30, 2017

Thank you @enumag.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 30, 2017

bug #24198 [HttpFoundation] Fix file upload multiple with no files (e…
…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 referenced Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment