[DI] Add getter injection #20973
[DI] Add getter injection #20973
Conversation
4816d13 to
2b924b6
Compare
| $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)) { |
There was a problem hiding this comment.
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.
|
@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 to
3dae76d
Compare
|
@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 :/ |
then you have your answer :) see the linked issue for more. |
|
Figured it out. But still.. why make the service a proxy, when we already can proxy the dependency itself ( |
|
As said in the linked issue |
|
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... |
|
I was just answering to your question, but there are more arguments. Please read the linked issue. |
93e0876 to
bb952d6
Compare
|
Implementation is ready, generated code looks good. |
|
Random would be more safe right? I'd go for that :) |
| 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); |
There was a problem hiding this comment.
nullable type hint is not compatible with php >=7.0 && < 7.1
There was a problem hiding this comment.
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 to
8751f89
Compare
|
What would be the list of things to do to bring the support of this functionality on PHP 5.6.X ? |
8751f89 to
d6ae0af
Compare
|
@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 to
23db36b
Compare
|
That means that classes written take advantage of that will be completely dependent on Symfony's container right? |
|
@stof comments addressed |
| namespace Symfony\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.
thanks, fixed
6b2cca4 to
ab094e2
Compare
6d7ff13 to
970f592
Compare
77fb641 to
26ac218
Compare
|
PR updated with |
26ac218 to
cb49858
Compare
|
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
|
Surprised to see such controversial feature get merged. There were various critiques by people who are not in symfony core. |
|
@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. |
|
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 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)"
|
Reverted in #22158 now that we have other alternatives. |
|
Note that |
|
@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. |
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: