[DependencyInjection] Autowiring: add setter injection support #17608

dunglas · 2016-01-30T09:53:21Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	todo

Add support for setter injection in the Dependency Injection Component.

Setter injection should be avoided when possible. However, there is some legitimate use cases for it. This PR follows a proposal of @weaverryan to ease using DunglasActionBundle with #16863. The basic idea is to include a setter for the required service in the trait and let the DependencyInjection Component autowire the dependency using the setter.

This way, a newcomer can use the trait without having to create or modify the constructor of the action.

/cc @derrabus

fabpot · 2016-01-31T07:45:01Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+     */
+    private function autowireConstructor($id, Definition $definition, \ReflectionClass $reflectionClass)
+    {
+        if (!($constructor = $reflectionClass->getConstructor())) {


() around the var def should be removed.

fabpot · 2016-01-31T07:48:57Z

This feature is indeed a must have to better support the uses cases described in the linked issue.

sstok · 2016-01-31T13:01:00Z

What if someone wants to use autowiring but only wants to use constructor autowire and not setter autowire? Or not for all methods (in that case setter autowiring should not be used).

dunglas · 2016-01-31T14:01:40Z

@sstok If a call is explicitly defined for a method, the autowiring system will ignore this method.

Supporting only constructor autowiring or both setter and constructor using a new flag in Definition should be possible but it looks overkill too me. If you have such edge case for a service, the clean way is just to not use autowiring at all for this service. What do you think?

dunglas · 2016-02-05T10:18:45Z

@fabpot support for function setDependencies(RouterInterface $router, Twig $twig) style added.

weaverryan · 2016-02-06T18:56:28Z

@dunglas I love it! I just tried the code, here are the issues / concerns I found:

The fact that would we start auto-wiring setters is a behavior change - it could be a BC break (setters are suddenly called that weren't before). Question: is this a BC break?
If I create a setter with no arguments, the setter is called. I don't think it should: in this case, there's nothing to inject.
If a setter does have an argument, but its optional and we can't autowire it, : I think we should do nothing, instead of calling the setter with null.
If a setter does have an argument and it's required, but we can't auto-wire it (e.g. no type-hint), I think we should do nothing (don't call the method, but don't throw an exception - the user may manually add a calls for this).

Thanks!

fabpot · 2016-02-18T15:34:37Z

@dunglas any feedback on @weaverryan comments?

dunglas · 2016-02-18T15:38:00Z

They are all relevants, I need to find some time to work on this.

dunglas · 2016-02-18T15:42:34Z

For the BC break, as @sstok has the same concern, I'll introduce a new aurowireSetter property in Definition that will be false by default and set it to true in action services created by DunglasActionBundle.

weaverryan · 2016-02-18T15:52:20Z

@dunglas That will fix BC, but I'm concerned about usability - e.g.:

services:
    spring_weather_manager:
        class: ...
        autowire: true
        autowire_setter: true

If it weren't for the BC problem, my opinion would be: always do setter injection - after all, if it causes you any issues, don't use it. For that reason, I'd prefer a global flag to turn this on/off that goes away in 4.0.

dunglas · 2016-02-18T15:56:07Z

On the other hand, the BC break is very limited: it will only impact people using autowiring (just a few), having classes with setters for services (unlikely, because it wasn't supported in the first version of the autowiring system), and having a setter that broke the class when triggered (very very unlikely). Maybe can we say that this "BC break" is acceptable, as it's more an edge case than a BC break.

weaverryan · 2016-02-18T16:01:18Z

Honestly, I'm inclined to agree. This is a "magic" feature you're opting into, it's only 1 version it's been available, it won't affect many people, and if it causes an issue, it'll happen at build time (very easy to debug).

fabpot · 2016-02-18T16:27:23Z

👍 for the small BC break instead of yet another configuration setting.

stof · 2016-02-18T16:29:44Z

I agree with you about accepting this small BC break (but we need to document it clearly in changelog files)

weaverryan · 2016-02-29T00:37:11Z

Ok, so BC break is ok. This leaves items 2-4 #17608 (comment), when you have some time :)

fabpot · 2016-03-02T12:49:28Z

@dunglas Any idea when you will be able to finish this one?

dunglas · 2016-03-02T14:07:46Z

Before the end of this week.

dunglas · 2016-03-03T14:15:10Z

All issues fixed. Can you review @weaverryan?

weaverryan · 2016-03-04T02:27:51Z

Hi Dunglas!

I need a few more days to get back a full report - I will make a very clear explanation of the functionality so that everyone is clear how it works (a few cases are almost subjective, so I want it to be clear to everyone).

The only issue I've found is that if we cannot autowire because the type-hint is for a non-existent class, it simply doesn't call the method. That's different than the constructor where it throws an exception.

Also, I think we (probably me, as I've had some recent PR's merged) may have introduced a bug into the master branch. I'm too short on time to look further right this moment, but I'm getting weird results if I type-hint something like DataCollector or EventSusbcribterInterface. These are items that should not be autowireable (as there are multiple services for them), but on master locally right now, they are autowiring - with one of the matching services. This does not happen on 3.0.1. If someone else can verify this, awesome. Otherwise, I'll look more soon.

I think we're getting quite close to mastering the exact behavior we want. We need to get it right for 3.1, because whatever we end up with will probably stay with us due to BC.

GuilhemN · 2016-03-07T21:44:29Z

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

+     *
+     * @throws RuntimeException
+     */
+    private function autowireMethod($id, Definition $definition, \ReflectionMethod $reflectionMethod, $constructor)


What do you think about renaming $constructor to $isConstructor to be clearer?

weaverryan · 2016-04-01T15:51:53Z

I think this PR is ready. The question is: do we want it? It's opinionated (but so is autowiring in general, and it's opt-in). Some of the new autowiring features/uses (like those in DunglasActionBundle) are quite radical, and I think they need to be tested before considering moving further forward (in the core) in that direction. However, if this were merged, it allows more of these ideas to be tested out in the wild - like using traits e.g. RouterTrait) to automatically setter-inject services+shortcuts into your services.

I'm 👍, but I don't feel as confidently about this as I have other things in the past.

@symfony/deciders

xabbuh · 2016-04-05T11:23:08Z

👍 LGTM

dunglas · 2016-04-18T19:09:33Z

Rebased. Can be merged.

dunglas · 2016-06-07T14:12:08Z

@symfony/deciders what do we do of this PR? Should I close it or merge it?

fabpot · 2016-07-01T06:02:23Z

Re-read everything here. @weaverryan and @dunglas did a great job at defining exactly what we should do and what is out of scope. I agree with everything @weaverryan said and as this is what @dunglas implemented, I'm 👍 (I would not advocate its usage for everything but as this is an opt-in feature anyway, this looks good to me).

fabpot · 2016-07-01T06:02:32Z

@dunglas Looks like it needs a rebase

dunglas · 2016-07-01T08:41:59Z

Rebased.

fabpot · 2016-07-01T08:42:58Z

@dunglas I didn't notice but history is really ugly but as there are 2 contribs, gh won't be able to squash commits. Can you do that for me?

dunglas · 2016-07-01T08:56:09Z

Should I merge all commits in 1 or should I keep the commit of @weaverryan?

fabpot · 2016-07-01T08:57:11Z

I think you can merge everything into 1 commit as @weaverryan contrib is not huge.

dunglas · 2016-07-01T08:58:58Z

Done.

fabpot · 2016-07-01T09:01:39Z

Thank you @dunglas.

fabpot · 2016-07-01T09:01:46Z

@dunglas Can you take care of the docs?

@weaverryan

… support (dunglas) This PR was merged into the 3.2-dev branch. Discussion ---------- [DependencyInjection] Autowiring: add setter injection support | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | todo Add support for setter injection in the Dependency Injection Component. Setter injection should be avoided when possible. However, there is some legitimate use cases for it. This PR follows a proposal of @weaverryan to ease using [DunglasActionBundle](https://github.com/dunglas/DunglasActionBundle) with #16863. The basic idea is to include a setter for the required service in the trait and let the DependencyInjection Component autowire the dependency using the setter. This way, a newcomer can use the trait without having to create or modify the constructor of the action. /cc @derrabus Commits ------- a0d7cbe [DependencyInjection] Autowiring: add setter injection support

dunglas · 2016-07-01T09:03:07Z

Yes, I've a lot of docs to write...

…etter injection support (dunglas)" This reverts commit 7eab6b9, reversing changes made to 35f201f.

… add setter injection support (dunglas)" (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- Revert "feature #17608 [DependencyInjection] Autowiring: add setter injection support (dunglas)" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This reverts commit 7eab6b9, reversing changes made to 35f201f. As discussed in #20167 Commits ------- bf91eda Revert "feature #17608 [DependencyInjection] Autowiring: add setter injection support (dunglas)"

@route

…t (dunglas) This PR was squashed before being merged into the 3.3-dev branch (closes #18193). Discussion ---------- [FrameworkBundle] Introduce autowirable ControllerTrait | Q | A | | --- | --- | | Branch | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #16863 | | License | MIT | | Doc PR | todo | This is the missing part of the new controller system and it's fully BC with the old one. Used together with the existing autowiring system, #17608 and [DunglasActionBundle](https://github.com/dunglas/DunglasActionBundle) it permits to inject explicit dependencies in controllers with 0 line of config. It's a great DX improvement for Symfony. It also has a lot of another advantages including enabling to reuse controller accros frameworks and make them easier to test. See https://dunglas.fr/2016/01/dunglasactionbundle-symfony-controllers-redesigned/ for all arguments. Magic methods of old controllers are still available if you use this new trait in actions. For instance, the [`BlogController::newAction`](https://github.com/symfony/symfony-demo/blob/master/src/AppBundle/Controller/Admin/BlogController.php#L70) form the `symfony-demo` can now looks like: ``` php namespace AppBundle\Action\Admin; use AppBundle\Form\PostType; use AppBundle\Utils\Slugger; use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; use Symfony\Bundle\FrameworkBundle\Controller\ControllerTrait; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Form\Extension\Core\Type\SubmitType; class NewAction { use ControllerTrait; private $slugger; public function __construct(Slugger $slugger) { $this->slugger = $slugger; } /** * @route("/new", name="admin_post_new") */ public function __invoke(Request $request) { $post = new Post(); $post->setAuthorEmail($this->getUser()->getEmail()); $form = $this->createForm(PostType::class, $post)->add('saveAndCreateNew', SubmitType::class); $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { $post->setSlug($this->slugger->slugify($post->getTitle())); $entityManager = $this->getDoctrine()->getManager(); $entityManager->persist($post); $entityManager->flush(); $this->addFlash('success', 'post.created_successfully'); if ($form->get('saveAndCreateNew')->isClicked()) { return $this->redirectToRoute('admin_post_new'); } return $this->redirectToRoute('admin_post_index'); } return $this->render('admin/blog/new.html.twig', array( 'post' => $post, 'form' => $form->createView(), )); } } ``` As you can see, there is no need to register the `slugger` service in `services.yml` anymore and the dependency is explicitly injected. In fact the container is not injected in controllers anymore. Convenience methods still work if the `ControllerTrait` is used (of course it's not mandatory). Here I've made the choice to use an invokable class but this is 100% optional, a class can still contain several actions if wanted. Annotations like `@Route` still work too. The old `abstract` controller isn't deprecated. There is no valid reason to deprecate it IMO. People liking using the "old" way still can. Unless in #16863, there is only one trait. This trait/class is basically a bunch of proxy methods to help newcomers. If you want to use only some methods, or want explicit dependencies (better), just inject the service you need in the constructor and don't use the trait. I'll create open a PR on the standard edition soon to include ActionBundle and provide a dev version of the standard edition to be able to play with this new system. I'll also backport tests added to the old controller test in 2.3+. **Edit:** It now uses getter injection to benefit from lazy service loading by default. Commits ------- 1f2521e [FrameworkBundle] Introduce autowirable ControllerTrait

dunglas added the DependencyInjection label Jan 30, 2016

carsonbot added the Status: Needs Review label Jan 30, 2016

dunglas mentioned this pull request Jan 30, 2016

[FrameworkBundle] Split abstract Controller class into traits #16863

Closed

fabpot reviewed Jan 31, 2016
View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Feb 20, 2016

javiereguiluz added the Feature label Mar 3, 2016

dunglas force-pushed the autowiring-setter branch from 5cab494 to f690258 Compare March 3, 2016 14:12

dunglas mentioned this pull request Mar 7, 2016

Auto-registering commands dunglas/DunglasActionBundle#10

Closed

GuilhemN reviewed Mar 7, 2016
View reviewed changes

dunglas force-pushed the autowiring-setter branch from 056612c to 5470db2 Compare April 18, 2016 19:08

dunglas force-pushed the autowiring-setter branch from 5511bc4 to 8c34d6d Compare July 1, 2016 08:36

[DependencyInjection] Autowiring: add setter injection support

a0d7cbe

dunglas force-pushed the autowiring-setter branch from 786a915 to a0d7cbe Compare July 1, 2016 08:58

fabpot merged commit a0d7cbe into symfony:master Jul 1, 2016

dunglas deleted the autowiring-setter branch July 1, 2016 09:02

xabbuh mentioned this pull request Jul 4, 2016

Autowring support for setter methods symfony/symfony-docs#6713

Closed

dunglas mentioned this pull request Aug 17, 2016

[DependencyInjection] Introduce a flag to enable setter autowiring #19631

Closed

dunglas mentioned this pull request Oct 6, 2016

[DependencyInjection] Docs for method autowiring symfony/symfony-docs#7041

Closed

This was referenced Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

[DependencyInjection] Make method (setter) autowiring configurable #20167

Merged

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Nov 2, 2016

Revert "feature symfony#17608 [DependencyInjection] Autowiring: add s…

bf91eda

…etter injection support (dunglas)" This reverts commit 7eab6b9, reversing changes made to 35f201f.

weaverryan mentioned this pull request Mar 17, 2017

[DI] Remove skipping magic for autowired methods #22030

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Autowiring: add setter injection support #17608

[DependencyInjection] Autowiring: add setter injection support #17608

dunglas commented Jan 30, 2016

fabpot Jan 31, 2016

fabpot commented Jan 31, 2016

sstok commented Jan 31, 2016

dunglas commented Jan 31, 2016

dunglas commented Feb 5, 2016

weaverryan commented Feb 6, 2016

fabpot commented Feb 18, 2016

dunglas commented Feb 18, 2016

dunglas commented Feb 18, 2016

weaverryan commented Feb 18, 2016

dunglas commented Feb 18, 2016

weaverryan commented Feb 18, 2016

fabpot commented Feb 18, 2016

stof commented Feb 18, 2016

weaverryan commented Feb 29, 2016

fabpot commented Mar 2, 2016

dunglas commented Mar 2, 2016

dunglas commented Mar 3, 2016

weaverryan commented Mar 4, 2016

GuilhemN Mar 7, 2016

weaverryan commented Apr 1, 2016

xabbuh commented Apr 5, 2016

dunglas commented Apr 18, 2016

dunglas commented Jun 7, 2016

fabpot commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

fabpot commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

[DependencyInjection] Autowiring: add setter injection support #17608

[DependencyInjection] Autowiring: add setter injection support #17608

Conversation

dunglas commented Jan 30, 2016

fabpot Jan 31, 2016

Choose a reason for hiding this comment

fabpot commented Jan 31, 2016

sstok commented Jan 31, 2016

dunglas commented Jan 31, 2016

dunglas commented Feb 5, 2016

weaverryan commented Feb 6, 2016

fabpot commented Feb 18, 2016

dunglas commented Feb 18, 2016

dunglas commented Feb 18, 2016

weaverryan commented Feb 18, 2016

dunglas commented Feb 18, 2016

weaverryan commented Feb 18, 2016

fabpot commented Feb 18, 2016

stof commented Feb 18, 2016

weaverryan commented Feb 29, 2016

fabpot commented Mar 2, 2016

dunglas commented Mar 2, 2016

dunglas commented Mar 3, 2016

weaverryan commented Mar 4, 2016

GuilhemN Mar 7, 2016

Choose a reason for hiding this comment

weaverryan commented Apr 1, 2016

xabbuh commented Apr 5, 2016

dunglas commented Apr 18, 2016

dunglas commented Jun 7, 2016

fabpot commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016

fabpot commented Jul 1, 2016

fabpot commented Jul 1, 2016

dunglas commented Jul 1, 2016