[Form] ChoiceType: Fix cast to string from null choice

[yceruto]

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	symfony/symfony-docs#6393

Given the following:

$builder->add('attending', 'choice', [
        'choices' => [
            'Yes' => true,
            'No' => false,
            'Maybe' => null,
        ]
    ]);;

Before:

<select id="form_attending" name="form[attending]">
    <option value="0">Yes</option>
    <option value="1">No</option>
    <option value="2" selected="selected">Maybe</option>
</select>

After:

<select id="form_attending" name="form[attending]">
    <option value="1">Yes</option>
    <option value="0">No</option>
    <option value="" selected="selected">Maybe</option>
</select>

This results more consistent with expected behavior.

[HeahDude]

Hey @yceruto thanks for opening this PR but as discussed in #17760 with @Tobion it would then collide with the placeholder value which is always an empty string.

It also means we're missing a test for this regression.

[yceruto]

it would then collide with the placeholder value which is always an empty string.

If I understood correctly (#17760 (comment)) then this PR don't affects the current behavior, because currently this already happen:

#17760 (comment) ... having both a placeholder and a choice with an explicit null or empty string '' value, there is a conflict... The default null choice is preselected over the placeholder added in the choice list.

[HeahDude]

when having both a placeholder and a choice with an explicit null or empty string '' value, there is a conflict... The default null choice is preselected over the placeholder added in the choice list.

Well it shouldn't https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L207.

[yceruto]

Well it shouldn't

Agreed, but IMHO this PR is not the source of bad behavior, this already happen before that:

Before this PR:

Result (should show two options and placeholder selected):

$builder->add('foo', 'choice', array(
    'choices' => array('None' => null),
    'placeholder' => 'Please choose',
))

<select id="form_foo" name="form[foo]" required="required">
    <option value="">Please choose</option>
    <option value="0" selected="selected">None</option> // wrong selected
</select>

Result (should show three options and placeholder selected):

$builder->add('foo', 'choice', array(
    'choices' => array('Red' => 'red', 'None' => ''),
    'placeholder' => 'Please choose',
))

<select id="form_foo" name="form[foo]" required="required">
    <option value="" selected="selected">Please choose</option>
    <option value="red">Red</option>
    <option value="" selected="selected">None</option> //  wrong selected
</select>

After this PR:

Result (should show two options and placeholder selected):

$builder->add('foo', 'choice', array(
    'choices' => array('None' => null), // or array('None' => ''),
    'placeholder' => 'Please choose',
))

<select id="form_foo" name="form[foo]" required="required">
    <option value selected="selected">None</option>
</select>

Result (should show three options and placeholder selected):

$builder->add('foo', 'choice', array(
    'choices' => array('Red' => 'red', 'None' => null), // or 'None' => ''
    'placeholder' => 'Please choose',
))

<select id="form_foo" name="form[foo]" required="required">
    <option value="" selected="selected">Please choose</option>
    <option value="red">Red</option>
    <option value="" selected="selected">None</option> //  wrong selected
</select>

The result in all cases (before/after) is wrong in any way.

This PR resolves an inconsistency issue, but do you think that makes the problem worse?

[yceruto]

Maybe this result after:

<select id="form_foo" name="form[foo]" required="required">
     // placeholder option ignored because there is a choice with "" or null.
    <option value selected="selected">None</option>
</select>

should be the correct behavior and placeholder should be ignored when there is a choice with '' or null, because in both case the result on submit should be the same: null.

EDIT: Just as placeholder is ignored when multiple is true.

[yceruto]

Otherwise, we need achieve render two options with the same empty value:

<select id="form_foo" name="form[foo]">
    <option value selected="selected">Please choose</option>
    <option value>None</option>
</select>

Is there the expected behavior?

[HeahDude]

IMHO both results before your PR are expected:

---

$builder->add('foo', 'choice', array(
    'choices' => array('None' => null),
    'placeholder' => 'Please choose',
))

<select id="form_foo" name="form[foo]" required="required">
    <option value="">Please choose</option>
    <option value="0" selected="selected">None</option> // wrong selected
</select>

I don't think this is wrongly selected if the data passed on form creation is null, then the choice with a model data null and value 0 is selected.

---

$builder->add('foo', 'choice', array(
    'choices' => array('None' => ''),
    'placeholder' => 'Please choose',
))

<select id="form_foo" name="form[foo]" required="required">
    <option value selected="selected">None</option> // missing placeholder and wrong selected
</select>

The pre selection here is tricky but expected and the missing placeholder is expected too https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php#L55.

---

The standard is about having the placeholder first because for a select input, the first value is always selected. By default, the empty string in this context means "I haven't choose anything yet", and it makes sense in some cases to bind it to an empty string or null in the model.

The thing is that it does not make sense to use an empty string as model choice (which may not be the first) and use the placeholder option at the same time, because even in this edge case the choice_value option can solve it..

If we want more control on it, we should consider it a feature and maybe add a placeholder_value option to define it, but I don't think it's worth it, let's improve the docs instead.

[yceruto]

👍 Fair enough then :)