[Form] Fix Array to string conversion

[enumag]

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Let's have a form with a textarea and some other field. Open the form in browser, open debug tools and add [] to the name attribute of the textarea. Put some invalid value into the other field. The result is an Array to string conversion on this line in TwigBridge. It's of course because the submitted value of the textarea is actually an array because of our manipulation via debug tools.

No matter what the user input is Symfony should not fail on an error like this which is why I consider this a bug.

I'm not quite sure if overwriting the submitted value and adding an error in a PRE_SUBMIT event is a good fix for this issue. Please share any suggestions.

Of course the current implementation is far from ready - this should be solved by a form extension for all fields except ChoiceType with multiple option and CollectionType. Those on the other hand need some protection against non-array values.

By the way adding a Type("string") constraint to the field does not help (obviously).

[HeahDude]

@enumag Could you please first add a failing test case for this?

Something like:

$form = $this->factory->create('text', 'test');

$form->submit(array());

$this->assertSame('test', $form->getData());
$this->assertFalse($form->isValid());

This listener sounds like a bad idea (also for performances), this should be checked in Form::submit() instead IMHO, like this is done for compound forms here

symfony/src/Symfony/Component/Form/Form.php

Line 555 in d88f2a4

throw new TransformationFailedException('Compound forms expect an array or NULL on submission.');

, or maybe here

symfony/src/Symfony/Component/Form/Form.php

Line 1116 in d88f2a4

private function viewToNorm($value)

when no view transformers are set.

But since not compound forms can be submitted an array (like a multiple non expanded ChoiceType as you pointed out), it means that we should use an internal option to define the behavior (accepting array or not), anyway I can't find a better solution right now.

[enumag]

@HeahDude Good suggestions although far from solving the issue. The problem with failing test is that checking if the form is not valid is NOT enough. It will still cause the error when rendering the invalid form. But I can't add a test for rendering because the form component itself has no templates.

I believe it's necessary to actually throw away the submitted value as it's not only invalid but also can't be used for rendering - the test should check that the value got trashed AND the form is invalid. Where do you think this should happen? (Probably based on the internal option.)

[HeahDude]

If my test above is green (which should not currently), then the issue should be solved, meaning we should prevent array() === $form->getData().

I've linked two places where I think we can handle this in the Form class. But others can have a better idea.

[yceruto]

I guess that should be fixed in 2.7 base branch?

[yceruto]

Note that FileType uses the multiple attribute as well, it don't use data transformer and expect an array() also.

[xabbuh]

@HeahDude We could introduce an option that can be used by form types to define the accepted types when the type is not compound. Before Symfony 4.0 the default value would be null which doesn't do anything, but we would already deprecate the null value. In 4.0, we would then disallow null and make string the new default value.

[Tobion]

Related to #4102

[lobodol]

I suggest you to add error this way:

new FormError('This value should be of type {{ type }}.', null, ['{{ type }}' => 'string'])

Thus, it uses an already existing error message which is convenient for translations.
What do you think about it ?

[enumag]

This solution is completely wrong. Read the comments above for details.

[nicolas-grekas]

Closing in favor of #29307