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

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

Closed
wants to merge 2 commits into
base: 2.7
from

Conversation

Projects
None yet
8 participants
@yceruto
Contributor

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;
}
@yceruto

This comment has been minimized.

Contributor

yceruto commented Nov 5, 2016

@ro0NL

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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 from [Form][DX] FileType derive "data_class" option from passed "multiple" option to [Form][DX] FileType improvements Nov 8, 2016

@yceruto

This comment has been minimized.

Contributor

yceruto commented Nov 8, 2016

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

@ogizanagi

This comment has been minimized.

Member

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

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

This comment has been minimized.

@ogizanagi

ogizanagi Nov 9, 2016

Member
- without choice any file
+ without choosing any file

? (can be inlined with previous one also IMHO)

@yceruto

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

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

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

This comment has been minimized.

This comment has been minimized.

@yceruto

yceruto Nov 9, 2016

Contributor

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

@ogizanagi

ogizanagi approved these changes Nov 9, 2016 edited

👍

Status: Reviewed

$data = $event->getData();
// submitted data for an input file (no required) without choosing any file
if (array(null) === $data) {

This comment has been minimized.

@ro0NL

ro0NL Nov 9, 2016

Contributor

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

This comment has been minimized.

@yceruto

yceruto Nov 9, 2016

Contributor

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

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Nov 10, 2016

Status: Reviewed

(bis 😆 )

@yceruto yceruto changed the title from [Form][DX] FileType improvements to [Form][DX] FileType "multiple" improvements Nov 10, 2016

@HeahDude

This comment has been minimized.

Member

HeahDude commented Nov 15, 2016

👍

Nice job with this PR @yceruto! What about having this merged in 2.7 as a bug fix instead?

@yceruto

This comment has been minimized.

Contributor

yceruto commented Nov 15, 2016

For me will be great to add this one since 2.7 (Rebasing...)

Status: Needs Work

@yceruto yceruto changed the base branch from master to 2.7 Nov 15, 2016

@yceruto yceruto changed the base branch from master to 2.7 Nov 16, 2016

@yceruto yceruto changed the title from [Form][DX] FileType "multiple" improvements to [Form][DX] FileType "multiple" fixes Nov 16, 2016

@yceruto

This comment has been minimized.

Contributor

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

This comment has been minimized.

@HeahDude

HeahDude Nov 16, 2016

Member

typo not required

'multiple' => true,
))->getForm();
// submitted data when an input file is uploaded without choosing any file

This comment has been minimized.

@HeahDude

HeahDude Nov 16, 2016

Member

does it deserve a test case without default required?

This comment has been minimized.

@yceruto

yceruto Nov 22, 2016

Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

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) {

This comment has been minimized.

@ro0NL

ro0NL Nov 22, 2016

Contributor

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.

This comment has been minimized.

@ogizanagi

ogizanagi Nov 22, 2016

Member

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?

This comment has been minimized.

@ro0NL

ro0NL Nov 22, 2016

Contributor

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?

This comment has been minimized.

@ogizanagi

ogizanagi Nov 22, 2016

Member

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.

This comment has been minimized.

@ro0NL

ro0NL Nov 22, 2016

Contributor

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)

This comment has been minimized.

@ro0NL

ro0NL Nov 23, 2016

Contributor

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

This comment has been minimized.

@yceruto

yceruto Nov 23, 2016

Contributor

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)

This comment has been minimized.

@ro0NL

ro0NL Nov 23, 2016

Contributor

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.

This comment has been minimized.

@yceruto

yceruto Nov 23, 2016

Contributor

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.

This comment has been minimized.

@ro0NL

ro0NL Nov 23, 2016

Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

HeahDude commented Dec 3, 2016

👍

@xabbuh

This comment has been minimized.

Member

xabbuh commented Dec 3, 2016

👍

Status: Reviewed

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 3, 2016

Thank you @yceruto.

fabpot added a commit that referenced this pull request Dec 3, 2016

bug #20418 [Form][DX] FileType "multiple" fixes (yceruto)
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 yceruto:form/file-type-dx-improvement branch Dec 5, 2016

fabpot added a commit that referenced this pull request Feb 1, 2018

bug #25948 [Form] Fixed empty data on expanded ChoiceType and FileTyp…
…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