👍

Status: Reviewed

ping @symfony/deciders

👍

Fix works! 👍

This fix contradictshttps://github.com/symfony/symfony/pull/16681/files#r45997531
But I think@webmozart was forgetting thatfalse would then conflict with the placeholder optional value.
So changingfalse to0 looks better to me indeed.
But thencastableToString must also be updated to reflect that change.

Please add a test for that (which would currently fail AFAIK):

'choices' => [        'Yes'  => true,        'No'   => false,        'Undecided'   => null,    ],

null and false should not conflict and thus can both be used as'' and'0' value (instead of fallback solution to use incrementing numbers).

Btw, this would fixfalse choices withrequired => false. But anull choice with required false, would still have the same problem?! IMO a placeholder value should always be different than any given choice.

So basically we need to map all scalars that would result in an empty string to something else to not conflict we the placeholder:

I would propose this mapping:

• false ->0
• null ->~
• '' ->- Seems like this is supposed to be possible. It will be treated as the placeholder itself which then prevents the placeholder to be added:https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L205

And if there are still conflicts e.g. because someone has bothnull and'~' as choices (which would conflict with above mapping), then it still uses the incrementing numbers as fallback which is whatcastableToString ensures.

Thanks@Tobion for the review. You're right,castableToString must be refactored so when using bothboolean andnull values it returns true for the test you mention pass.

I'm fixing it.

Status: Needs Work

Tests are fixed.

Status: Needs Review

The problem now is

'choices' => [    'Undecided'   => null,    'Yes'  => true,    'No'   => false,],'placeholder' => 'Please choose',

will not add the placeholder anymore becausenull is mapped to'' value and thus the placeholder is ignored because the choice assumes the null is already the placeholder value. And even worse is that people cannot selectUndecided in this required select anymore because the browser will treat it as the placeholder as well (cuz it has an empty value). Before it used incrementing integers and thus was not affected by this.
That is why I suggested to change the mapping fromnull to something like~ as value to not be treated as placeholder.

@Tobion, thanks again for the heads-up, so I keep on testing.

Status: Needs Work

This seems to break API ways on Symfony 2.8.3. When before I could do the following :

<?phpclass ChoiceTypeextends AbstractType{publicfunctionbuildForm(FormBuilderInterface$builder,array$options)    {$builder->add('status', ChoiceType::class, ['choices'  => [true,false,'true','false'],'choices_as_values' =>true        ]);    }}

With the following json sent data :

{"choices":[          {"hash":"foo4u","label":"foo","status":true          },          {"hash":"bar4u","label":"bar","status":"false"          },          {"hash":"baz4u","label":"baz","status":false          }        ]}

whilre before I could use the valuestrue,"true",false and"false" I can now use onlytrue andfalse.

And when I change the order in thechoices key in the options with['true', 'false', true, false] instead, all myfalse are changed tonull...

@Taluu, The submission of aChoiceType actually requires thestring value to get properly mapped to the model choice data.

If you submit a true value it will be cast to string so"1" not by the choice list constructor like in this fix but directly in theForm::submit() method. Changing the behavior at this point is a BC break.

Sofalse get casted to an empty string which cannot map thefalse model choice data expecting the string"0" as set in the choice list constructor (by this PR).

If there isnull or an object among the choices, the defaultArrayChoiceList::castableToString() method will return false then you should rely on numeric index to send asstring orint in a json hash.

false value could get in conflict with placeholder in some cases (solves by this PR).

Another way to resolve this is to return false incastableToString() is there is boolean, but we loose the ability to matchtrue as '"1"' with is a bigger BC break...

You can rely onchoice_value option and always send the good strings values on submission, or usechoice_label option and a choices array with a numeric index and submit the right index.

For legacy reason we have these"true" and"false" values. But we have an internal ticket to use real booleans instead ("one day my prince will come"), and then using aCheckboxType would be the way to go.

Until then, we will have to stick with"true" and"false". Using either['true', 'false'] or[true, false] works though...

Edge case then. Happy coding :)

Yep, I confirm, this is indeed due to this. Don't know if it was there or not before, but using aCheckboxType when we will have fixed the internal ticket (And chosen between string or boolean) will fix the damn thing. Yeay.

Symfony\Component\Validator\ConstraintViolationObject(Symfony\Component\Form\Form).children[choices].children[0].children[status] = 1Caused by:Symfony\Component\Form\Exception\TransformationFailedExceptionUnable to reverse value for property path "[status]": The choice "1" does not exist or is not uniqueCaused by:Symfony\Component\Form\Exception\TransformationFailedExceptionThe choice "1" does not exist or is not unique

And using directly theCheckboxType is not possible (Even though it passes with flying colors - a"false" is transformed intofalse and"true" intotrue), as we have a need to have"true" and"false" values. So yeah nothing to report.

@Taluu, note that theChoiceType when expanded usesCheckboxType orRadioType (which extends the first) to represent as a virtual boolean if the actual string value is selected or not as a choice among other values and to be mapped back to a model value.

That's why it messes with PATCH method and$clearMissing (ref :#17771 (comment)).

When you need to useCheckboxType orRadioType to map a real boolean property in a form, consider using aCollectionType.

@Taluu You could preprocess your request data and postprocess your responses to transform Booleans to strings and back. That would at least fix your API.