[Form] Remove deprecated a callable empty_data in ChoiceType #25925
Conversation
peter-gribanov
commented
Jan 25, 2018
| Q | A |
|---|---|
| Branch? | 5.0 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #25896 |
| License | MIT |
Update master
| @@ -91,7 +91,11 @@ public function buildForm(FormBuilderInterface $builder, array $options) | ||
| $emptyData = $form->getConfig()->getEmptyData(); | ||
|
|
||
| if (false === FormUtil::isEmpty($emptyData) && array() !== $emptyData) { | ||
| $data = is_callable($emptyData) ? call_user_func($emptyData, $form, $data) : $emptyData; | ||
| if (is_callable($emptyData) && !is_string($emptyData)) { |
There was a problem hiding this comment.
You marked the behavior as deprecated, but doesn't this check completely remove the feature? I think you should still support this case and throw a deprecation instead, otherwise people can't upgrade.
This also means your branch can target 4.1 instead of 5.0. To remove it in 5.0, a new patch is required when 4.3 is released in 1.5 year (but not your responsibility).
There was a problem hiding this comment.
This PR for 5.0. For 4.1 see #25924
There was a problem hiding this comment.
I'm not sure how useful it is to make this PR right now, because that means we'll have an open PR for about 1.5 year and then someone else might make a second PR to do the same. If every BC break got a PR open, the amount of open PRs will be tremendous, hence it's custom to open them after 4.3.
There was a problem hiding this comment.
This #25896 bug, too, will not be resolved for 1.5 years and so what?
I'm a author of this deprecated code and nobody except me knows how to cut it better.
After 1.5 years, everyone will forget what and how. And so i propose to solve a specific problem now.
There was a problem hiding this comment.
What prevents to create Mileston 5.0 and forget about this PR before release of 4.3 and creation of branch 5.0?
There was a problem hiding this comment.
will not be resolved for 1.5 years and so what?
It will create a lot of open PRs, which makes it really hard to distinguish between PRs that are just waiting and PRs that could be merged (in theory).
I'm a author of this deprecated code and nobody except me knows how to cut it better.
After 1.5 years, everyone will forget what and how.
The deprecation should make clear what the intended behavior is and just the deprecated branch should be removed. This is how has been all along. In case people working on the removal can't figure it out, they can always ping the original author of the deprecation.
What prevents to create Mileston 5.0 and forget about this PR before release of 4.3 and creation of branch 5.0?
Nothing really, but keeping a PR for that long, is bound to cause issues. As I've mentioned before, it would cause a massive amount of waiting PRs, clogging the PR list. Next to that, it will cause a ton of merge conflicts in the upgrade files, meaning something has to be done eventually anyway. Sometimes, a deprecation gets un-deprecated, that means that every time this happens, the person doing so, has to make sure that the other PR is closed as well. This means extra overhead for the core developers and PRs roaming around that might not go anywhere, ever.
|
Closing in favor of #25948 |