[2.7][Form] Fixed ChoiceType with legacy ChoiceList #14551
Conversation
|
Great PR! |
| && null === $label && null === $index && null === $groupBy && null === $attr) { | ||
| return new ChoiceListView($list->getRemainingViews(), $list->getPreferredViews()); | ||
| $mapToNonLegacyChoiceView = function (LegacyChoiceView $choiceView) { | ||
| return new ChoiceView($choiceView->label, $choiceView->value, $choiceView->data); |
There was a problem hiding this comment.
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.
|
Apart from my comment, I don't have any objections :) 👍 |
|
Keeping the legacy @webmozart If you are in favor of changing the typehint to enable the usage of the legacy @@ -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',
); |
|
👍 Will this PR fix #14382 without any code change on bundles / core project? Thanks. |
|
@soullivaneuh Yes, this fixes the code which keeps BC. |
|
Thank you @xelaris. |
This PR was merged into the 2.7 branch. Discussion ---------- [2.7][Form] Fixed ChoiceType with legacy ChoiceList | 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: ```php $formBuilder->add('choices', 'choice', array( 'choice_list' => new SimpleChoiceList(array( 'creditcard' => 'Credit card payment', 'cash' => 'Cash payment' )) )); ``` Commits ------- a98e484 [Form] Fix ChoiceType with legacy ChoiceList
|
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');
)); |
The "Backwards compatibility" condition doesn't grap (e.g. when passing a
SimpleChoiceListaschoice_listonChoiceType), as the default value for theChoiceTypeoptionpreferred_choicesisarray()instead ofnull. So I changed the condition fromnull === $preferredChoicestoempty($preferredChoices).Then there was an issue with accessing
attrinform_div_layout.html.twig, since the deprecatedSymfony\Component\Form\Extension\Core\View\ChoiceViewdoesn't provide anattrattribute. Since the docblocks ofSymfony\Component\Form\ChoiceList\View\ChoiceListViewstate$choicesand$preferredChoicesto be instances ofSymfony\Component\Form\ChoiceList\View\ChoiceViewinstead ofSymfony\Component\Form\Extension\Core\View\ChoiceViewI fixed the template issue by mapping the deprecatedChoiceViewobjects to the new one with an emptyattr.@webmozart Could you have a look at it, please?
Without this PR the following example would render numeric values as labels: