Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] use a choice label normalizer instead of violating ChoiceListFactoryInterface::createView#32955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…mponent\Form\ChoiceList\Factory\ChoiceListFactoryInterface::createView params
| $choiceLabelNormalizer =function (Options$options,$choiceLabel) { | ||
| if (false ===$choiceLabel) { | ||
| returnfunction () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I afraid we can't do this here, because you'll break things for people relying on thefalse value, e.g.false === $options['choice_label'].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you propose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess deprecating to passfalse to thecreateView method and make the normalization tocallable before calling it, I only found one occurence:ChoiceType::createChoiceListView(), but likely must be done for all current implementations ofChoiceListFactoryInterface. Also that should be targeted to 4.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The point is, it's a public method, hence we can't add thecallable type-hint to the$label argument without adding first a deprecation warning about not passingfalse.
Make the normalization tocallable before calling tocreateView is the intended operation to get rid of the deprecation warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're talking about this changehttps://github.com/symfony/symfony/pull/32237/files#diff-ca1123eff9e0aa207aaec667aa16c8e8R48 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It will be nice to have?callable $label for consistency with other arguments.
I agree with@yceruto — we should deprecatefalse. I can help with the deprecation if you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Feel free to take over
nicolas-grekas commentedAug 12, 2019
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