[DI] Getter autowiring #21031

Merged
merged 1 commit into from Feb 2, 2017
@dunglas
Member
dunglas commented Dec 23, 2016 edited
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.

@dunglas dunglas changed the title from Getter autowiring to [DI] Getter autowiring Dec 23, 2016
@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Dec 26, 2016
@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

+ return false;
+ }
+
+ if (0 !== $reflectionMethod->getNumberOfParameters() || $reflectionMethod->isFinal() || $reflectionMethod->returnsReference() || !($returnType = $reflectionMethod->getReturnType())) {
@theofidry
theofidry Jan 10, 2017 Contributor

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

@dunglas
dunglas Jan 10, 2017 Member

IIRC, we prefer long lines in such cases.

@nicolas-grekas
nicolas-grekas Jan 10, 2017 edited Member

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

+ return false;
+ }
+
+ $class = (string) $returnType;
@theofidry
theofidry Jan 10, 2017 Contributor

is the cast necessary?

@dunglas
dunglas Jan 10, 2017 Member

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

@theofidry
theofidry Jan 10, 2017 Contributor

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

@nicolas-grekas
nicolas-grekas Jan 10, 2017 Member

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

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));
@theofidry
theofidry Jan 10, 2017 Contributor

shouldn't this be in the PhpDumper or something?

+ // should not be called
+ }
+
+ public function &getReference(): A
@theofidry
theofidry Jan 10, 2017 Contributor

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

@dunglas
dunglas Jan 10, 2017 Member

Can you elaborate?

@theofidry
theofidry Jan 10, 2017 Contributor

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

+ {
+ // should not be called
+ }
+
@theofidry
theofidry Jan 10, 2017 Contributor

one extra line

@@ -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);
@theofidry
theofidry Jan 10, 2017 Contributor

is all of that necessary for the container ref?

@nicolas-grekas

๐Ÿ‘ once #20973 is merged (with minor comments)

+
+namespace Symfony\Component\DependencyInjection\Tests\Fixtures;
+
+if (PHP_VERSION_ID >= 70100) {
@nicolas-grekas
nicolas-grekas Jan 10, 2017 Member

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

@@ -497,6 +499,32 @@ public function testExplicitMethodInjection()
);
}
+ public function testGetterOverriding()
+ {
+ if (PHP_VERSION_ID < 70100) {
@nicolas-grekas
nicolas-grekas Jan 10, 2017 Member

should be moved to an @requires PHP 7.1 annotation

+ {
+ foreach ($autowiredMethod as $reflectionMethod) {
+ if (
+ isset($overridenGetters[$reflectionMethod->name]) ||
@nicolas-grekas
nicolas-grekas Jan 30, 2017 Member

strtolower($reflectionMethod->name)

+ $this->populateAvailableTypes();
+ }
+
+ $class = $returnType->__toString();
@nicolas-grekas
nicolas-grekas Jan 30, 2017 Member

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

@nicolas-grekas

๐Ÿ‘

+ *
+ * @return array
+ */
+ private function autowireOverridenGetters(array $overridenGetters, array $autowiredMethod)
@xabbuh
xabbuh Feb 2, 2017 Member

$autowiredMethods

+ || 0 !== $reflectionMethod->getNumberOfParameters()
+ || $reflectionMethod->isFinal()
+ || $reflectionMethod->returnsReference()
+ || !($returnType = $reflectionMethod->getReturnType())
@xabbuh
xabbuh Feb 2, 2017 Member

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.

@nicolas-grekas
nicolas-grekas Feb 2, 2017 edited Member

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

+ continue;
+ }
+
+ if (null === $this->types) {
@fabpot
Member
fabpot commented Feb 2, 2017

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

@fabpot
Member
fabpot commented Feb 2, 2017

Thank you @dunglas.

@fabpot fabpot merged commit c48c36b into symfony:master Feb 2, 2017
@fabpot fabpot added a commit that referenced this pull request Feb 2, 2017
@fabpot fabpot feature #21031 [DI] Getter autowiring (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
b50efa5
@nicolas-grekas nicolas-grekas moved from In Review to Done in Lower entry bar Feb 3, 2017
@dunglas dunglas deleted the dunglas:getter_autowiring branch Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment