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

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

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
9 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 26, 2017

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 #21770 when merged
  • add tests
  • add cleaning pass to remove empty service locators

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch 2 times, most recently from 2554653 to 5e84861 Feb 26, 2017

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Feb 27, 2017

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 nicolas-grekas force-pushed the nicolas-grekas:di-controller branch 3 times, most recently from 7438115 to d1889dd Feb 27, 2017

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch 5 times, most recently from 8579093 to d33e043 Mar 5, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 6, 2017

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.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch 2 times, most recently from da56af9 to 15f56e1 Mar 6, 2017

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 6, 2017

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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 6, 2017

@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

This comment has been minimized.

Copy link
Contributor

linaori commented Mar 6, 2017

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

@simensen

This comment has been minimized.

Copy link
Contributor

simensen commented Mar 7, 2017

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 nicolas-grekas force-pushed the nicolas-grekas:di-controller branch from 15f56e1 to e2be25e Mar 7, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 7, 2017

@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

This comment has been minimized.

Copy link
Contributor

simensen commented Mar 7, 2017

@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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 7, 2017

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 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 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 nicolas-grekas force-pushed the nicolas-grekas:di-controller branch 3 times, most recently from d94a2ab to 2b6e005 Mar 9, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 9, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch from 2b6e005 to 7beb45c Mar 9, 2017

@mvrhov

This comment has been minimized.

Copy link
Contributor

mvrhov commented Mar 10, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch from 7beb45c to 0218537 Mar 10, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 10, 2017

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

@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Mar 12, 2017

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:

  1. 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)

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch from 0218537 to 7a56616 Mar 16, 2017

@nicolas-grekas nicolas-grekas removed the Ready label Mar 17, 2017

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch from 7a56616 to 9afcc63 Mar 17, 2017

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-controller branch from 9afcc63 to 9c6e672 Mar 22, 2017

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 22, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 9c6e672 into symfony:master Mar 22, 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 Mar 22, 2017

feature #21771 [FrameworkBundle] Add new "controller.service_argument…
…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

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Mar 22, 2017

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.

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-controller branch Mar 22, 2017

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 22, 2017

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

This comment has been minimized.

Copy link
Member

weaverryan commented Mar 23, 2017

Docs issue added! symfony/symfony-docs#7672

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

feature #22157 [FrameworkBundle] Introduce AbstractController, replac…
…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
public function process(ContainerBuilder $container)
{
if (false === $container->hasDefinition($this->resolverServiceId)) {

This comment has been minimized.

@HeahDude

HeahDude Apr 9, 2017

Member

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 10, 2017

Author Member

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

This comment has been minimized.

@HeahDude

HeahDude Apr 22, 2017

Member

and happens once per action

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

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

@GeoffreyHervet

This comment has been minimized.

Copy link

GeoffreyHervet commented May 12, 2017

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

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