-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
7e94a7b
to
2e043a5
Compare
Travis CI: PHP: hhvm-stable (Failure unrelated) |
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 |
@ro0NL yeah, I had already planned it in a separated PR 👍 Edit: because |
Could be one PR if you ask me :) |
I've updated this PR with the 2nd FileType improvement, as it depends on the 1st one. |
@yceruto : This transformer looks strange to me for the use-case (there is no bijective transformation).
Would it be feasible to listen on |
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)? |
@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)
We need be consistent with FileType data flow, otherwise when Edit: How we could guarantee this without the data transformer ? |
a72c5c4
to
a85588f
Compare
I've updated the implementation:
ping @ogizanagi |
a85588f
to
594da8f
Compare
$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. See https://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 callable empty_data
, thanks!
594da8f
to
0aeb078
Compare
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 of UploadedFile
instances.
Status: Reviewed (bis 😆 ) |
f9abb5a
to
507f82a
Compare
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.
typo not 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.
Could anyone remove |
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), 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.
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 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?
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.
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 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
.
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 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)
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 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
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 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)
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 👍
@HeahDude now in 2.7 base branch as "bugfix" any thought about this one? It's ready for @symfony/deciders? |
👍 |
👍 Status: Reviewed |
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" 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
…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
(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.: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 aFile
instance 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 nofalse
and noempty()
@Assert\File()
pass! because the element isnull
Apart from that really we expecting an empty array instead.
The PR's effect
Before:
After: