[Form] weaken the rejection of submitted array data #29911

xabbuh · 2019-01-16T22:07:23Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#29809, #29841, #29905
License	MIT
Doc PR

It seems that the bugfix made in #29307 seems to break quite some applications. I therefore suggest that we solve the issue a bit different by introducing a new accept_multiple_values option. This value will be null by default which means no checks will be performed for backwards compatibility. The core form types do explicitly set this value to false if they expect the submitted data to be strings. If a form is compound, the option value will automatically default to true.

Since from my experience most custom form types have child forms, they are already compound and so do accept arrays. Nothing will change for them. Other custom types will not have the check as long as they do not extend one of the built-in types that have this check enabled.

In Symfony 4.3 we can then deprecate the null default value so that each type must opt for either true or false as the value in 5.0.

xabbuh · 2019-01-16T22:11:52Z

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

@@ -328,6 +328,8 @@ public function configureOptions(OptionsResolver $resolver)
            'placeholder' => $placeholderDefault,
            'error_bubbling' => false,
            'compound' => $compound,
+            // submitted arrays are dealt with in data transformers
+            'accept_multiple_values' => true,


Not sure if that's the best name. I could also imagine something like accept_array_data.

Suggestion: accept_unstructured_data? "Array" could be mistaken for a flat array without special string keys, just like multiple values on a TextType.

'scalar' => false?

nicolas-grekas · 2019-01-17T06:35:52Z

Will that make the reported breaks work again? Eg submitting a json-decoded array in a TextType?
I'm not sure we should do this for now: input data should always be strictly validated. Allowing arrays was a really nasty behavior, on the edge of a security risk maybe for some.

OskarStark · 2019-01-17T07:47:47Z

bugfix made in # seems to break

which PR are you talking about? 🤔

althaus · 2019-01-17T08:00:20Z

@OskarStark See #29809 for the main discussion triggered by PR #29307 .

althaus · 2019-01-17T08:04:48Z

The core form types do explicitly set this value to false if they expect the submitted data to be strings.

What'd the behaviour of the null form type which defaults to TextType? My reading is that those "evil" forms are still broken?

xabbuh · 2019-01-17T08:23:39Z

@nicolas-grekas As long as you do not explicitly set the accept_multiple_values to true yourself array values would still be rejected (that's why we need to be explicit about this in the core form types).

But what this will allow easily again having a custom type like this:

class UnstructuredType extends AbstractType
{
}

@althaus Yes, if you are not explicitly about the type being used and the TextType is the result of the guessing process, this PR does not change anything for you.

alexander-schranz · 2019-01-17T09:48:41Z

@xabbuh if I'm interpereting it correctly I could use for unstructured data this then:

use Symfony\Component\Form\Extension\Core\Type\FormType;

$builder->add('some_unstructured_data', FormType::class');

xabbuh · 2019-01-17T09:55:40Z

@alexander-schranz Yes, that should work (that basically is the same as this test: https://github.com/symfony/symfony/pull/29911/files#diff-de65be85a5affed1f499b79d96965419R1062). But I'd be happy if you could confirm that in your application context.

alexander-schranz · 2019-01-17T10:02:03Z

src/Symfony/Component/Form/Tests/CompoundFormTest.php

+        $this->form->add($this->getBuilder('foo', null, null, ['compound' => true])->getForm());
+
+        $this->form->submit([
+            'foo' => ['foo'],


would also add test for a nested array not just a list:

[ 'foo' => [ 'foo' => 'foo', 'bar' => [ 'baz' => 'baz', ] ] ]

@xabbuh

alexander-schranz · 2019-01-17T10:06:23Z

@xabbuh will test it in the application at the evening. Thank you for having a look at the issue!

xabbuh · 2019-01-17T21:05:18Z

@alexander-schranz makes sense, done

alexander-schranz · 2019-01-20T23:44:14Z

src/Symfony/Component/Form/Extension/Core/Type/TextType.php

@@ -37,6 +37,7 @@ public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'compound' => false,
+            'accept_multiple_values' => false,


@xabbuh this should set to true until 5.0 to avoid the bc break which was introduce in 3.4.21, else applications which did work before 3.4.21 still would crash.

nicolas-grekas · 2019-01-21T06:46:39Z

I'm not sure at all we should do this change.
The behavior change we introduced is a bug fix.
I'm sorry some "hacked" the component and are caught by the fix, but every bug fix does that potentially.
Does this PR provide something that cannot be achieved otherwise?
Adding a new option means added complexity for everyone, better think twice about it.

althaus · 2019-01-21T07:55:37Z

I'm sorry some "hacked" the component

I'm pretty sure that 9 out of 10 didn't do this on purpose, but either by accident or due to lacking docs back then. The Form component really can be a b**ch sometimes. It's the only one forcing us to rework code parts every now and then due to some "fix". :/

I can also see that adding a fire-and-forget option just for this small thing on all those classes could raise more headache than it solves. So I'd be glad if someone can imagine a solution working for both sides.

fnagel · 2019-01-21T09:37:13Z

I'm sorry some "hacked" the component and are caught by the fix, but every bug fix does that potentially.

True that, but even big bundles like RestBundle using that "hack" and recommend it in their docs.

nicolas-grekas · 2019-01-21T09:44:32Z

True that, but even big bundles like RestBundle using that "hack" and recommend it in their docs.

time to fix them!

dmaicher · 2019-01-21T19:43:25Z

I'm also not sure I would fix this by adding another option. I find it too confusing to have

compound
multiple
accept_multiple_values (or whatever new name we pick)

In the end the fix for all affected users would be something like this here, or not?
sulu/sulu#4349

xabbuh · 2019-01-21T19:54:35Z

@dmaicher Basically yes, except that the type could be shortened to not have to include the configureOptions() method at all.

xabbuh · 2019-01-25T10:10:46Z

closing here as it doesn't look like the proposed change is of interest

xabbuh added this to the 3.4 milestone Jan 16, 2019

carsonbot added Status: Needs Review Form Bug labels Jan 16, 2019

xabbuh force-pushed the issue-29809 branch from 43ecc11 to d51aea3 Compare January 16, 2019 22:09

xabbuh commented Jan 16, 2019

View reviewed changes

This was referenced Jan 16, 2019

Form Validation Error for Array input #29905

Closed

FormType mapping for array on TextType not longer working #29809

Closed

alexander-schranz reviewed Jan 17, 2019

View reviewed changes

dpfaffenbauer mentioned this pull request Jan 17, 2019

"settings: this value is not valid" error when saving new Executable dpfaffenbauer/ProcessManager#28

Closed

weaken the rejection of submitted array data

3ea6f14

xabbuh force-pushed the issue-29809 branch from d51aea3 to 3ea6f14 Compare January 17, 2019 21:05

xabbuh referenced this pull request Jan 18, 2019

[Form] Filter arrays out of scalar form types

000e4aa

alexander-schranz reviewed Jan 20, 2019

View reviewed changes

dpfaffenbauer mentioned this pull request Jan 24, 2019

[CoreBundle] fix store-mail configuration type coreshop/CoreShop#789

Merged

xabbuh closed this Jan 25, 2019

xabbuh deleted the issue-29809 branch January 25, 2019 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] weaken the rejection of submitted array data #29911

[Form] weaken the rejection of submitted array data #29911

xabbuh commented Jan 16, 2019 •

edited

xabbuh Jan 16, 2019

YetiCGN Jan 17, 2019 •

edited

asentner Apr 9, 2019

nicolas-grekas commented Jan 17, 2019

OskarStark commented Jan 17, 2019

althaus commented Jan 17, 2019 •

edited

althaus commented Jan 17, 2019

xabbuh commented Jan 17, 2019

alexander-schranz commented Jan 17, 2019 •

edited

xabbuh commented Jan 17, 2019

alexander-schranz Jan 17, 2019 •

edited

alexander-schranz commented Jan 17, 2019

xabbuh commented Jan 17, 2019

alexander-schranz Jan 20, 2019

nicolas-grekas commented Jan 21, 2019

althaus commented Jan 21, 2019

fnagel commented Jan 21, 2019

nicolas-grekas commented Jan 21, 2019

dmaicher commented Jan 21, 2019

xabbuh commented Jan 21, 2019

xabbuh commented Jan 25, 2019

[Form] weaken the rejection of submitted array data #29911

[Form] weaken the rejection of submitted array data #29911

Conversation

xabbuh commented Jan 16, 2019 • edited

xabbuh Jan 16, 2019

Choose a reason for hiding this comment

YetiCGN Jan 17, 2019 • edited

Choose a reason for hiding this comment

asentner Apr 9, 2019

Choose a reason for hiding this comment

nicolas-grekas commented Jan 17, 2019

OskarStark commented Jan 17, 2019

althaus commented Jan 17, 2019 • edited

althaus commented Jan 17, 2019

xabbuh commented Jan 17, 2019

alexander-schranz commented Jan 17, 2019 • edited

xabbuh commented Jan 17, 2019

alexander-schranz Jan 17, 2019 • edited

Choose a reason for hiding this comment

alexander-schranz commented Jan 17, 2019

xabbuh commented Jan 17, 2019

alexander-schranz Jan 20, 2019

Choose a reason for hiding this comment

nicolas-grekas commented Jan 21, 2019

althaus commented Jan 21, 2019

fnagel commented Jan 21, 2019

nicolas-grekas commented Jan 21, 2019

dmaicher commented Jan 21, 2019

xabbuh commented Jan 21, 2019

xabbuh commented Jan 25, 2019

xabbuh commented Jan 16, 2019 •

edited

YetiCGN Jan 17, 2019 •

edited

althaus commented Jan 17, 2019 •

edited

alexander-schranz commented Jan 17, 2019 •

edited

alexander-schranz Jan 17, 2019 •

edited