👍

Status: Reviewed

ping @symfony/deciders

👍

Fix works! 👍

This fix contradicts https://github.com/symfony/symfony/pull/16681/files#r45997531
But I think @webmozart was forgetting that false would then conflict with the placeholder optional value.
So changing false to 0 looks better to me indeed.
But then castableToString 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 fix false choices with required => false. But a null 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 both null and '~' as choices (which would conflict with above mapping), then it still uses the incrementing numbers as fallback which is what castableToString ensures.

Thanks @Tobion for the review. You're right, castableToString must be refactored so when using both boolean and null 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 because null 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 select Undecided 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 from null 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 :

<?php
class ChoiceType extends AbstractType
{
    public function buildForm(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 values true, "true", false and "false" I can now use only true and false.

And when I change the order in the choices key in the options with ['true', 'false', true, false] instead, all my false are changed to null...

@Taluu, The submission of a ChoiceType actually requires the string 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 the Form::submit() method. Changing the behavior at this point is a BC break.

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

If there is null or an object among the choices, the default ArrayChoiceList::castableToString() method will return false then you should rely on numeric index to send as string or int 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 in castableToString() is there is boolean, but we loose the ability to match true as '"1"' with is a bigger BC break...

You can rely on choice_value option and always send the good strings values on submission, or use choice_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 a CheckboxType 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 a CheckboxType when we will have fixed the internal ticket (And chosen between string or boolean) will fix the damn thing. Yeay.

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

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

@Taluu, note that the ChoiceType when expanded uses CheckboxType or RadioType (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 use CheckboxType or RadioType to map a real boolean property in a form, consider using a CollectionType.

@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.