[Form] Fix ChoiceType to ensure submitted data is not nested unnecessarily #21267

issei-m · 2017-01-13T06:40:02Z

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

Fixed ChoiceType to protect against some problem caused by treating of array.

Let's say we have the choice-form like:

$form = $factory->create(ChoiceType, null, [
    'choices' => [
        'A',
        'B',
        'C',
    ],
    'expanded' => true,
    'multiple' => true,
]);

Then, submit data like this:

$form->submit([
    [], // unnecessality nested
]);

(Yes, I agree in most cases these situation doesn't happen, but can be)

Then, we get array_flip(): Can only flip STRING and INTEGER values! error at here.
Even if form is not multiple, annoying Array to string conversion error occurs in here (via ChoicesToValuesTransformer).
(As far as I know, non-multiple and non-expanded form has no problem, thanks to ChoiceToValueTransformer)

To resolve these problems, I just added a simple-validation listener to choice type.

…arily

--- By applying http://fabbot.io/patch/symfony/symfony/21267/466bebe26e8697d9c32cea0044b25f580c04a3bf/cs.diff

HeahDude

Hi @issei-m, sorry I took some time to answer here, although your concern is legit this should never happen in an html context but using forms with json API might break indeed.

Since 2.x is still, maintained for years, maybe we should merge your patch, but please note that I'm working on solving this issue more globally in master and will propose a PR soon, to avoid patching in form types forever and stumbling upon weird side effects.

I'm a bit afraid that if we accept this PR, we'll end up with PRE_SUBMIT validation listeners everywhere, as that would be bad for performances too.

I've left some minor comments but then I would be +0.

What do you think symfony deciders?

HeahDude · 2017-01-19T09:50:46Z

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

@@ -65,6 +65,22 @@ public function __construct(ChoiceListFactoryInterface $choiceListFactory = null
     */
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
+        // To avoid some problem against treating of array (e.g. Array to string conversion),


Could you please move this new block at the end of the method?

HeahDude · 2017-01-19T09:53:23Z

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

+            }
+
+            foreach ($data as $v) {
+                if (is_array($v)) {


Should be:

if (null !== $v && !is_string($v)) { throw new TransformationFailedException('All choices submitted must be NULL or strings.'); }

issei-m · 2017-01-19T11:28:19Z

@HeahDude Just I've fixed point of mentioned by you.

xabbuh · 2017-02-08T19:45:01Z

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

+            }
+
+            foreach ($data as $v) {
+                if (null !== $v && !is_string($v)) {


Instead of adding a new event listener we could do this in the already existing one before flipping the data, can't we?

The listener you mentioned is only added when choice is multiple. Even though we would do so we still need to add a new listener for this.

Thanks, I missed that.

xabbuh · 2017-02-09T10:11:26Z

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

@@ -160,6 +160,22 @@ public function buildForm(FormBuilderInterface $builder, array $options)
            // transformation is merged back into the original collection
            $builder->addEventSubscriber(new MergeCollectionListener(true, true));
        }
+
+        // To avoid some problem against treating of array (e.g. Array to string conversion),
+        // we have to first ensure the all elements of submitted data ain't an array.


"To avoid issues when the submitted choices are arrays (i.e. array to string conversions), we have to ensure that all elements of the submitted choice data are strings or null."

xabbuh · 2017-02-09T15:49:20Z

One last thing that I missed during my previous review: Please move the data provider after the test method.

issei-m · 2017-02-10T03:47:54Z

@xabbuh It's done. Thanks for your advice!

xabbuh · 2017-02-10T08:38:48Z

👍

Status: Reviewed

fabpot · 2017-03-01T14:58:35Z

Thank you @issei-m.

…ed unnecessarily (issei-m) This PR was squashed before being merged into the 2.7 branch (closes #21267). Discussion ---------- [Form] Fix ChoiceType to ensure submitted data is not nested unnecessarily | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Fixed ChoiceType to protect against some problem caused by treating of array. Let's say we have the choice-form like: ```php $form = $factory->create(ChoiceType, null, [ 'choices' => [ 'A', 'B', 'C', ], 'expanded' => true, 'multiple' => true, ]); ``` Then, submit data like this: ```php $form->submit([ [], // unnecessality nested ]); ``` (Yes, I agree in most cases these situation doesn't happen, but can be) Then, we get `array_flip(): Can only flip STRING and INTEGER values!` error at [here](https://github.com/symfony/symfony/blob/6babdb3296a5fbbd9c69d1e3410adbdf749959ef/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L114). Even if form is not `multiple`, annoying `Array to string conversion` error occurs in [here](https://github.com/symfony/symfony/blob/6babdb3296a5fbbd9c69d1e3410adbdf749959ef/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php#L144) (via [ChoicesToValuesTransformer](https://github.com/symfony/symfony/blob/5129c4cf7e294b1a5ea30d6fec6e89b75396dcd2/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoicesToValuesTransformer.php#L74)). (As far as I know, non-multiple and non-expanded form has no problem, thanks to [ChoiceToValueTransformer](https://github.com/symfony/symfony/blob/6babdb3296a5fbbd9c69d1e3410adbdf749959ef/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToValueTransformer.php#L43)) To resolve these problems, I just added a simple-validation listener to choice type. Commits ------- 64d7a82 [Form] Fix ChoiceType to ensure submitted data is not nested unnecessarily

This PR was squashed before being merged into the 2.7 branch (closes #21957). Discussion ---------- [Form] Choice type int values (BC Fix) | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21952 | License | MIT #21267 unnecessarily forced all choice values to be strings, this was a BC braking change. Everything can work fine if they are strings or ints. specifically the issue was because `array_flip` is used on the `choices` array, but this function will work fine with ints as well as strings. Normally, when using HTML forms all data comes as strings, but the `symfony/form` component has use cases beyond this, specifically, I use it with JSON data, where valid values for a `ChoiceType` might not be strings. Commits ------- ed211e9 [Form] Choice type int values (BC Fix)

- symfony 3.2.5/6で、choice typeでint値を受け付けなくなっていたため(symfony/symfony#21267) - 21267はbc break扱いで再度修正が入っている(symfony/symfony#21957) - symfony3.2.7で解消されると思われるが、テスト通したいので直しておく

…y/symfony#21267) - 21267はbc break扱いで再度修正が入っている(symfony/symfony#21957) - symfony3.2.7で解消されると思われるが、テスト通したいので直しておく

pl-github · 2017-05-05T10:23:48Z

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

+            }
+
+            foreach ($data as $v) {
+                if (null !== $v && !is_string($v)) {


The is_string() check may be a little bit to restrictive in a loosely typed language like PHP.
In our code we have an event listener that adds IDs to the submitted form data. But the IDs are integers. So in Symfony 2.8.17 our processing works fine, but in 2.8.18 our processing fails. So for me the fixed code introduces a BC break. What is your opinion about that?

see #21957 which is part of the 2.8.19 release

Thanks :)
But 2.8.19 has some changes in DoctrineBridge that breaks our code (so we switched back to 2.18.18) :(

[Form] Simplify FQCN in ChoiceTypeTest

cd7d2a4

carsonbot added Status: Needs Review Form Bug labels Jan 13, 2017

issei-m added 3 commits January 13, 2017 15:59

[Form] Add failure test case to ChoiceTypeTest for invalid nested value

fb33ad2

[Form] Fix ChoiceType to ensure submitted data is not nested unnecess…

98195dd

…arily

Fix CS

9d406db

--- By applying http://fabbot.io/patch/symfony/symfony/21267/466bebe26e8697d9c32cea0044b25f580c04a3bf/cs.diff

issei-m force-pushed the fix-choice-bug branch from 81092ad to 9d406db Compare January 13, 2017 07:00

nicolas-grekas added this to the 2.7 milestone Jan 16, 2017

HeahDude suggested changes Jan 19, 2017

View reviewed changes

[Form] Fix validation-listener logic and move block to the end-of-method

61c80c2

xabbuh reviewed Feb 8, 2017

View reviewed changes

xabbuh reviewed Feb 9, 2017

View reviewed changes

Update comment

eb4d85a

Move test data provider after the test method

d6d2db5

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 10, 2017

fabpot closed this Mar 1, 2017

issei-m deleted the fix-choice-bug branch March 1, 2017 23:43

This was referenced Mar 6, 2017

Release v2.7.25 #21884

Merged

Release v2.8.18 #21885

Merged

Release v3.2.5 #21939

Merged

superhaggis mentioned this pull request Mar 9, 2017

[Form]/[Doctrine][Bridge] EntityType broken for "multiple" under Symfony 3.2.5 #21947

Closed

mcfedr mentioned this pull request Mar 9, 2017

ChoiceType with int values broken #21952

Closed

mcfedr mentioned this pull request Mar 10, 2017

[Form] Choice type int values (BC Fix) #21957

Closed

pl-github reviewed May 5, 2017

View reviewed changes

Mopster mentioned this pull request Sep 6, 2017

ChoiceType multiple = true, expanded = false throws a TransformationFailedException [2.8] #24107

Closed

Uh oh!

[Form] Fix ChoiceType to ensure submitted data is not nested unnecessarily #21267

[Form] Fix ChoiceType to ensure submitted data is not nested unnecessarily #21267

Uh oh!

Conversation

issei-m commented Jan 13, 2017

Uh oh!

HeahDude left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

issei-m commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

issei-m Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Feb 9, 2017

Uh oh!

issei-m commented Feb 10, 2017

Uh oh!

xabbuh commented Feb 10, 2017

Uh oh!

fabpot commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HeahDude left a comment •

edited

Loading

issei-m commented Jan 19, 2017 •

edited

Loading

issei-m Feb 9, 2017 •

edited

Loading