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] Add getter injection#20973
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
4816d13 to2b924b6Compare| $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)) { |
nicolas-grekasDec 17, 2016 • 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.
This diff is pure cs, yet is allows to highlight that there is no use ofgetOverriddenGetters here. The reason is that overridden getters are lazy edges, thus should be considered the same as lazy services a few line above.
javiereguiluz commentedDec 17, 2016
@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! |
03c7680 to3dae76dComparero0NL commentedDec 17, 2016 • 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.
@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:trueservice:class:Servicearguments:['@dep'] with service:class:Servicegetters:getDep:'@dep' and creating classes like class Service {publicfunctiongetDep() {}// stub} edit: i see the benfit for being lazy though :/ |
nicolas-grekas commentedDec 18, 2016
then you have your answer :) see the linked issue for more. |
ro0NL commentedDec 18, 2016
Figured it out. But still.. why make the service a proxy, when we already can proxy the dependency itself ( |
nicolas-grekas commentedDec 18, 2016
As said in the linked issue |
ro0NL commentedDec 18, 2016
Ok.. if that's the case i'd favor a core implementation of 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 commentedDec 18, 2016
I was just answering to your question, but there are more arguments. Please read the linked issue. |
93e0876 tobb952d6Comparenicolas-grekas commentedDec 18, 2016
Implementation is ready, generated code looks good. |
ro0NL commentedDec 18, 2016
Random would be more safe right? I'd go for that :) |
| thrownewRuntimeException(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); |
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.
nullable type hint is not compatible with php >=7.0 && < 7.1
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, 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 :)
abbbae9 to8751f89Comparejean-pasqualini commentedDec 19, 2016
What would be the list of things to do to bring the support of this functionality on PHP 5.6.X ? |
8751f89 tod6ae0afComparenicolas-grekas commentedDec 19, 2016
@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 :) |
af71bbf to23db36bComparemnapoli commentedDec 23, 2016
That means that classes written take advantage of that will be completely dependent on Symfony's container right? |
nicolas-grekas commentedJan 10, 2017
@stof comments addressed |
| namespaceSymfony\Component\DependencyInjection\LazyProxy; | ||
| /** | ||
| * Interface used to labels proxy classes with overridden getters as generated while compiling the container. |
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.
Typo: "used to label"
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.
thanks, fixed
6b2cca4 toab094e2Compare6d7ff13 to970f592Compare77fb641 to26ac218Comparenicolas-grekas commentedJan 27, 2017
PR updated with |
26ac218 tocb49858Comparefabpot commentedJan 30, 2017
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#7300Getter 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 5Commits-------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 | 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
flip111 commentedFeb 16, 2017
Surprised to see such controversial feature get merged. There were various critiques by people who are not in symfony core. |
dunglas commentedFeb 16, 2017
@flip111 it has been merged as anexperimental 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 commentedFeb 21, 2017
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? DIC should be a layer to make our projects better. Are we agree about this? So... then what? 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
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?) |
…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 commit2183f98, reversingchanges made tob465634.| 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 commentedMar 25, 2017
Reverted in#22158 now that we have other alternatives. |
nicolas-grekas commentedApr 11, 2017
Note that |
kiler129 commentedApr 26, 2017
@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. |
Uh oh!
There was an error while loading.Please reload this page.
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: