-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Deprecate passing false as $label to ChoiceListFactoryInterface #33074
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php
Show resolved
Hide resolved
9b7ca2b
to
90aaa6c
Compare
Passing false looks fine to me. I'm not sure we should do this for the sake of adding type hints. Every deprecation adds to the migration process, we already have a lot of them. I'd prefer updating the docblock and mention that false is legal (and explain the purpose of it of course.) |
Okay, I agree, this went too far :) |
I don't think anyone will be affected by this. Poeple use the option and are unlikely to have changed the implementation. Otherwise I would also say it's not worth it. But currently there are two ways to do the same thing: passing false and passing a callable that returns false. And passing false directly violates the interface. So either we need to break BC by changing the interface or we need to do this here. So to me, this PR is the better solution. |
Then let's finish this! @yceruto , I've just committed what you proposed. Did I get you right? |
src/Symfony/Component/Form/ChoiceList/Factory/CachingFactoryDecorator.php
Outdated
Show resolved
Hide resolved
@yceruto , thanx, done. Also added tests to make sure the deprecation is triggered by default. |
3b43b43
to
79a3ee2
Compare
This looks clumsy. I'm for updating the type annotation and declare it as |
Yes, I agree it could be the simpler variant. |
…teView $label argument (vudaltsov) This PR was merged into the 3.4 branch. Discussion ---------- [Form] Add bool type to ChoiceListFactoryInterface::createView $label argument | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Replaces #33074 Ping @nicolas-grekas , @Tobion , @yceruto . Commits ------- 8f5d1ca Add false type to ChoiceListFactoryInterface::createView $label argument
Replaces #32955 , see #32955 (comment)