[DI] Add getter injection #20973

nicolas-grekas · 2016-12-17T15:32:36Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20657, #835
License	MIT
Doc PR	symfony/symfony-docs#7300

Getter overriding by the container will allow a new kind of dependency injection which enables easier laziness and more immutable classes, by not requiring any corresponding setter. See linked issue for more.

This is WIP:

nicolas-grekas · 2016-12-17T15:36:52Z

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

-                    $arguments = array_merge($service->getMethodCalls(), $service->getArguments(), $service->getProperties());
-
-                    if ($this->hasReference($id, $arguments, $deep, $visited)) {
+                    if ($this->hasReference($id, $service->getMethodCalls(), $deep, $visited) || $this->hasReference($id, $service->getArguments(), $deep, $visited) || $this->hasReference($id, $service->getProperties(), $deep, $visited)) {


This diff is pure cs, yet is allows to highlight that there is no use of getOverriddenGetters here. The reason is that overridden getters are lazy edges, thus should be considered the same as lazy services a few line above.

javiereguiluz · 2016-12-17T15:42:58Z

@nicolas-grekas thanks for this PR and for your other proposals to improve the DI component.

Given that this kind of concepts are a bit abstract, it'd be nice if you displayed a very simple before/after comparison code sample showing the benefits. Thanks!

ro0NL · 2016-12-17T22:50:52Z

@nicolas-grekas im curious.. why would one choose getter overriding over setter/constructor injection?

It's really cool.. but it allows to bypass traditional service definitions like;

dep:
   class: ...
   lazy: true

service:
  class: Service
  arguments: ['@dep']

with

service:
  class: Service
  getters:
     getDep: '@dep'

and creating classes like

class Service {
   public function getDep() {} // stub
}

edit: i see the benfit for being lazy though :/

nicolas-grekas · 2016-12-18T08:50:50Z

edit: i see the benfit for being lazy though :/

then you have your answer :) see the linked issue for more.

ro0NL · 2016-12-18T10:31:55Z

Figured it out. But still.. why make the service a proxy, when we already can proxy the dependency itself (lazy: true).

nicolas-grekas · 2016-12-18T10:35:05Z

As said in the linked issue lazy: true has no effect when ocramius/proxy-manager is not installed.
Which means it's a userland only feature, we can't rely on it in the core.

ro0NL · 2016-12-18T10:45:00Z

Ok.. if that's the case i'd favor a core implementation of lazy: true. Personally i have no problems with a package less or more.

I think i'd stick with lazy dependencies, opposed to lazy services. It feels weird to do a different approach only to save on a package...

nicolas-grekas · 2016-12-18T10:56:32Z

I was just answering to your question, but there are more arguments. Please read the linked issue.

nicolas-grekas · 2016-12-18T13:20:13Z

Implementation is ready, generated code looks good.
Note that the anonymous proxy classes need a property or two, whose name shouldn't collide with any existing properties in the service class.
I chose a 1337 5|*34|< strategy, naming those: private $c0nt41n3r and private $g3ttErV4lu35;.
We could have a randomly generated suffix instead. WDYT?

ro0NL · 2016-12-18T13:35:48Z

Random would be more safe right? I'd go for that :)

jderusse · 2016-12-18T13:52:08Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+                throw new RuntimeException(sprintf('Invalid getter for service "%s": method "%s::%s" has parameters.', $id, $class->name, $name));
+            }
+            if ($type = $r->getReturnType()) {
+                $type = ': '.($type->allowsNull() ? '?' : '').$this->generateTypeHint($type, $r);


nullable type hint is not compatible with php >=7.0 && < 7.1

Indeed, and this code is fine on php7.0 also because of the above "if". On 7.0 a return type can't be nullable, but the method exists. So: all is fine :)

jean-pasqualini · 2016-12-19T08:20:14Z

@nicolas-grekas

What would be the list of things to do to bring the support of this functionality on PHP 5.6.X ?

nicolas-grekas · 2016-12-19T09:15:32Z

@jean-pasqualini: we'd need to replace the anonymous inline classes by real classes, dumped in the same file as the container, just after it (as done for lazy-proxies right now by the addProxyClasses method). There is no major blocker I guess, just more time to spend, so maybe later, maybe by someone else than me :)

mnapoli · 2016-12-23T14:31:05Z

That means that classes written take advantage of that will be completely dependent on Symfony's container right?

nicolas-grekas · 2017-01-10T14:49:33Z

@stof comments addressed

jakzal · 2017-01-10T17:06:05Z

src/Symfony/Component/DependencyInjection/LazyProxy/GetterProxyInterface.php

+namespace Symfony\Component\DependencyInjection\LazyProxy;
+
+/**
+ * Interface used to labels proxy classes with overridden getters as generated while compiling the container.


Typo: "used to label"

thanks, fixed

nicolas-grekas · 2017-01-27T10:06:49Z

PR updated with @experimental mentions, as explained in http://symfony.com/blog/experimental-features
Ready to merge :)

fabpot · 2017-01-30T19:43:40Z

Thank you @nicolas-grekas.

This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Add getter injection | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20657 | License | MIT | Doc PR | symfony/symfony-docs#7300 Getter overriding by the container will allow a new kind of dependency injection which enables easier laziness and more immutable classes, by not requiring any corresponding setter. See linked issue for more. This is WIP: - [x] wire the concept - [x] dump anonymous classes with PhpDumper - [x] generate at runtime in ContainerBuilder::createService - [x] tests - [x] make it work on PHP 5 Commits ------- cb49858 [DI] Add getter injection

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

flip111 · 2017-02-16T12:09:56Z

Surprised to see such controversial feature get merged. There were various critiques by people who are not in symfony core.

dunglas · 2017-02-16T12:51:19Z

@flip111 it has been merged as an experimental feature (it may be removed if we're not convinced by it). Anyway, as with all Symfony features, you don't have to use it if you don't like it.

mmoreram · 2017-02-21T13:00:48Z

Hello everyone.

I'm so sure that is too late, but anyway, I would like to expose my POV.

I'm sorry but... @nicolas-grekas what is that?
I mean, even if experimental... what is this new feature trying to solve?
Dependency Injection should be a component with the capability to solve and implements some patterns to allow people to expose their services into the framework. At least, I think that this was meant to be at the beginning.

DIC should be a layer to make our projects better. Are we agree about this?
The Symfony DIC started by adding some extra features, not helping us making better code (for example, allowing us to add dependencies in public parameters, or by adding setters), but the explanation of "We should help as much as possible new adopters to adapt their code to this new way of thinking" make me feel so good with that.

So... then what?
Do you (and when I say you I mean the core Symfony team) still remember what is the purpose of Symfony? Or the question should be... what is YOUR purpose of Symfony right now? Make a tool for everyone, in every situation and in every single kind of project?

It should be not.

Then... maybe is to provide a good RAD layer to new adopters? And do you think that these kind of features will help them to understand better the pattern and the project? Don't you think that these kind of features (like Autowiring, BTW) will only cause the effect of making new people understand in a bad way what the Dependency Injection is?

If your obsession is RAD, please, focus on real RAD

Easier way of defining bundles
Better code (by better code I mean better readable code)
Better, bigger and stronger communities and events

Please, focus on what's important for all the Framework and components, and don't be obsessed on do more, and more, and more, even if is not important or it should not be part of the core... (this is why we have bundles, right?)

…as)" This reverts commit 2183f98, reversing changes made to b465634.

…las-grekas)" (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- Revert "feature #20973 [DI] Add getter injection (nicolas-grekas)" This reverts commit 2183f98, reversing changes made to b465634. | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no (master only) | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Let's remove getter injection, we now have enough alternative mechanisms to achieve almost the same results (e.g. `ServiceSubscriberInterface`, see #21708)., and I'm tired being called by names because of it. The only use case in core is `ControllerTrait`, but this should be gone if #22157 is merged. Commits ------- 23fa3a0 Revert "feature #20973 [DI] Add getter injection (nicolas-grekas)"

nicolas-grekas · 2017-03-25T21:25:00Z

Reverted in #22158 now that we have other alternatives.

nicolas-grekas · 2017-04-11T07:42:50Z

Note that JMSDiExtraBundle provides getter injection since years:
http://jmsyst.com/bundles/JMSDiExtraBundle/master/usage#method-getter-injection

kiler129 · 2017-04-26T16:45:39Z

@nicolas-grekas: About what alternatives you're talking about? I'm fiddling around trying to find why this feature was reverted - blog post didn't mentioned that.

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

nicolas-grekas force-pushed the getter-injection branch from 4816d13 to 2b924b6 Compare December 17, 2016 15:33

nicolas-grekas commented Dec 17, 2016

View reviewed changes

nicolas-grekas force-pushed the getter-injection branch 2 times, most recently from 03c7680 to 3dae76d Compare December 17, 2016 22:49

nicolas-grekas force-pushed the getter-injection branch 2 times, most recently from 93e0876 to bb952d6 Compare December 18, 2016 13:17

jderusse reviewed Dec 18, 2016

View reviewed changes

nicolas-grekas force-pushed the getter-injection branch 2 times, most recently from abbbae9 to 8751f89 Compare December 18, 2016 23:22

nicolas-grekas force-pushed the getter-injection branch from 8751f89 to d6ae0af Compare December 19, 2016 08:59

nicolas-grekas force-pushed the getter-injection branch 2 times, most recently from af71bbf to 23db36b Compare December 20, 2016 09:02

javiereguiluz added the DependencyInjection label Dec 20, 2016

ro0NL mentioned this pull request Dec 20, 2016

[DI][Proposal] Support anonymous classes in service definition #20997

Closed

This was referenced Dec 21, 2016

[FrameworkBundle] Introduce autowirable ControllerTrait #18193

Closed

[DI] Getter autowiring #21031

Merged

jakzal reviewed Jan 10, 2017

View reviewed changes

nicolas-grekas force-pushed the getter-injection branch from 6b2cca4 to ab094e2 Compare January 10, 2017 17:14

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

nicolas-grekas force-pushed the getter-injection branch 4 times, most recently from 6d7ff13 to 970f592 Compare January 18, 2017 15:48

nicolas-grekas force-pushed the getter-injection branch 3 times, most recently from 77fb641 to 26ac218 Compare January 26, 2017 23:08

[DI] Add getter injection

cb49858

nicolas-grekas force-pushed the getter-injection branch from 26ac218 to cb49858 Compare January 28, 2017 07:50

fabpot merged commit cb49858 into symfony:master Jan 30, 2017

nicolas-grekas deleted the getter-injection branch January 30, 2017 19:44

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Mar 25, 2017

Revert "feature symfony#20973 [DI] Add getter injection (nicolas-grek…

a078eba

…as)" This reverts commit 2183f98, reversing changes made to b465634.

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Mar 25, 2017

Revert "feature symfony#20973 [DI] Add getter injection (nicolas-grek…

ac7f875

…as)" This reverts commit 2183f98, reversing changes made to b465634.

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Mar 25, 2017

Revert "feature symfony#20973 [DI] Add getter injection (nicolas-grek…

23fa3a0

…as)" This reverts commit 2183f98, reversing changes made to b465634.

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] Add getter injection #20973

[DI] Add getter injection #20973

nicolas-grekas commented Dec 17, 2016 •

edited

Loading

nicolas-grekas Dec 17, 2016 •

edited

Loading

javiereguiluz commented Dec 17, 2016

ro0NL commented Dec 17, 2016 •

edited

Loading

nicolas-grekas commented Dec 18, 2016

ro0NL commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

ro0NL commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

ro0NL commented Dec 18, 2016

jderusse Dec 18, 2016

nicolas-grekas Dec 18, 2016

jean-pasqualini commented Dec 19, 2016

nicolas-grekas commented Dec 19, 2016

mnapoli commented Dec 23, 2016

nicolas-grekas commented Jan 10, 2017

jakzal Jan 10, 2017

nicolas-grekas Jan 10, 2017

nicolas-grekas commented Jan 27, 2017

fabpot commented Jan 30, 2017

flip111 commented Feb 16, 2017

dunglas commented Feb 16, 2017

mmoreram commented Feb 21, 2017

nicolas-grekas commented Mar 25, 2017

nicolas-grekas commented Apr 11, 2017

kiler129 commented Apr 26, 2017

[DI] Add getter injection #20973

[DI] Add getter injection #20973

Conversation

nicolas-grekas commented Dec 17, 2016 • edited Loading

nicolas-grekas Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

javiereguiluz commented Dec 17, 2016

ro0NL commented Dec 17, 2016 • edited Loading

nicolas-grekas commented Dec 18, 2016

ro0NL commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

ro0NL commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

ro0NL commented Dec 18, 2016

jderusse Dec 18, 2016

Choose a reason for hiding this comment

nicolas-grekas Dec 18, 2016

Choose a reason for hiding this comment

jean-pasqualini commented Dec 19, 2016

nicolas-grekas commented Dec 19, 2016

mnapoli commented Dec 23, 2016

nicolas-grekas commented Jan 10, 2017

jakzal Jan 10, 2017

Choose a reason for hiding this comment

nicolas-grekas Jan 10, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Jan 27, 2017

fabpot commented Jan 30, 2017

flip111 commented Feb 16, 2017

dunglas commented Feb 16, 2017

mmoreram commented Feb 21, 2017

nicolas-grekas commented Mar 25, 2017

nicolas-grekas commented Apr 11, 2017

kiler129 commented Apr 26, 2017

nicolas-grekas commented Dec 17, 2016 •

edited

Loading

nicolas-grekas Dec 17, 2016 •

edited

Loading

ro0NL commented Dec 17, 2016 •

edited

Loading