Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedNov 5, 2016
edited
Loading

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#12547
LicenseMIT
Doc PR-

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

Information

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

// src/AppBundle/Entity/Product.phpclass 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 withmultiple = true, so IMHO it shouldn't expect aFile 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 emptyarray() at submit no file

Information

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

// src/AppBundle/Entity/Product.phpuseSymfony\Component\Validator\ConstraintsasAssert;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 wasarray(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:

// src/AppBundle/Entity/Product.phpuseSymfony\Component\Validator\ConstraintsasAssert;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.phpuseSymfony\Component\Validator\ConstraintsasAssert;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;}

ogizanagi, javiereguiluz, chalasr, apfelbox, ro0NL, Koc, and yceruto reacted with thumbs up emoji
@carsonbotcarsonbot added Status: Needs Review Form DXDX = Developer eXperience (anything that improves the experience of using Symfony) Feature labelsNov 5, 2016
@ycerutoycerutoforce-pushed theform/file-type-dx-improvement branch from7e94a7b to2e043a5CompareNovember 5, 2016 19:48
@yceruto
Copy link
MemberAuthor

yceruto commentedNov 5, 2016
edited
Loading

@ro0NL
Copy link
Contributor

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
MemberAuthor

yceruto commentedNov 6, 2016
edited
Loading

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

Edit: becauseempty_data doesn't work when no file is uploaded (withmultiple = true it submitarray(null) instead ofnull orarray()) so we need a transformer to fix that and validate also the array ofFile andUploadedFile .

@ro0NL
Copy link
Contributor

Could be one PR if you ask me :)

@javiereguiluzjaviereguiluz added this to the3.2 milestoneNov 7, 2016
@ycerutoyceruto changed the title[Form][DX] FileType derive "data_class" option from passed "multiple" option[Form][DX] FileType improvementsNov 8, 2016
@yceruto
Copy link
MemberAuthor

yceruto commentedNov 8, 2016
edited
Loading

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

@ogizanagi
Copy link
Contributor

ogizanagi commentedNov 8, 2016
edited
Loading

@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 onFormEvents::PRE_SUBMIT and set an empty array (or even$form->getConfig()->getEmptyData() result) if the event data isarray(null) ?

@yceruto
Copy link
MemberAuthor

yceruto commentedNov 9, 2016
edited
Loading

@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 commentedNov 9, 2016
edited
Loading

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

yceruto reacted with thumbs up emoji

))->getForm();

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

@ogizanagiogizanagiNov 9, 2016
edited
Loading

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 reacted with thumbs up emoji
@yceruto
Copy link
MemberAuthor

yceruto commentedNov 9, 2016
edited
Loading

We need be consistent with FileType data flow, otherwise whenmultiple = 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 ?

@ycerutoycerutoforce-pushed theform/file-type-dx-improvement branch 2 times, most recently froma72c5c4 toa85588fCompareNovember 9, 2016 14:25
@yceruto
Copy link
MemberAuthor

yceruto commentedNov 9, 2016
edited
Loading

I've updated the implementation:

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

ping@ogizanagi

@ycerutoycerutoforce-pushed theform/file-type-dx-improvement branch froma85588f to594da8fCompareNovember 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.

yceruto reacted with thumbs up emoji
Copy link
MemberAuthor

@ycerutoycerutoNov 9, 2016
edited
Loading

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!

@ycerutoycerutoforce-pushed theform/file-type-dx-improvement branch from594da8f to0aeb078CompareNovember 9, 2016 15:46
Copy link
Contributor

@ogizanagiogizanagi left a comment
edited
Loading

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
MemberAuthor

@ycerutoycerutoNov 9, 2016
edited
Loading

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
Copy link
Contributor

Status: Reviewed

(bis 😆 )

Simperfit and yceruto reacted with thumbs up emojiSimperfit, yceruto, and chalasr reacted with laugh emoji

@ycerutoyceruto changed the title[Form][DX] FileType improvements[Form][DX] FileType "multiple" improvementsNov 10, 2016
@ycerutoyceruto changed the title[Form][DX] FileType "multiple" improvements[Form][DX] FileType "multiple" fixesNov 16, 2016
@ycerutoycerutoforce-pushed theform/file-type-dx-improvement branch fromf9abb5a to507f82aCompareNovember 16, 2016 12:43
@yceruto
Copy link
MemberAuthor

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.

typonot required

yceruto reacted with thumbs up emoji
'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?

yceruto reacted with confused emoji
Copy link
MemberAuthor

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.

@fabpotfabpot removed this from the3.2 milestoneNov 16, 2016
@yceruto
Copy link
MemberAuthor

yceruto commentedNov 18, 2016
edited
Loading

Could anyone removeFeature and addBug label, please ? Thanks.

@javiereguiluzjaviereguiluz added Bug and removed Feature labelsNov 18, 2016
@yceruto
Copy link
MemberAuthor

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

Thank you@ogizanagi for you previous review.

ogizanagi reacted with thumbs up emoji

$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), 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.

Copy link
Contributor

@ogizanagiogizanagiNov 22, 2016
edited
Loading

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?

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, themultiple 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

@ogizanagiogizanagiNov 22, 2016
edited
Loading

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.

Copy link
Contributor

@ro0NLro0NLNov 22, 2016
edited
Loading

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)

Copy link
Contributor

@ro0NLro0NLNov 23, 2016
edited
Loading

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" 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
MemberAuthor

@ycerutoycerutoNov 23, 2016
edited
Loading

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)

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
MemberAuthor

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 reacted with hooray emoji
@yceruto
Copy link
MemberAuthor

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

It's ready for @symfony/deciders?

@HeahDude
Copy link
Contributor

👍

@xabbuh
Copy link
Member

👍

Status: Reviewed

@fabpot
Copy link
Member

Thank you@yceruto.

fabpot added a commit that referenced this pull requestDec 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" 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
@fabpotfabpot closed thisDec 3, 2016
@ycerutoyceruto deleted the form/file-type-dx-improvement branchDecember 5, 2016 15:23
This was referencedDec 13, 2016
fabpot added a commit that referenced this pull requestFeb 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 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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

3 more reviewers

@ro0NLro0NLro0NL left review comments

@ogizanagiogizanagiogizanagi approved these changes

@HeahDudeHeahDudeHeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

BugDXDX = Developer eXperience (anything that improves the experience of using Symfony)FormStatus: Reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@yceruto@ro0NL@ogizanagi@HeahDude@xabbuh@fabpot@javiereguiluz@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp