Skip to content
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] Add $triggerDeprecation arg to offsetGet() method in Options interface #31799

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@yceruto
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

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 yceruto:options_offsetGet_signature branch from dc55eea to b8cd9ba Jun 3, 2019

@yceruto yceruto force-pushed the yceruto:options_offsetGet_signature branch from b8cd9ba to d983fd8 Jun 3, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

(don't forget the UPGRADE files :) )

@yceruto

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

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

* @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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 8, 2019

Member

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 yceruto:options_offsetGet_signature branch Jun 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.