[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions #21771

nicolas-grekas · 2017-02-26T21:45:31Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	(no test yet)
Fixed tickets	-
License	MIT
Doc PR	-

Talking with @simensen and @weaverryan, we wondered if we could leverage the ArgumentResolver mechanism to make it inject services on demand, using e.g. autowiring.

class PostController
{
  public function indexAction(Request $request, PostRepository $postRepository)
  {
    // PostRepository comes from the container
    $postRepository->findAll(); // ...
  }
}

This PR achieves that, using a new "controller.service_arguments" tag. Typically:

services:
    AppBundle\Controller\PostController:
        autowire: true
        tags:
            - name: controller.service_arguments

It also supports with explicit wiring (thus doesn't necessarily require autowiring if you don't want to use it):

services:
    AppBundle\Controller\PostController:
        tags:
            - name: controller.service_arguments
              action: fooAction
              argument: logger
              id: my_logger

~~The attached diff is bigger than strictly required for now, until #21770 is merged.~~

Todo:

rebase on top of [DI] Allow extensions to create ServiceLocator as services #21770 when merged
add tests
add cleaning pass to remove empty service locators

linaori · 2017-02-27T10:35:31Z

~~I'm personally against this suggestion, especially now that autowiring the constructor is a lot easier: #21164 (comment)~~

@nicolas-grekas made some excellent points regarding DX. I will not use it myself, but it's beneficial when it comes to lazy loading, which can fully replace the container usage, which I highly encourage!

If you ask me:

Constructor stateless dependencies, e.g. services, things that are the same no matter what action is called.

Action stateful dependencies, e.g. entities, parameters or other objects that can have different states each time it gets called.

The session can be different each time it's called, therefore it shouldn't be a constructor dependency. However, when using the Request::getSession(), you don't have access to getFlashBag(), which is Session specific.

I think that adding the ability to "inject" services into the action arguments, will add a layer of magic that makes it more complex in combination with autowiring.

nicolas-grekas · 2017-03-06T15:42:21Z

Now with explicit wiring:
<tag name="routing.controller" action="fooAction" argument="logger" id="logger"/>
repeat the tag for several args/actions.

And with tests.
PR is thus ready.
Status: needs review

@iltar: except when there is only one action (e.g. __invoke style - but this is not the general case), constructor injection is not an option for controllers, because this kind of injection misses a core property that is required here: laziness.

That's why people inject the container instead in fact (and/or extend the base controller really).
We all know that injecting the container is an anti-pattern - the bad kind of service locators.

By injecting services into actions, we get laziness, and we ask people for writing a type hint instead of calling $this->get('foo'). This makes dependencies explicit - thus helps the reader, and the IDE (auto-completion without the clever phpstorm plugin for Symfony). DX wise, it's a good trade - an enjoyable one to me.

The alternatives we have that provide laziness are getter injection - and service subscribers. But these won't provide the same DX here (getter injection is nice when shipped via a trait, see #18193).

All in all, this will provide a seamless DX to me, without making people write "bad" code anymore.

linaori · 2017-03-06T17:32:55Z

Will this make it possible to do something like <tag name="routing.controller"> and have it registered as service? This would replace the annotation for me:

/**
 * @Route(service="...")
 */
class FooController {}

nicolas-grekas · 2017-03-06T18:22:13Z

@iltar so, this is good to you now? cool :)
About your question, this won't help (#21282 could). Maybe ContainerControllerResolver could do a by-fqcn lookup in the container? Dunno if that'd be a good idea thought. I'll let you give it a try if you want :)

linaori · 2017-03-06T18:23:07Z

@nicolas-grekas I'm not against it and your argument makes sense!

simensen · 2017-03-07T00:05:14Z

I need to look at the actual implementation in more detail, but what is described here sounds pretty close to what I was hoping for in this implementation.

I also like this bit:

<tag name="routing.controller" action="fooAction" argument="logger" id="my_logger"/>

What would I want to do if I wanted to have all actions mapped automatically and also map a specific service? Would it work like this?

<tag name="routing.controller" action="*Action"/>
<tag name="routing.controller" action="fooAction" argument="logger" id="my_logger"/>

... or even:

<tag name="routing.controller" action="fooAction"/>
<tag name="routing.controller" action="fooAction" argument="logger" id="my_logger"/>

How would __invoke be handled? Would it be possible to assume __invoke if action is not specified? For example, if you had a simple controller that only had __invoke, you could do this?

<tag name="routing.controller"/>

All-in-all, I think I like this approach. I know you had some technical reasons why it might not be the best way to go. Were there any things you've done in this PR that you wish you could have done differently?

Tentative 👍 from me for now. :) Super excited about this direction!

nicolas-grekas · 2017-03-07T08:14:22Z

@simensen:

Would it work like this?

<tag name="routing.controller" action="*Action"/>
<tag name="routing.controller" action="fooAction" argument="logger" id="my_logger"/>

Yes! You'd just need one more thing to have the DX you envision: enable autowiring on the definitions that have these tags. The "routing.controller" does not enable autowiring automatically (that'd be coupling), but it plays well with autowired definitions (that's composition).

<tag name="routing.controller" action="fooAction"/>
<tag name="routing.controller" action="fooAction" argument="logger" id="my_logger"/>

That one would be redundant, only the second tag is needed.

Would it be possible to assume __invoke if action is not specified?

As you know, I wondered the same. Thinking about this, it doesn't make much sense for __invoke-style controllers to use the "routing.controller" tag: they could very well use classical constructor injection. That'd be what I'd recommend in fact.
As such, the "action" attribute is required. An exception is thrown if it's missing.

you had some technical reasons why it might not be the best way to go.

They vanished now that we have #21770, which serves as the infrastructure that this PR leverages.

I'm really happy with the PR as is :)

simensen · 2017-03-07T14:38:31Z

@nicolas-grekas:

I'm really happy with the PR as is :)

That makes me happy. :)

I'm a bit lost on this:

enable autowiring on the definitions that have these tags

Does this mean that if autowiring is defined for the controller in question then autowiring is used but if not then it isn't?

... it doesn't make much sense for __invoke-style controllers to use the "routing.controller" tag: they could very well use classical constructor injection. That'd be what I'd recommend in fact.

What about closures? Or is that something we don't need to worry about supporting in core? For example, Silex-style closures:

$app->get('/', function (Request $request, PostRepository $postRepository) {
    // ...
});

This might be well out of scope so I'm not going to worry about this too much. Mostly just curious if this would somehow be covered?

nicolas-grekas · 2017-03-07T14:41:41Z

if autowiring is defined for the controller in question then autowiring is used but if not then it isn't?

yep

What about closures?

All of this is bound to Symfony's DIC (almost all the logic is in a compiler pass), so Silex would need its own wiring system.

nicolas-grekas · 2017-03-09T17:39:44Z

PR updated. Everything now moved to HttpKernel, tag renamed to controller.service_arguments.

When the tag is set, all public methods are now considered for generating a service-locator based argument-resolver.
This means the "action" attribute is now optional. If provided, it doesn't accept wildcards anymore. It must now be accompanied by the "argument" and "id" attributes. Eg same as before:
<tag name="controller.service_arguments" action="fooAction" argument="logger" id="my_logger"/>

All public methods are considered for generating a service-locator: in fact, methods that are listed to be called at instantiation time (ie the one returned by Definition::getMethodCalls()) are excluded of course.

So, we now have two passes:

the first looks like a restricted version of the previous one (no wildcard support, all methods considered as service-locator generator candidates) - it generates the service locators;
the second one runs just before removing passes, and removes any emptied service-locators (because of "ignore-on-invalid" processing), and any service-locators bound to call-at-instantiation methods.

There is a possibility that this may generate some unused service-locators, bound to public methods that are not used as actions. Yet, that should be rare and that'll be just dead code with no practical downside, especially compared to the huge +-side of not having to configure anything.

if you do everything correct, but configure your route to not use the service syntax

this PR provides nothing new on this topic: today also, if you add a bogus argument to an action (eg a bad type hint for a doctrine entity with the proper argument resolver configured), then the controller fails when it is called without earlier notice. There is no chance to make anything better here, at least that's not what this PR is about - entities or services or whatever.

use action: *Action, but then accidentally forget to suffix your method with Action

valid concern, fixed! see description above.

in a perfect world, this argument resolver would work for all controllers

not in my perfect world! I certainly do not want to have services come in when I did not ask for!
Doing so would mean relying on some global mechanism that knows better than me which services should be injected. One may think that, well, just do $container->get(MyTypeHint::class), or $autowirer->getTheServiceThatMatches(LoggerInterface::class). If so, please realize that this is applying a convention that you can't escape from. Symfony has always provided the opposite, via flexible OOP architectures that you can use however you want/need (no hard-coded conventions - and no runtime introspection).

The problem is that there is no decent way to know all of the controllers in the system at compile time.

As stated, that's certainly not a problem to me, quite the contrary.
Keeping things simple means not doing that kind of global things.
Instead, Symfony has always chosen the "be explicit" path.
No exception here.

when the user makes a mistake, we must tell them exactly what went wrong - no wtf moments

I really like that approach, thanks for pushing it to us.
So, on this topic, the code has many checks/exceptions thrown in each possible bad situations.
In situations where things cannot be decided as legitimate or mistake, I added log messages so that the user can inspect what's going on by looking at them.

@weaverryan, I think I addressed all your concerns :)

mvrhov · 2017-03-10T12:38:05Z

Instead of that. Why not promote one Action per class. Then everything works with constructor autowiring which is already in core

nicolas-grekas · 2017-03-10T18:50:17Z

"service" attribute renamed to "id" for consistency with existing practice.
@mvrhov classical actions are still first class citizens.

weaverryan · 2017-03-12T03:33:35Z

Awesome! So this new approach is very interesting - it removes several of the issues I was having earlier!

Before we dive further into the details, the implementation makes me realize something: from a purely technical perspective, this feature could work without the controllers being services. If I told you that I wanted service auto-completion on my AppBundle\Controller\SecurityController actions, then you could create a service locator for all of those arguments. After all, the RegisterControllerArgumentLocatorsPass does not actually rely on the controller being a service... other than this is the way you opt into this feature. But (again, from a purely technical perspective... just stay with me), it would be equally possible to allow the user to opt into this feature without needing to register the controller as a service. For example:

framework:
    controllers_service_arguments:
        AppBundle\Controller: true
        # .. and you could probably have config to control specific args...

I'm not proposing this is a superior syntax! It just strikes me as a bit unnatural that we're requiring the controllers to be services... when we don't really need that! In fact, one of my original comments was this:

if you do everything correct, but configure your route to not use the service syntax, then the feature doesn't work.

You said you didn't do anything to address this... but you did! Thanks to this line in RegisterControllerArgumentLocatorsPass:

if ($id === $class) {
    $controllers[$id.'::'.$r->name] = new Reference($argsId);
}

It is now possible to register a controller as a service via the PSR-4 loader, but then refer to it using the non-service syntax. Yes: as long as your register the controller as a service with the correct tag, you are then allowed to use it NOT as a service, but enjoy the service argument injection. I can imagine this conversation:

Person 1) I can't get Symfony to auto-inject my LoggerInterface argument to my controller
Person 2) Just register the controllers as services, and it'll work

But in reality, Person 1 is still not actually using their controller as a service. This whole situation is what made me realize that the requirement that the controllers be services is unnatural... simply because the feature itself doesn't require it! Even if everyone was creating all controllers as services, I still think the implementation shouldn't rely on this unrelated thing.

So:

I 100% agree that you should be able to opt into / opt out of this feature (and somehow control edge case services - i.e. the action, id, argument options in your implementation)
But, since the implementation doesn't rely on controllers being services, I think the opt-in should be done in a different, but similar way - i.e. by pointing at what classes you want this for. Then, the controller names in the service locator would be keyed almost exactly like now: using the className::methodName syntax (arguments.AppBundle\Controller\SecurityController::loginAction) and also any service ids that match the class name (e.g. arguments.AppBundle\Controller\SecurityController:loginAction or security_controller:loginAction if I created a custom service id).

What are your thoughts about this? And thanks :)

…t services into actions

fabpot · 2017-03-22T22:24:40Z

Thank you @nicolas-grekas.

@simensen

…s" tag to inject services into actions (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | (no test yet) | Fixed tickets | - | License | MIT | Doc PR | - Talking with @simensen and @weaverryan, we wondered if we could leverage the `ArgumentResolver` mechanism to make it inject services on demand, using e.g. autowiring. ```php class PostController { public function indexAction(Request $request, PostRepository $postRepository) { // PostRepository comes from the container $postRepository->findAll(); // ... } } ``` This PR achieves that, using a new "controller.service_arguments" tag. Typically: ```yaml services: AppBundle\Controller\PostController: autowire: true tags: - name: controller.service_arguments ``` It also supports with explicit wiring (thus doesn't necessarily require autowiring if you don't want to use it): ```yaml services: AppBundle\Controller\PostController: tags: - name: controller.service_arguments action: fooAction argument: logger id: my_logger ``` ~~The attached diff is bigger than strictly required for now, until #21770 is merged.~~ Todo: - [x] rebase on top of #21770 when merged - [x] add tests - [x] add cleaning pass to remove empty service locators Commits ------- 9c6e672 [FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions

nicolas-grekas · 2017-03-22T22:26:35Z

Happy the PR is merged, let's play with it now :)

@weaverryan to answer your comment, having left some time to think about it:

this feature could work without the controllers being services. If I told you that I wanted service auto-completion on my AppBundle\Controller\SecurityController actions

The PR just needs a service locator that returns something for any string value of the "_controller" routing param. So yes this can work with, any such "_controller", not only those registered as services.
Yet as you spotted, this service locator needs to be created.

I think it would be possible to implement this "controllers_service_arguments" config key yes. I just don't see the need to fight against having controllers-as-services, thus don't see the need to seek for any other configuration mean.

Person 1) I can't get Symfony to auto-inject my LoggerInterface argument to my controller
Person 2) Just register the controllers as services, and it'll work

I don't see anything wrong with P2's answer. We need a "Px" that will need to answer something to P1, because P1 needs to opt-in. Being via service registration or another mean doesn't matter.
In fact, since we know this will always be possible via services, seeking for a P3 will just create alternatives, thus make things confusing ("should I follow the answer of P2 or P3?!")

I really look forward the RX (reviewer experience) where I don't need do find $this->get('foo') nested in the code anymore, but just have to look at the action signature to know what and how services are going to be used.

fabpot · 2017-03-22T22:40:25Z

Just tested it on a project... and that works! It's deceptively simple and I will activate it in the default configuration for apps using Flex. Can't wait to share that with everyone.

weaverryan · 2017-03-23T11:29:39Z

Docs issue added! symfony/symfony-docs#7672

@container

…ing ControllerTrait (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] Introduce AbstractController, replacing ControllerTrait | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no (master only) | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Basically reverts and replaces #18193. Instead of using getter injection to provide our controller helpers, let's leverage the new `ServiceSubscriberInterface` (see #21708). This is what the proposed `AbstractController` class provides. So, instead of extending `Controller`, this would encourage extending `AbstractController`. This provides almost the same experience, but makes the container private, thus not usable by userland (this safeguard was already provided by `ControllerTrait`). I did not deprecate `Controller`, but I think we should. Now that we also have "controller.service_arguments" (see #21771), we have everything in place to encourage *not* using the container in controllers directly anymore. My target in doing so is removing getter injection, which won't have any use case in core anymore. The wiring for this could be: ```yaml services: _instanceof: Symfony\Bundle\FrameworkBundle\Controller\AbstractController: calls: [ [ setContainer, [ '@container' ] ] ] tags: [ container.service_subscriber ] ```` But this is optional, because we don't really need to inject a scoped service locator: injecting the real container is fine also, since everything is private. And this is done automatically on ControllerResolver. Commits ------- a93f059 [FrameworkBundle] Introduce AbstractController, replacing ControllerTrait

HeahDude · 2017-04-09T10:31:23Z

src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php

+
+    public function process(ContainerBuilder $container)
+    {
+        if (false === $container->hasDefinition($this->resolverServiceId)) {


Why not the other way around? If some tags are found then register the resolver.

Because currently the resolver is asked to resolve before DefaultValueResolver even if there is no controller tagged.

I'd say because "less dynamic" is better. The extra check you're talking about is not a perf issue: it is really fast - and happens once per action (you don't call that many actions for constructing one response, do you?)

and happens once per action

Isn't it once per argument of an action, and sub-action(s)? Although I'm fine with it.

GeoffreyHervet · 2017-05-12T12:29:18Z

In symfony4, will you remove \Symfony\Bundle\FrameworkBundle\Controller\Controller ?

nicolas-grekas added this to the 3.3 milestone Feb 26, 2017

carsonbot added Status: Needs Review FrameworkBundle Feature labels Feb 26, 2017

nicolas-grekas force-pushed the di-controller branch 2 times, most recently from 2554653 to 5e84861 Compare February 26, 2017 22:08

nicolas-grekas force-pushed the di-controller branch 3 times, most recently from 7438115 to d1889dd Compare March 1, 2017 21:16

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Mar 4, 2017

nicolas-grekas force-pushed the di-controller branch 5 times, most recently from 8579093 to d33e043 Compare March 6, 2017 15:11

carsonbot added Status: Needs Review and removed Status: Needs Work labels Mar 6, 2017

nicolas-grekas force-pushed the di-controller branch 2 times, most recently from da56af9 to 15f56e1 Compare March 6, 2017 16:43

nicolas-grekas force-pushed the di-controller branch from 15f56e1 to e2be25e Compare March 7, 2017 07:21

nicolas-grekas changed the title ~~[FrameworkBundle] Add new "routing.controller" tag to inject autowired services into actions~~ [FrameworkBundle] Add new "controller.service_arguments" tag to inject autowired services into actions Mar 9, 2017

nicolas-grekas changed the title ~~[FrameworkBundle] Add new "controller.service_arguments" tag to inject autowired services into actions~~ [FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions Mar 9, 2017

nicolas-grekas force-pushed the di-controller branch 3 times, most recently from d94a2ab to 2b6e005 Compare March 9, 2017 17:02

nicolas-grekas force-pushed the di-controller branch from 2b6e005 to 7beb45c Compare March 9, 2017 17:56

nicolas-grekas force-pushed the di-controller branch from 7beb45c to 0218537 Compare March 10, 2017 18:41

nicolas-grekas force-pushed the di-controller branch from 0218537 to 7a56616 Compare March 16, 2017 15:49

nicolas-grekas removed the Ready label Mar 17, 2017

nicolas-grekas force-pushed the di-controller branch from 7a56616 to 9afcc63 Compare March 17, 2017 21:48

[FrameworkBundle] Add new "controller.service_arguments" tag to injec…

9c6e672

…t services into actions

nicolas-grekas force-pushed the di-controller 8000 branch from 9afcc63 to 9c6e672 Compare March 22, 2017 20:35

fabpot merged commit 9c6e672 into symfony:master Mar 22, 2017

nicolas-grekas deleted the di-controller branch March 22, 2017 22:26

weaverryan mentioned this pull request Mar 23, 2017

[3.3] New controller.service_arguments feature symfony/symfony-docs#7672

Closed

nicolas-grekas mentioned this pull request Mar 25, 2017

[FrameworkBundle] Introduce AbstractController, replacing ControllerTrait #22157

Merged

BPScott mentioned this pull request Mar 28, 2017

[DI] controller.service_arguments does not handle invokable controllers #22202

Closed

HeahDude reviewed Apr 9, 2017

View reviewed changes

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

[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions #21771

[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions #21771

[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions #21771

[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions #21771

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment