-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[OptionsResolver] Improve the deprecation feature by handling package and version #36345
Conversation
e7ecd23
to
c7470df
Compare
src/Symfony/Component/OptionsResolver/Debug/OptionsResolverIntrospector.php
Show resolved
Hide resolved
9597f05
to
2d4ff54
Compare
fa043de
to
23b5749
Compare
src/Symfony/Component/OptionsResolver/Tests/Debug/OptionsResolverIntrospectorTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/OptionsResolver/Tests/Debug/OptionsResolverIntrospectorTest.php
Outdated
Show resolved
Hide resolved
38d61f7
to
56044bb
Compare
605e8bb
to
3da97ec
Compare
3da97ec
to
c3f5e2c
Compare
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.
(failures are false positives.)
Thank you @atailouloute. |
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). |
I think @stof is making a valid point and making the signature |
The point is, package and version should not be optional, we really want them to be required. |
Keeping |
Uh oh!
There was an error while loading. Please reload this page.