Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
7e94a7b to2e043a5Compareyceruto commentedNov 5, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Travis CI:PHP: hhvm-stable (Failure unrelated) |
ro0NL commentedNov 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 commentedNov 6, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ro0NL yeah, I had already planned it in a separated PR 👍 Edit: because |
ro0NL commentedNov 6, 2016
Could be one PR if you ask me :) |
yceruto commentedNov 8, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've updated this PR with the 2nd FileType improvement, as it depends on the 1st one. |
ogizanagi commentedNov 8, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@yceruto : This transformer looks strange to me for the use-case (there is no bijective transformation).
Would it be feasible to listen on |
yceruto commentedNov 9, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedNov 9, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
There was a problem hiding this comment.
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 commentedNov 9, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We need be consistent with FileType data flow, otherwise when Edit: How we could guarantee this without the data transformer ? |
a72c5c4 toa85588fCompareyceruto commentedNov 9, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've updated the implementation:
ping@ogizanagi |
a85588f to594da8fCompare| $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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Beware, the empty data could be a callable. Seehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L96-L100
There was a problem hiding this comment.
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 callableempty_data, thanks!
594da8f to0aeb078Compare
ogizanagi left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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, ...)?
There was a problem hiding this comment.
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 ofUploadedFile instances.
ogizanagi commentedNov 10, 2016
Status: Reviewed (bis 😆 ) |
f9abb5a to507f82aCompareyceruto commentedNov 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
typonot required
| 'multiple' =>true, | ||
| ))->getForm(); | ||
| // submitted data when an input file is uploaded without choosing any file |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
yceruto commentedNov 18, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Could anyone remove |
yceruto commentedNov 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) { |
There was a problem hiding this comment.
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), hencemultiple.
So if many inputs are submitted, and only few files are uploaded, we getarray(null, <file>, null, null) for instance. And as the data model is a collection, i guessarray(<file>) is actually expected.
ogizanagiNov 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 aCollectionType ofFileType than a singleFileType withmultiple option set totrue (which is rendered with a single<input type="file" multiple>), right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ogizanagiNov 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 themultiple option is meant to render a single field (thenative HTML input withmultiple attribute).
If you want multiple inputs, you'll use aCollectionType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, but that makes themultiple option pretty much useless, as the collection type doesnt need it. Imo. this PR fixes the filetype when used standalonewith the multiple option..
What's the point innull|File vs.array()|array(File)
There was a problem hiding this comment.
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 themultiple="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. changingname="texttype toname="texttype[weird]", will make it crash already (a good thing);
Expected argument of type "string", "array" givenor without validators;
An exception has been thrown during the rendering of a template ("Notice: Array to string conversion") in form_div_layout.html.twigThere was a problem hiding this comment.
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 withAll() 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 aarray() result withType("array") constraint if submitnull (or anything else) value (instead ofarray(null)) withmultiple="true" (by the way, in this casearray_filter($event->getData()) fails)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 commentedDec 2, 2016
@HeahDude now in 2.7 base branch as "bugfix" any thought about this one? It's ready for @symfony/deciders? |
HeahDude commentedDec 3, 2016
👍 |
xabbuh commentedDec 3, 2016
👍 Status: Reviewed |
fabpot commentedDec 3, 2016
Thank you@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" optionInformation-------------Following this tutorial ["How to Upload Files"][1] but storing many `brochures` instead of one, i.e.:```php// src/AppBundle/Entity/Product.phpclass 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 fileInformation-------------Based on the same information before, but adding some constraints:```php// src/AppBundle/Entity/Product.phpuse 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.phpuse 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.phpuse 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.htmlCommits-------36b7ba6 [Form][DX] FileType "multiple" fixes
…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 withhttps://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

Uh oh!
There was an error while loading.Please reload this page.
(1st) Derive "data_class" option from passed "multiple" option
Information
Following this tutorial"How to Upload Files" but storing many
brochuresinstead of one, i.e.:The Problem
I found a pain point here when the form is loaded again after save some brochures (Exception):
The message is very clear, but counter-intuitive in this case, because the form field (
FileType) was configured withmultiple = true, so IMHO it shouldn't expect aFileinstance but an array of them at all events.The PR's effect
Before:
After:
(2nd) Return empty
array()at submit no fileInformation
Based on the same information before, but adding some constraints:
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 nofalseand noempty()@Assert\File()pass! because the element isnullApart from that really we expecting an empty array instead.
The PR's effect
Before:
After: