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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] Add getter injection #20973

Merged
merged 1 commit into from Jan 30, 2017

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 17, 2016

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:

  • wire the concept
  • dump anonymous classes with PhpDumper
  • generate at runtime in ContainerBuilder::createService
  • tests
  • make it work on PHP 5

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch from 4816d13 to 2b924b6 Dec 17, 2016

$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 comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 17, 2016

Member

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

This comment has been minimized.

Copy link
Member

javiereguiluz commented Dec 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!

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch 2 times, most recently from 03c7680 to 3dae76d Dec 17, 2016

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 17, 2016

@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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 18, 2016

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

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

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 18, 2016

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 18, 2016

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

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 18, 2016

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 18, 2016

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch 2 times, most recently from 93e0876 to bb952d6 Dec 18, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 18, 2016

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

This comment has been minimized.

Copy link
Contributor

ro0NL commented Dec 18, 2016

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);

This comment has been minimized.

@jderusse

jderusse Dec 18, 2016

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 18, 2016

Member

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 :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch 2 times, most recently from abbbae9 to 8751f89 Dec 18, 2016

@jean-pasqualini

This comment has been minimized.

Copy link
Contributor

jean-pasqualini commented Dec 19, 2016

@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 nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch from 8751f89 to d6ae0af Dec 19, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 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 :)

@mnapoli

This comment has been minimized.

Copy link
Contributor

mnapoli commented Dec 23, 2016

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch from 6b2cca4 to ab094e2 Jan 10, 2017

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch 4 times, most recently from 6d7ff13 to 970f592 Jan 11, 2017

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 27, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:getter-injection branch from 26ac218 to cb49858 Jan 28, 2017

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 30, 2017

Thank you @nicolas-grekas.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 30, 2017

feature #20973 [DI] Add getter injection (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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:getter-injection branch Jan 30, 2017

@nicolas-grekas nicolas-grekas moved this from In Review to Done in Lower entry bar Jan 31, 2017

@nicolas-grekas nicolas-grekas moved this from In Review to Done in Laziness in core Feb 2, 2017

fabpot added a commit that referenced this pull request Feb 2, 2017

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
@flip111

This comment has been minimized.

Copy link
Contributor

flip111 commented Feb 16, 2017

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

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Feb 16, 2017

@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

This comment has been minimized.

Copy link
Contributor

mmoreram commented Feb 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?
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?)

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…
…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…
…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…
…as)"

This reverts commit 2183f98, reversing
changes made to b465634.

fabpot added a commit that referenced this pull request Mar 25, 2017

feature #22158 Revert "feature #20973 [DI] Add getter injection (nico…
…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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 25, 2017

Reverted in #22158 now that we have other alternatives.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 11, 2017

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

@kiler129

This comment has been minimized.

Copy link
Contributor

kiler129 commented Apr 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.

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment