NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

yceruto wants to merge1 commit intosymfony:masterfromyceruto:options_offsetGet_signature

Closed

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

yceruto wants to merge1 commit intosymfony:masterfromyceruto:options_offsetGet_signature

Conversation

Copy link

Member

yceruto commentedJun 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

carsonbot added Status: Needs Review OptionsResolver labels

Jun 2, 2019

Copy link

MemberAuthor

yceruto commentedJun 2, 2019•
edited
Loading

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

I thinkDebugClassLoader 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?

Copy link

MemberAuthor

yceruto commentedJun 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 force-pushed theoptions_offsetGet_signature branch fromdc55eea tob8cd9baCompare

June 3, 2019 02:56

Add $triggerDeprecation arg to offsetGet() method in Options interface

d983fd8

yceruto force-pushed theoptions_offsetGet_signature branch fromb8cd9ba tod983fd8Compare

June 3, 2019 11:23

Copy link

Member

nicolas-grekas commentedJun 3, 2019

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

nicolas-grekas added this to thenext milestone

Jun 3, 2019

Copy link

Member

nicolas-grekas commentedJun 3, 2019

(don't forget the UPGRADE files :) )

Copy link

MemberAuthor

yceruto commentedJun 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?

Copy link

Member

nicolas-grekas commentedJun 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"

Copy link

Member

nicolas-grekas commentedJun 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.

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?