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] Getter autowiring#21031
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
[DI] Getter autowiring#21031
Uh oh!
There was an error while loading.Please reload this page.
Conversation
theofidry left a comment
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.
I can't help to find it extremely dirty and tricky, as you're really starting to have any method/typehint registering services left and right.
Besides my comments, there is two things I would like to ask/address:
- How does it works when auto-wiring is not explicit? For example when the typehint is an interface? When the return typehint is the class
Barbut an existing serviceBaris already registered? - My other more serious concern is how easy/hard it is to debug?
I opened#21222 for the second point to avoid hijacking this thread
| if (!method_exists($reflectionMethod,'getReturnType')) { | ||
| returnfalse; | ||
| } | ||
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.
Given the length of the line, IMO$returnType = $reflectionMethod->getReturnType() assignment should be on a separate line
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.
IIRC, we prefer long lines in such cases.
nicolas-grekasJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
for readability:0 < $r->getNumberOfParameters()?
for the assignment, move it to a next "if" block?if (!$returnType = $reflectionMethod->getReturnType()) {
| if (0 !==$reflectionMethod->getNumberOfParameters() ||$reflectionMethod->isFinal() ||$reflectionMethod->returnsReference() || !($returnType =$reflectionMethod->getReturnType())) { | ||
| returnfalse; | ||
| } | ||
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.
is the cast necessary?
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.
Yes, next methods expect a class name as parameter, not a\ReflectionType instance. An alternative is to call__toString() explicitly.
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 this could be done in the assignment line$returnType = (string) $reflectionMethod->getReturnType()?
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.
In fact,ReflectionType::__toString is deprecated since PHP 7.1, you should use this instead:
onhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1786
| } | ||
| if ($definition->getOverriddenGetters()) { | ||
| $salt =str_replace('.','',uniqid('',true)); | ||
| $service =eval(sprintf('return new class(...$arguments) extends %s { private $container%3$s; private $values%3$s; %s };',$r->name,$this->addOverriddenGetters($id,$definition,$r,$salt),$salt)); |
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 this be in thePhpDumper or something?
| // should not be called | ||
| } | ||
| publicfunction &getReference():A |
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.
Oo, I'm seeing things I should not see :P
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.
Can you elaborate?
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.
Nothing, i've just never seen a function declared with a reference like that
| { | ||
| // should not be called | ||
| } | ||
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.
one extra line
| 'debug' =>true, | ||
| ),$options); | ||
| $this->containerRef ='container'.substr(strtr(base64_encode(md5($options['namespace'].'\\'.$options['class'].'+'.$options['base_class'],true)),'+/','__'),0, -2); |
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.
is all of that necessary for the container ref?
9ed1810 to3efd930Comparenicolas-grekas left a comment
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.
👍 once#20973 is merged (with minor comments)
| namespaceSymfony\Component\DependencyInjection\Tests\Fixtures; | ||
| if (PHP_VERSION_ID >=70100) { |
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.
this check can be removed:, because it doesn't work anyway: the following will trigger a parse error
| publicfunctiontestGetterOverriding() | ||
| { | ||
| if (PHP_VERSION_ID <70100) { |
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.
should be moved to an@requires PHP 7.1 annotation
ffa556f toafc023eCompare739697c to24820beCompare| privatefunctionautowireOverridenGetters(array$overridenGetters,array$autowiredMethod) | ||
| { | ||
| foreach ($autowiredMethodas$reflectionMethod) { | ||
| if ( |
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.
strtolower($reflectionMethod->name)
| if (null ===$this->types) { | ||
| $this->populateAvailableTypes(); | ||
| } | ||
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.
$returnType instanceof \ReflectionNamedType ? $returnType->getName() : $returnType->__toString()
because__toString is deprecated since php 7.1
088daea tof5b09f4Comparenicolas-grekas left a comment
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.
👍
| * @param array $autowiredMethod | ||
| * | ||
| * @return array | ||
| */ |
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.
$autowiredMethods
| || !method_exists($reflectionMethod,'getReturnType') | ||
| ||0 !==$reflectionMethod->getNumberOfParameters() | ||
| ||$reflectionMethod->isFinal() | ||
| ||$reflectionMethod->returnsReference() |
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.
IMO the whole condition deserves some comments about why autowiring is skipped here. Maybe it's even good to split it in severalif expressions.
nicolas-grekasFeb 2, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I can partially answer here: this list is just the list of cases where explicit getter injection throws an exception already - thus cases where autowiring should just skip.
A comment would be nice for sure, but splitting the condition is not needed imho.
stof left a comment
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.
Except for the changed needed by the deprecation of autowiring types, this PR looks good to me
| ) { | ||
| continue; | ||
| } | ||
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.
This logic must be updated to account forhttps://github.com/symfony/symfony/pull/21494/files#diff-62df969ae028c559d33ffd256de1ac49R248 before this line
f5b09f4 to3bb7fc2Comparefabpot commentedFeb 2, 2017
Can you add a note in the CHANGELOG and mark this as being experimental? |
3bb7fc2 toc48c36bComparefabpot commentedFeb 2, 2017
Thank you@dunglas. |
This PR was merged into the 3.3-dev branch.Discussion----------[DI] Getter autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | todoThis PR adds support for getter autowiring.#20973 must be merged first.Example:```yaml# app/config/config.ymlservices: Foo\Bar: autowire: ['get*']``````phpnamespace Foo;class Bar{ protected function getBaz(): Baz // this feature only works with PHP 7+ { }}class Baz{}`````Baz` will be automatically registered as a service and an instance will be returned when `Bar::getBaz` will be called (and only at this time, lazy loading).This feature requires PHP 7 or superior.Commits-------c48c36b [DI] Add support for getter autowiring
Uh oh!
There was an error while loading.Please reload this page.
This PR adds support for getter autowiring.#20973 must be merged first.
Example:
Bazwill be automatically registered as a service and an instance will be returned whenBar::getBazwill be called (and only at this time, lazy loading).This feature requires PHP 7 or superior.