Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Always consider abstract getters as autowiring candidates#21602
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
34bf1bb toe667e47Comparenicolas-grekas commentedFeb 14, 2017
now with tests |
| try { | ||
| $this->processValue($argumentDefinition,true); | ||
| $this->processValue($argumentDefinition,false); |
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.
just omit the argument entirely
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.
indeed, done
e667e47 to2b55ed8Comparestof commentedFeb 14, 2017
👍 |
| * @author Kévin Dunglas <dunglas@gmail.com> | ||
| */ | ||
| class GetterOverriding | ||
| abstractclass GetterOverriding |
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.
Maybe should you add another test instead of editing this one to be sure that both cases are covered?
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.
test case splitted
| } | ||
| } | ||
| if ($reflectionMethod->isAbstract() && !$reflectionMethod->getNumberOfParameters()) { |
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.
Shouldn't we check all conditions making it eligible to autowiring (return type...).
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.
that won't provide anything more to me since those checks are going to happen anyway later on
dunglas commentedFeb 14, 2017
👍 |
c984198 to61ea411Compare61ea411 to8f246bdComparefabpot commentedFeb 27, 2017
Thank you@nicolas-grekas. |
…ates (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Always consider abstract getters as autowiring candidates| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes (a missing part of getter autowiring really)| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -When a definition is set to be autowired with no method explicitly configured, we already wire the constructor.We should also autowire abstract getters - with the same reasoning that makes us autowire the constructor: without concrete getters, the class is unusable. This just makes it usable again.Commits-------8f246bd [DI] Always consider abstract getters as autowiring candidates
Uh oh!
There was an error while loading.Please reload this page.
When a definition is set to be autowired with no method explicitly configured, we already wire the constructor.
We should also autowire abstract getters - with the same reasoning that makes us autowire the constructor: without concrete getters, the class is unusable. This just makes it usable again.