[Form] use a choice label normalizer instead of violating ChoiceListFactoryInterface::createView #32955
Conversation
…mponent\Form\ChoiceList\Factory\ChoiceListFactoryInterface::createView params
| @@ -312,6 +312,16 @@ public function configureOptions(OptionsResolver $resolver) | |||
| return $choiceTranslationDomain; | |||
| }; | |||
|
|
|||
| $choiceLabelNormalizer = function (Options $options, $choiceLabel) { | |||
| if (false === $choiceLabel) { | |||
| return function () { | |||
There was a problem hiding this comment.
I afraid we can't do this here, because you'll break things for people relying on the false value, e.g. false === $options['choice_label'].
There was a problem hiding this comment.
I don't think that counts otherwise we would not be able to add or change any form option. But I'm fine to merge this PR to 4.4 only.
There was a problem hiding this comment.
we have (since 4.2) the ->setDeprecated() method to deprecate any form option before removing it or changing its value.
Tecnically this would look like a BC break to me.
There was a problem hiding this comment.
I guess deprecating to pass false to the createView method and make the normalization to callable before calling it, I only found one occurence: ChoiceType::createChoiceListView(), but likely must be done for all current implementations of ChoiceListFactoryInterface. Also that should be targeted to 4.4.
There was a problem hiding this comment.
make the normalization to callable before calling it
Then there is no point in adding a deprecation as false will never be passed.
There was a problem hiding this comment.
The point is, it's a public method, hence we can't add the callable type-hint to the $label argument without adding first a deprecation warning about not passing false.
Make the normalization to callable before calling to createView is the intended operation to get rid of the deprecation warning.
There was a problem hiding this comment.
We're talking about this change https://github.com/symfony/symfony/pull/32237/files#diff-ca1123eff9e0aa207aaec667aa16c8e8R48 right?
There was a problem hiding this comment.
It will be nice to have ?callable $label for consistency with other arguments.
I agree with @yceruto — we should deprecate false. I can help with the deprecation if you wish.
There was a problem hiding this comment.
Feel free to take over
|
Same as in #33074 (comment): I'm for updating the type annotation and declare it as callable|false|null, with proper runtime validation that throws a TypeError when appropriate. |
Continuation of #32935 which offers a slightly different implementation of #17798
This way,
'choice_label' => falsedoes not need to be passed to\Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface::createViewwhich violates@param callable|null $label. So we can add a real callable type in #32237