Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
507f82a
[Form] FileType "multiple" fixes
yceruto Nov 2, 2016
59695cf
fixed typo
yceruto Nov 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 36 additions & 5 deletions src/Symfony/Component/Form/Extension/Core/Type/FileType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,37 @@
namespace Symfony\Component\Form\Extension\Core\Type;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\OptionsResolver\Options;
use Symfony\Component\OptionsResolver\OptionsResolver;

class FileType extends AbstractType
{
/**
* {@inheritdoc}
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
if ($options['multiple']) {
$builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event) {
$form = $event->getForm();
$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.

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

Copy link
Member Author

@yceruto yceruto Nov 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 of UploadedFile instances.

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 •
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 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 •
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 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 •
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 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 •
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 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 •
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 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 👍

$emptyData = $form->getConfig()->getEmptyData();

$data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData;
$event->setData($data);
}
});
}
}

/**
* {@inheritdoc}
*/
Expand All @@ -39,20 +64,26 @@ public function buildView(FormView $view, FormInterface $form, array $options)
*/
public function finishView(FormView $view, FormInterface $form, array $options)
{
$view
->vars['multipart'] = true
;
$view->vars['multipart'] = true;
}

/**
* {@inheritdoc}
*/
public function configureOptions(OptionsResolver $resolver)
{
$dataClass = function (Options $options) {
return $options['multiple'] ? null : 'Symfony\Component\HttpFoundation\File\File';
};

$emptyData = function (Options $options) {
return $options['multiple'] ? array() : null;
};

$resolver->setDefaults(array(
'compound' => false,
'data_class' => 'Symfony\Component\HttpFoundation\File\File',
'empty_data' => null,
'data_class' => $dataClass,
'empty_data' => $emptyData,
'multiple' => false,
));
}
Expand Down
27 changes: 27 additions & 0 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/FileTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,33 @@ public function testSubmitEmpty()
$this->assertNull($form->getData());
}

public function testSubmitEmptyMultiple()
{
$form = $this->factory->createBuilder('file', null, array(
'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.

$form->submit(array(null));

$this->assertSame(array(), $form->getData());
}

public function testSetDataMultiple()
{
$form = $this->factory->createBuilder('file', null, array(
'multiple' => true,
))->getForm();

$data = array(
$this->createUploadedFileMock('abcdef', 'first.jpg', true),
$this->createUploadedFileMock('zyxwvu', 'second.jpg', true),
);

$form->setData($data);
$this->assertSame($data, $form->getData());
}

public function testSubmitMultiple()
{
$form = $this->factory->createBuilder('file', null, array(
Expand Down