-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] Remove deprecated a callable empty_data in ChoiceType #25925
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
Closed
peter-gribanov
wants to merge
6
commits into
symfony:master
from
peter-gribanov:empty_data_callable_deprecated_remove
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
96e194c
Merge pull request #1 from symfony/master
peter-gribanov b8ac58c
mark usage of callable empty_data not instance of \Closure is deprecated
peter-gribanov f8cc144
allow use array as handler
peter-gribanov 62d2c14
add info in UPGRADE and CHANGELOG. Add callable empty_data tests
peter-gribanov da3c6ac
fix CS
peter-gribanov eb95111
remove deprecated feature
peter-gribanov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR for 5.0. For 4.1 see #25924
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.
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.