Skip to content

[OptionsResolver] Improve the deprecation feature by handling package and version #36345

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

Merged

Conversation

atailouloute
Copy link
Contributor

@atailouloute atailouloute commented Apr 4, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? yes
Tickets
License MIT
Doc PR TODO

@atailouloute atailouloute changed the title [OptionsResolver] Improve the deprecation feature by handling package… [OptionsResolver] Improve the deprecation feature by handling package and version Apr 4, 2020
@atailouloute atailouloute force-pushed the improve-options-resolver-deprecation branch from e7ecd23 to c7470df Compare April 4, 2020 19:48
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 4, 2020
@atailouloute atailouloute force-pushed the improve-options-resolver-deprecation branch 3 times, most recently from 9597f05 to 2d4ff54 Compare April 5, 2020 10:14
@atailouloute atailouloute requested a review from xabbuh as a code owner April 5, 2020 10:14
@atailouloute atailouloute force-pushed the improve-options-resolver-deprecation branch 2 times, most recently from fa043de to 23b5749 Compare April 5, 2020 10:34
@atailouloute atailouloute force-pushed the improve-options-resolver-deprecation branch 4 times, most recently from 38d61f7 to 56044bb Compare April 6, 2020 12:15
@atailouloute atailouloute force-pushed the improve-options-resolver-deprecation branch 2 times, most recently from 605e8bb to 3da97ec Compare April 6, 2020 17:40
@atailouloute atailouloute force-pushed the improve-options-resolver-deprecation branch from 3da97ec to c3f5e2c Compare April 6, 2020 17:49
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(failures are false positives.)

@fabpot
Copy link
Member

fabpot commented Apr 8, 2020

Thank you @atailouloute.

@fabpot fabpot merged commit c6a176d into symfony:master Apr 8, 2020
@atailouloute atailouloute deleted the improve-options-resolver-deprecation branch April 8, 2020 07:52
@stof
Copy link
Member

stof commented Apr 8, 2020

Adding the package and version before the message makes it much harder for the ecosystem to support both options-resolver 5.1+ and options-resolver 5.0-, as they would need to detect which signature to use when configuring their options (and I'm not even sure there is an easy and performant way to do that detection, which is important as the OptionsResolver is in the hot path of complex forms IIRC).

@wouterj
Copy link
Member

wouterj commented Apr 11, 2020

I think @stof is making a valid point and making the signature setDeprecated($message, $package = '', $version = '') makes it much easier to support both versions.

@nicolas-grekas
Copy link
Member

The point is, package and version should not be optional, we really want them to be required.

@xabbuh
Copy link
Member

xabbuh commented Apr 12, 2020

Keeping $message as the first argument would still help though as PHP would ignore additional arguments for OptionsResolver < 5.1 (we could allow it to be null to opt-in for reusing the default message template).

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants