[2.7][Form] Fixed ChoiceType with legacy ChoiceList

[xelaris]

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

The "Backwards compatibility" condition doesn't grap (e.g. when passing a SimpleChoiceList as choice_list on ChoiceType), as the default value for the ChoiceType option preferred_choices is array() instead of null. So I changed the condition from null === $preferredChoices to empty($preferredChoices).
Then there was an issue with accessing attr in form_div_layout.html.twig, since the deprecated Symfony\Component\Form\Extension\Core\View\ChoiceView doesn't provide an attr attribute. Since the docblocks of Symfony\Component\Form\ChoiceList\View\ChoiceListView state $choices and $preferredChoices to be instances of Symfony\Component\Form\ChoiceList\View\ChoiceView instead of Symfony\Component\Form\Extension\Core\View\ChoiceView I fixed the template issue by mapping the deprecated ChoiceView objects to the new one with an empty attr.

@webmozart Could you have a look at it, please?

Without this PR the following example would render numeric values as labels:

        $formBuilder->add('choices', 'choice', array(
            'choice_list' => new SimpleChoiceList(array(
                'creditcard' => 'Credit card payment',
                'cash' => 'Cash payment'
            ))
        ));

[webmozart]

Great PR!

[webmozart]

I think a more efficient solution would be to simply define the "attr" property dynamically:

$choiceView->attr = array();

After all, this method is called pretty often.

[webmozart]

Apart from my comment, I don't have any objections :) 👍

[xelaris]

Keeping the legacy ChoiceView objects (and adding the attr attribute dynamically) to introduce as less overhead as possible sounds good, but changing it causes testLegacySubmitSingleExpandedObjectChoices tests to fail, since ChoiceType::addSubForm() expects $coiceView to be instance of the new ChoiceView. As addSubForm is a private method I think it is save to typehint with the deprecated ChoiceView and accessing the attr only if it is an instance of the new one, isn't it?
Apart from the efficiency, I think it would be more clean and safe to provide ChoiceListViews with instances of the new ChoiceView only (as stated in ChoiceListView php doc) . But I don't know what matters more here.

@webmozart If you are in favor of changing the typehint to enable the usage of the legacy ChoiceViews and save the mapping, I would to this change:

@@ -408,12 +409,12 @@ class ChoiceType extends AbstractType
      *
      * @return mixed
      */
-    private function addSubForm(FormBuilderInterface $builder, $name, ChoiceView $choiceView, array $options)
+    private function addSubForm(FormBuilderInterface $builder, $name, LegacyChoiceView $choiceView, array $options)
     {
         $choiceOpts = array(
             'value' => $choiceView->value,
             'label' => $choiceView->label,
-            'attr' => $choiceView->attr,
+            'attr' => $choiceView instanceof ChoiceView ? $choiceView->attr : array(),
             'translation_domain' => $options['translation_domain'],
             'block_name' => 'entry',
         );

[soullivaneuh]

👍

Will this PR fix #14382 without any code change on bundles / core project?

Thanks.

[xelaris]

@soullivaneuh Yes, this fixes the code which keeps BC.

[fabpot]

Thank you @xelaris.

[trsteel88]

This is still causing issues when I use an ObjectChoiceList and it is grouped by something.

e.g:

$resolver->setDefaults(array(
    'choice_list' =>  new ObjectChoiceList($someEntities, 'name', array(), 'some_group', 'id');
));