Skip to content

[Form][DX] FileType "multiple" fixes #20418

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

Closed
wants to merge 2 commits into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Nov 5, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12547
License MIT
Doc PR -

(1st) Derive "data_class" option from passed "multiple" option

Information

Following this tutorial "How to Upload Files" but storing many brochures instead of one, i.e.:

// src/AppBundle/Entity/Product.php

class Product 
{
    /**
     * @var string[]
     *
     * @ORM\Column(type="array")
     */
    private $brochures;
    
    //...
}
//src/AppBundle/Form/ProductType.php

$builder->add('brochures', FileType::class, array(
    'label' => 'Brochures (PDF files)',
    'multiple' => true,
));

The Problem

I found a pain point here when the form is loaded again after save some brochures (Exception):

The form's view data is expected to be an instance of class Symfony\Component\HttpFoundation\File\File, but is a(n) array. You can avoid this error by setting the "data_class" option to null or by adding a view transformer that transforms a(n) array to an instance of Symfony\Component\HttpFoundation\File\File.

The message is very clear, but counter-intuitive in this case, because the form field (FileType) was configured with multiple = true, so IMHO it shouldn't expect a File instance but an array of them at all events.

The PR's effect

Before:

$form = $this->createFormBuilder($product)
    ->add('brochures', FileType::class, [
        'multiple' => true,
	'data_class' => null, // <---- mandatory
    ])
    ->getForm();

After:

$form = $this->createFormBuilder($product)
    ->add('brochures', FileType::class, [
        'multiple' => true,
    ])
    ->getForm();

(2nd) Return empty array() at submit no file

Information

Based on the same information before, but adding some constraints:

// src/AppBundle/Entity/Product.php

use Symfony\Component\Validator\Constraints as Assert;

class Product 
{
    /**
     * @var string[]
     *
     * @ORM\Column(type="array")
     *
     * @Assert\Count(min="1") // or @Assert\NotBlank()
     * @Assert\All({
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     * 
     */
    private $brochures;
}

This should require at least one file to be stored.

The Problem

But, when no file is uploaded at submit the form, it's valid completely. The submitted data for this field was array(null) so the constraints pass without any problem:

  • @Assert\Count(min="1") pass! because contains at least one element (No matter what)
  • @Assert\NotBlank() it could pass! because no false and no empty()
  • @Assert\File() pass! because the element is null

Apart from that really we expecting an empty array instead.

The PR's effect

Before:

// src/AppBundle/Entity/Product.php

use Symfony\Component\Validator\Constraints as Assert;

class Product 
{
    /**
     * @var string[]
     *
     * @ORM\Column(type="array")
     *
     * @Assert\All({
     *     @Assert\NotBlank,
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     * 
     */
    private $brochures;
}

After:

// src/AppBundle/Entity/Product.php

use Symfony\Component\Validator\Constraints as Assert;

class Product 
{
    /**
     * @var string[]
     *
     * @ORM\Column(type="array")
     *
     * @Assert\Count(min="1") // or @Assert\NotBlank
     * @Assert\All({
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     * 
     */
    private $brochures;
}

@carsonbot carsonbot added Status: Needs Review Form DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Nov 5, 2016
@yceruto yceruto force-pushed the form/file-type-dx-improvement branch from 7e94a7b to 2e043a5 Compare November 5, 2016 19:48
@yceruto
Copy link
Member Author

yceruto commented Nov 5, 2016

Travis CI: PHP: hhvm-stable (Failure unrelated)

@ro0NL
Copy link
Contributor

ro0NL commented Nov 6, 2016

Should we also return an empty array, if multiple? ie. like the choice type does: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L244

@yceruto
Copy link
Member Author

yceruto commented Nov 6, 2016

@ro0NL yeah, I had already planned it in a separated PR 👍

Edit: because empty_data doesn't work when no file is uploaded (with multiple = true it submit array(null) instead of null or array()) so we need a transformer to fix that and validate also the array of File and UploadedFile .

@ro0NL
Copy link
Contributor

ro0NL commented Nov 6, 2016

Could be one PR if you ask me :)

@javiereguiluz javiereguiluz added this to the 3.2 milestone Nov 7, 2016
@yceruto yceruto changed the title [Form][DX] FileType derive "data_class" option from passed "multiple" option [Form][DX] FileType improvements Nov 8, 2016
@yceruto
Copy link
Member Author

yceruto commented Nov 8, 2016

I've updated this PR with the 2nd FileType improvement, as it depends on the 1st one.

@ogizanagi
Copy link
Contributor

ogizanagi commented Nov 8, 2016

@yceruto : This transformer looks strange to me for the use-case (there is no bijective transformation).

Edit: because empty_data doesn't work when no file is uploaded (with multiple = true it submit array(null) instead of null or array()) so we need a transformer to fix that and validate also the array of File and UploadedFile .

Would it be feasible to listen on FormEvents::PRE_SUBMIT and set an empty array (or even $form->getConfig()->getEmptyData() result) if the event data is array(null) ?

@yceruto
Copy link
Member Author

yceruto commented Nov 9, 2016

@ogizanagi:

Would it be feasible to listen on FormEvents::PRE_SUBMIT and set an empty array (or even $form->getConfig()->getEmptyData() result) if the event data is array(null) ?

Yes! could be (my first thought was that) but, what about the data flow validation? Do you think that transformer should be removed completely (even when there is no bijective transformation)?

@ogizanagi
Copy link
Contributor

ogizanagi commented Nov 9, 2016

@yceruto : I think it should be removed completely. It's not the data transformer's responsibility to validate the data, except when it prevents transformation. There is no transformation here. ^^'

))->getForm();

// submitted data when an input file is uploaded
// without choice any file
Copy link
Contributor

@ogizanagi ogizanagi Nov 9, 2016

Choose a reason for hiding this comment

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

- without choice any file
+ without choosing any file

? (can be inlined with previous one also IMHO)

@yceruto
Copy link
Member Author

yceruto commented Nov 9, 2016

We need be consistent with FileType data flow, otherwise when multiple = true any data type can be passed to this one (even when it do nothing with these values).

Edit: How we could guarantee this without the data transformer ?

@yceruto yceruto force-pushed the form/file-type-dx-improvement branch 2 times, most recently from a72c5c4 to a85588f Compare November 9, 2016 14:25
@yceruto
Copy link
Member Author

yceruto commented Nov 9, 2016

I've updated the implementation:

  • Removed DataTransformer
  • Added default 'empty_data' => array() when 'multiple' => true
  • Added PRE_SUBMIT listener to change array(null) to default empty data.

ping @ogizanagi

@yceruto yceruto force-pushed the form/file-type-dx-improvement branch from a85588f to 594da8f Compare November 9, 2016 14:48
$builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) {
// submitted data for an input file (no required) without choosing any file
if (array(null) === $event->getData()) {
$event->setData($event->getForm()->getConfig()->getEmptyData());
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@yceruto yceruto Nov 9, 2016

Choose a reason for hiding this comment

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

Opps! updated, added support to callable empty_data, thanks!

@yceruto yceruto force-pushed the form/file-type-dx-improvement branch from 594da8f to 0aeb078 Compare November 9, 2016 15:46
Copy link
Contributor

@ogizanagi ogizanagi left a comment

Choose a reason for hiding this comment

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

👍

Status: Reviewed

$data = $event->getData();

// submitted data for an input file (no required) without choosing any file
if (array(null) === $data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expect multiple nulls here? array(null, null, ...)?

Copy link
Member Author

@yceruto yceruto Nov 9, 2016

Choose a reason for hiding this comment

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

Nope! array(null) mean no file uploaded, otherwise we expect array of UploadedFile instances.

@ogizanagi
Copy link
Contributor

Status: Reviewed

(bis 😆 )

@yceruto yceruto changed the title [Form][DX] FileType improvements [Form][DX] FileType "multiple" improvements Nov 10, 2016
@yceruto yceruto changed the title [Form][DX] FileType "multiple" improvements [Form][DX] FileType "multiple" fixes Nov 16, 2016
@yceruto yceruto force-pushed the form/file-type-dx-improvement branch from f9abb5a to 507f82a Compare November 16, 2016 12:43
@yceruto
Copy link
Member Author

yceruto commented Nov 16, 2016

Rebased to 2.7 finished 😅

Status: Needs Review

$form = $event->getForm();
$data = $event->getData();

// submitted data for an input file (no required) without choosing any file
Copy link
Contributor

Choose a reason for hiding this comment

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

typo not required

'multiple' => true,
))->getForm();

// submitted data when an input file is uploaded without choosing any file
Copy link
Contributor

Choose a reason for hiding this comment

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

does it deserve a test case without default required?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is often the case, for example, when editing a form requesting to change the related image optionally.

@fabpot fabpot removed this from the 3.2 milestone Nov 16, 2016
@yceruto
Copy link
Member Author

yceruto commented Nov 18, 2016

Could anyone remove Feature and add Bug label, please ? Thanks.

@javiereguiluz javiereguiluz added Bug and removed Feature labels Nov 18, 2016
@yceruto
Copy link
Member Author

yceruto commented Nov 22, 2016

It's ready for review again now in 2.7 branch.

Thank you @ogizanagi for you previous review.

$data = $event->getData();

// submitted data for an input file (not required) without choosing any file
if (array(null) === $data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Still not sure about this, by default only 1 <input> is rendered, but it can in fact be many (ie. created by the client with a "Add file" button), hence multiple.

So if many inputs are submitted, and only few files are uploaded, we get array(null, <file>, null, null) for instance. And as the data model is a collection, i guess array(<file>) is actually expected.

Copy link
Contributor

@ogizanagi ogizanagi Nov 22, 2016

Choose a reason for hiding this comment

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

What you describe here looks more like a CollectionType of FileType than a single FileType with multiple option set to true (which is rendered with a single <input type="file" multiple>), right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess im not sure this involves / should involve the collection type, the multiple option only works with it?

What if multiple inputs are there somehow.. we currently get on submit

image

Imo. we should filter nulls, or we shouldnt (not only a single null). Maybe make it an option?

Copy link
Contributor

@ogizanagi ogizanagi Nov 22, 2016

Choose a reason for hiding this comment

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

What if multiple inputs are there somehow..

But it shouldn't. No matter how IMHO. 😅
FileType with the multiple option is meant to render a single field (the native HTML input with multiple attribute).

If you want multiple inputs, you'll use a CollectionType.

Copy link
Contributor

@ro0NL ro0NL Nov 22, 2016

Choose a reason for hiding this comment

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

Yeah, but that makes the multiple option pretty much useless, as the collection type doesnt need it. Imo. this PR fixes the filetype when used standalone with the multiple option..

What's the point in null|File vs. array()|array(File)

Copy link
Contributor

@ro0NL ro0NL Nov 23, 2016

Choose a reason for hiding this comment

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

Not sure it needs a data transformer, as this is only about filtering out null values. Again; to enforce the multiple="true" effect server-side. It's an edge case.

This does not apply in any way to other types. Doing weird things with textypes, ie. changing name="texttype to name="texttype[weird]", will make it crash already (a good thing);

Expected argument of type "string", "array" given

or without validators;

An exception has been thrown during the rendering of a template ("Notice: Array to string conversion") in form_div_layout.html.twig

Copy link
Member Author

@yceruto yceruto Nov 23, 2016

Choose a reason for hiding this comment

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

this is only about filtering out null values... to enforce the multiple="true" effect server-side. It's an edge case.

Imho it's already cover, we can avoid that with All() constraint (See in description):

     * @Assert\All({
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     */
    private $brochures;

Also by manipulating the DOM or doing a raw request we can guarantee a array() result with Type("array") constraint if submit null (or anything else) value (instead of array(null)) with multiple="true" (by the way, in this case array_filter($event->getData()) fails)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we're letting the user deal with it then. Just saying, not everybody probably expects this behavior (leaving a hole in your architecture).

But we'er good then.

Copy link
Member Author

Choose a reason for hiding this comment

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

not everybody probably expects this behavior.

Exactly, the form type only should cover the normal expected behavior.

leaving a hole in your architecture

AFAIK this hole is responsibility of constraints/validators.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, the form type only should cover the normal expected behavior.

Imo. it should define consistent behavior :)

AFAIK this hole is responsibility of constraints/validators.

Just saying we could avoid the whole thing by filtering out nulls out-of-the-box, having consistent behavior :) But im fine with keeping it in user-land as well.

The point is clear, thanks for letting me clarify!

if everbody's in favor with this approach, im as well 👍

@yceruto
Copy link
Member Author

yceruto commented Dec 2, 2016

@HeahDude now in 2.7 base branch as "bugfix" any thought about this one?

It's ready for @symfony/deciders?

@HeahDude
Copy link
Contributor

HeahDude commented Dec 3, 2016

👍

@xabbuh
Copy link
Member

xabbuh commented Dec 3, 2016

👍

Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Dec 3, 2016

Thank you @yceruto.

fabpot added a commit that referenced this pull request Dec 3, 2016
This PR was squashed before being merged into the 2.7 branch (closes #20418).

Discussion
----------

[Form][DX] FileType "multiple" fixes

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12547
| License       | MIT
| Doc PR        | -

# (1st) Derive "data_class" option from passed "multiple" option

Information
-------------

Following this tutorial ["How to Upload Files"][1] but storing many `brochures` instead of one, i.e.:

```php
// src/AppBundle/Entity/Product.php

class Product
{
    /**
     * @var string[]
     *
     * @Orm\Column(type="array")
     */
    private $brochures;

    //...
}
```

```php
//src/AppBundle/Form/ProductType.php

$builder->add('brochures', FileType::class, array(
    'label' => 'Brochures (PDF files)',
    'multiple' => true,
));
```

The Problem
--------------

I found a pain point here when the form is loaded again after save some brochures (Exception):

> The form's view data is expected to be an instance of class Symfony\Component\HttpFoundation\File\File, but is a(n) array. You can avoid this error by setting the "data_class" option to null or by adding a view transformer that transforms a(n) array to an instance of Symfony\Component\HttpFoundation\File\File.

The message is very clear, but counter-intuitive in this case, because the form field (`FileType`) was configured with `multiple = true`, so IMHO it shouldn't expect a `File` instance but an array of them at all events.

The PR's effect
---------------

**Before:**

```php
$form = $this->createFormBuilder($product)
    ->add('brochures', FileType::class, [
        'multiple' => true,
	'data_class' => null, // <---- mandatory
    ])
    ->getForm();
```

**After:**

```php
$form = $this->createFormBuilder($product)
    ->add('brochures', FileType::class, [
        'multiple' => true,
    ])
    ->getForm();
```

# (2nd) Return empty `array()` at submit no file

Information
-------------

Based on the same information before, but adding some constraints:

```php
// src/AppBundle/Entity/Product.php

use Symfony\Component\Validator\Constraints as Assert;

class Product
{
    /**
     * @var string[]
     *
     * @Orm\Column(type="array")
     *
     * @Assert\Count(min="1") // or @Assert\NotBlank()
     * @Assert\All({
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     *
     */
    private $brochures;
}
```

This should require at least one file to be stored.

The Problem
--------------

But, when no file is uploaded at submit the form, it's valid completely. The submitted data for this field was `array(null)` so the constraints pass without any problem:

* `@Assert\Count(min="1")` pass! because contains at least one element (No matter what)
* `@Assert\NotBlank()` it could pass! because no `false` and no `empty()`
* `@Assert\File()` pass! because the element is `null`

Apart from that really we expecting an empty array instead.

The PR's effect
----------------

**Before:**

```php
// src/AppBundle/Entity/Product.php

use Symfony\Component\Validator\Constraints as Assert;

class Product
{
    /**
     * @var string[]
     *
     * @Orm\Column(type="array")
     *
     * @Assert\All({
     *     @Assert\NotBlank,
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     *
     */
    private $brochures;
}
```

**After:**

```php
// src/AppBundle/Entity/Product.php

use Symfony\Component\Validator\Constraints as Assert;

class Product
{
    /**
     * @var string[]
     *
     * @Orm\Column(type="array")
     *
     * @Assert\Count(min="1") // or @Assert\NotBlank
     * @Assert\All({
     *     @Assert\File(mimeTypes = {"application/pdf", "application/x-pdf"})
     * })
     *
     */
    private $brochures;
}
```

  [1]: http://symfony.com/doc/current/controller/upload_file.html

Commits
-------

36b7ba6 [Form][DX] FileType "multiple" fixes
@fabpot fabpot closed this Dec 3, 2016
@yceruto yceruto deleted the form/file-type-dx-improvement branch December 5, 2016 15:23
fabpot added a commit that referenced this pull request Feb 1, 2018
…e (HeahDude)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fixed empty data on expanded ChoiceType and FileType

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25896
| License       | MIT
| Doc PR        | ~

Alternative of #25924.

I don't think we have to do this by adding overhead in master and getting it properly fixed in 2 years.

This is bug I've introduced while fixing another bug #17822. Which then has been replicated in #20418 and #24993.

I propose instead to clean up the code for all LTS, since this is a wrong behaviour that has never been documented, and most of all unreliable.
The `empty_data` can by anything in the view format as long as the reverse view transformation supports it. Even an object derived from the data class could be invokable.

I think we should remain consistent with https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Form/Form.php#L615.

Commits
-------

9722c06 [Form] Fixed empty data on expanded ChoiceType and FileType
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug DX DX = Developer eXperience (anything that improves the experience of using Symfony) Form Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants