Skip to content

[OptionsResolver] Add $triggerDeprecation arg to offsetGet() method in Options interface #31799

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[OptionsResolver] Add $triggerDeprecation arg to offsetGet() method in Options interface #31799

wants to merge 1 commit into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jun 2, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Ref: #28878

@yceruto
Copy link
Member Author

yceruto commented Jun 2, 2019 •
edited
Loading

We lack a deprecation notice in 4.4 :/ this is a special case.

I think DebugClassLoader could do the job and trigger the auto-deprecation notice when the arguments defined in the @method annotation don't match the arguments of the real method, WDYT?

@yceruto
Copy link
Member Author

yceruto commented Jun 2, 2019 •
edited
Loading

Or at least trigger only if the number of arguments in the real method is less than the ones defined in @method annotation

@yceruto yceruto force-pushed the options_offsetGet_signature branch from dc55eea to b8cd9ba Compare June 3, 2019 02:56
@yceruto yceruto force-pushed the options_offsetGet_signature branch from b8cd9ba to d983fd8 Compare June 3, 2019 11:23
@nicolas-grekas
Copy link
Member

I agree DebugClassLoader should be able to trigger a deprecation notice here. Would you be able to have a look?

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 3, 2019
@nicolas-grekas
Copy link
Member

(don't forget the UPGRADE files :) )

@yceruto
Copy link
Member Author

yceruto commented Jun 3, 2019

I agree DebugClassLoader should be able to trigger a deprecation notice here. Would you be able to have a look?

Yes, I think so :)
Next, I need a way to get rid of that deprecation, thoughts?

@nicolas-grekas
Copy link
Member

Adding @param bool $triggerDeprecation works already for getting rid of the deprecation I think. - It means "I know I should add this argument but I can't for BC"

@nicolas-grekas
Copy link
Member

Actually I'm proposing to remove the argument from the interface in #31950
I fail to understand why it should be on the interface.

nicolas-grekas
nicolas-grekas reviewed Jun 8, 2019
View reviewed changes
src/Symfony/Component/OptionsResolver/Options.php
* @throws NoSuchOptionException If the option is not set
* @throws InvalidOptionsException If the option doesn't fulfill the
* specified validation rules
* @throws OptionDefinitionException If there is a cyclic dependency between
Copy link
Member

Choose a reason for hiding this comment

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

all this is not a minor change: was it implicitly the definition of the contracts of the interface before?
also, does it make sense to have all this at the abstraction level? or are they implementation details of OptionsResolver, the implementation?

@yceruto yceruto closed this Jun 8, 2019
@yceruto yceruto deleted the options_offsetGet_signature branch June 8, 2019 12:41
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
OptionsResolver Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
3 participants
@yceruto @nicolas-grekas @carsonbot