[DI] Getter autowiring #21031

dunglas · 2016-12-23T13:04:37Z

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	todo

This PR adds support for getter autowiring. #20973 must be merged first.

Example:

# app/config/config.yml

services:
    Foo\Bar:
        autowire: ['get*']

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

theofidry

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 Bar but an existing service Bar is 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

theofidry · 2017-01-09T23:51:47Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+            return false;
+        }
+
+        if (0 !== $reflectionMethod->getNumberOfParameters() || $reflectionMethod->isFinal() || $reflectionMethod->returnsReference() || !($returnType = $reflectionMethod->getReturnType())) {


Given the length of the line, IMO $returnType = $reflectionMethod->getReturnType() assignment should be on a separate line

IIRC, we prefer long lines in such cases.

for readability: 0 < $r->getNumberOfParameters()?
for the assignment, move it to a next "if" block?
if (!$returnType = $reflectionMethod->getReturnType()) {

theofidry · 2017-01-09T23:52:01Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+            return false;
+        }
+
+        $class = (string) $returnType;


is the cast necessary?

Yes, next methods expect a class name as parameter, not a \ReflectionType instance. An alternative is to call __toString() explicitly.

maybe this could be done in the assignment line $returnType = (string) $reflectionMethod->getReturnType()?

In fact, ReflectionType::__toString is deprecated since PHP 7.1, you should use this instead:
on https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1786

theofidry · 2017-01-09T23:53:57Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

            if (!$definition->isDeprecated() && 0 < strpos($r->getDocComment(), "\n * @deprecated ")) {
                @trigger_error(sprintf('The "%s" service relies on the deprecated "%s" class. It should either be deprecated or its implementation upgraded.', $id, $r->name), E_USER_DEPRECATED);
            }
+            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));


shouldn't this be in the PhpDumper or something?

theofidry · 2017-01-09T23:56:55Z

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

+            // should not be called
+        }
+
+        public function &getReference(): A


Oo, I'm seeing things I should not see :P

Can you elaborate?

Nothing, i've just never seen a function declared with a reference like that

theofidry · 2017-01-09T23:57:00Z

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

+        {
+            // should not be called
+        }
+


one extra line

theofidry · 2017-01-09T23:58:46Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -116,6 +118,8 @@ public function dump(array $options = array())
            'debug' => true,
        ), $options);

+        $this->containerRef = 'container'.substr(strtr(base64_encode(md5($options['namespace'].'\\'.$options['class'].'+'.$options['base_class'], true)), '+/', '__'), 0, -2);


is all of that necessary for the container ref?

nicolas-grekas

👍 once #20973 is merged (with minor comments)

nicolas-grekas · 2017-01-10T15:57:53Z

src/Symfony/Component/DependencyInjection/Tests/Fixtures/GetterOverriding.php

+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures;
+
+if (PHP_VERSION_ID >= 70100) {


this check can be removed:, because it doesn't work anyway: the following will trigger a parse error

nicolas-grekas · 2017-01-10T15:58:14Z

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php

@@ -497,6 +499,32 @@ public function testExplicitMethodInjection()
        );
    }

+    public function testGetterOverriding()
+    {
+        if (PHP_VERSION_ID < 70100) {


should be moved to an @requires PHP 7.1 annotation

nicolas-grekas · 2017-01-30T23:22:06Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+    {
+        foreach ($autowiredMethod as $reflectionMethod) {
+            if (
+                isset($overridenGetters[$reflectionMethod->name]) ||


strtolower($reflectionMethod->name)

nicolas-grekas · 2017-01-30T23:24:01Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+                $this->populateAvailableTypes();
+            }
+
+            $class = $returnType->__toString();


$returnType instanceof \ReflectionNamedType ? $returnType->getName() : $returnType->__toString()
because __toString is deprecated since php 7.1

nicolas-grekas

👍

xabbuh · 2017-02-02T11:17:04Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+     *
+     * @return array
+     */
+    private function autowireOverridenGetters(array $overridenGetters, array $autowiredMethod)


$autowiredMethods

xabbuh · 2017-02-02T11:18:34Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+                || 0 !== $reflectionMethod->getNumberOfParameters()
+                || $reflectionMethod->isFinal()
+                || $reflectionMethod->returnsReference()
+                || !($returnType = $reflectionMethod->getReturnType())


IMO the whole condition deserves some comments about why autowiring is skipped here. Maybe it's even good to split it in several if expressions.

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

Except for the changed needed by the deprecation of autowiring types, this PR looks good to me

stof · 2017-02-02T13:03:37Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+                continue;
+            }
+
+            if (null === $this->types) {


This logic must be updated to account for https://github.com/symfony/symfony/pull/21494/files#diff-62df969ae028c559d33ffd256de1ac49R248 before this line

fabpot · 2017-02-02T19:36:00Z

Can you add a note in the CHANGELOG and mark this as being experimental?

fabpot · 2017-02-02T19:40:13Z

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 | todo This PR adds support for getter autowiring. #20973 must be merged first. Example: ```yaml # app/config/config.yml services: Foo\Bar: autowire: ['get*'] ``` ```php namespace 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

dunglas added the DependencyInjection label Dec 23, 2016

carsonbot added Status: Needs Review Feature labels Dec 23, 2016

dunglas changed the title ~~Getter autowiring~~ [DI] Getter autowiring Dec 23, 2016

nicolas-grekas added this to the 3.3 milestone Dec 26, 2016

stof mentioned this pull request Jan 1, 2017

[RFC] Define services via annotations #21103

Closed

This was referenced Jan 9, 2017

[DI] Add getter injection #20973

Merged

Add support for method autowiring dunglas/DunglasActionBundle#64

Merged

theofidry mentioned this pull request Jan 10, 2017

[DI] Autowiring debug tools #21222

Closed

theofidry suggested changes Jan 10, 2017

View reviewed changes

dunglas force-pushed the getter_autowiring branch 2 times, most recently from 9ed1810 to 3efd930 Compare January 10, 2017 10:37

nicolas-grekas approved these changes Jan 10, 2017

View reviewed changes

dunglas force-pushed the getter_autowiring branch 2 times, most recently from ffa556f to afc023e Compare January 10, 2017 22:33

dunglas mentioned this pull request Jan 11, 2017

[do not merge] Change the default directory structure and use new features of 3.3 symfony/symfony-standard#1034

Closed

dunglas force-pushed the getter_autowiring branch 4 times, most recently from 739697c to 24820be Compare January 30, 2017 21:48

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

dunglas force-pushed the getter_autowiring branch 2 times, most recently from 088daea to f5b09f4 Compare January 31, 2017 09:52

nicolas-grekas approved these changes Jan 31, 2017

View reviewed changes

xabbuh reviewed Feb 2, 2017

View reviewed changes

stof requested changes Feb 2, 2017

View reviewed changes

nicolas-grekas force-pushed the getter_autowiring branch from f5b09f4 to 3bb7fc2 Compare February 2, 2017 19:30

[DI] Add support for getter autowiring

c48c36b

nicolas-grekas force-pushed the getter_autowiring branch from 3bb7fc2 to c48c36b Compare February 2, 2017 19:37

fabpot merged commit c48c36b into symfony:master Feb 2, 2017

javiereguiluz mentioned this pull request Feb 12, 2017

Added support for getter autowiring symfony/symfony-docs#7470

Closed

dunglas mentioned this pull request Feb 13, 2017

[FrameworkBundle] Introduce autowirable ControllerTrait #18193

Closed

dunglas deleted the getter_autowiring branch February 13, 2017 09:23

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Getter autowiring #21031

[DI] Getter autowiring #21031

dunglas commented Dec 23, 2016 •

edited

Loading

theofidry left a comment

theofidry Jan 9, 2017

dunglas Jan 10, 2017

nicolas-grekas Jan 10, 2017 •

edited

Loading

theofidry Jan 9, 2017

dunglas Jan 10, 2017

theofidry Jan 10, 2017

nicolas-grekas Jan 10, 2017

theofidry Jan 9, 2017

theofidry Jan 9, 2017

dunglas Jan 10, 2017

theofidry Jan 10, 2017

theofidry Jan 9, 2017

theofidry Jan 9, 2017

nicolas-grekas left a comment

nicolas-grekas Jan 10, 2017

nicolas-grekas Jan 10, 2017

nicolas-grekas Jan 30, 2017

nicolas-grekas Jan 30, 2017

nicolas-grekas left a comment

xabbuh Feb 2, 2017

xabbuh Feb 2, 2017

nicolas-grekas Feb 2, 2017 •

edited

Loading

stof left a comment

stof Feb 2, 2017

fabpot commented Feb 2, 2017

fabpot commented Feb 2, 2017


		namespace Symfony\Component\DependencyInjection\Tests\Fixtures;

		if (PHP_VERSION_ID >= 70100) {

[DI] Getter autowiring #21031

[DI] Getter autowiring #21031

Conversation

dunglas commented Dec 23, 2016 • edited Loading

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

stof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 2, 2017

fabpot commented Feb 2, 2017

dunglas commented Dec 23, 2016 •

edited

Loading

nicolas-grekas Jan 10, 2017 •

edited

Loading

nicolas-grekas Feb 2, 2017 •

edited

Loading