Tests will be green when#18206 will be merged.

@dunglas PR#16863 does not deprecate theController class either.

@derrabus right, sorry about that I edit the description.

@iltar I'd like to see the trait to be split as well (see#16863), to make those helpers reusable in non-controller classes like event listeners for instance. If this PR gets merged, I will rebase my PR onto it.

The trait introduces some logic to fetch the depedency from the container if it hasn't been set via a setter. I don't see this logic being covered by the tests.

@derrabus I've added such tests in separate PRs:#18206,#18204,#18203

ping @symfony/deciders

👎 Unless I'm missing something, this would require (if you're autowiring your controller)all of these services to be eagerly instantiated in order to render your controller. Most are common services, but not all of them (form factory, serializer, etc).

I understand why some people might find this useful. I would rather see an approach that:

A) Completely leaves theController alone. I see you made the trait look for a$this->container property as a fallback, but it's just weird - checking for a propertyand also falling back to the container just don't look compatible.

B) Introduce smaller traits that have some of the shortcuts of the controller. And sure, you could wrap those up into one "big" trait for convenience. But even that, I would only include themost common services - likerouter,logger - notserializer,form.factory, etc.

Cheers!

The check for the container in A is basically for BC and ease the maintenance (it allows to use the trait in the abstract controller class).

B is useless. This trait is just a set of proxy method. If you want only some dependencies, just inject services you use in the constructor.

Closing for now.

@dunglas Yea, I know that (A) was for BC - it makes sense, but it's just a little magic, with the trait looking for a property that's named$container.

About (B), it doesn't look useless to me. It brings 2 advantages: (A) RAD (using a trait is even less work than adding a constructor arg, though not aton less work) and (B) familiarity/consistency: people can use the same shortcut methods - likegenerateUrl() - that they use in normal controllers.

For A, it allows us to maintain only one trait, and it works without autowiring with any class having a$container property (the convention in Symfony). IMO it's the best solution on the long term to be compatible both with the legacy and enable new usages. Add a new method to the trait (like thejson one in 3.1), and it will be also available in the traditionalabstract controller. The same apply for bug fixes and so on.

For B, IMO there are 2 kind of users:

1. Newcomers and developers using Symfony as a RAD tool (my case, most of the time): they don't care about performance optimization and will always include theControllerTrait because it's the easiest and quickest to use (and it works exactly like the good oldabstract controller)
2. Advanced developers who care about performance, code quality and maintainability (my case on some projects): they will always carefully choose which dependencies to inject in their controllers and will explicitly inject them in the constructor (autowiring helps here, but traits are useless for them).

For instance, take a look at theRenderHelperTrait created by@derrabus in#16863:https://github.com/derrabus/symfony/blob/master-controller-trait/src/Symfony/Bundle/FrameworkBundle/Templating/RenderHelperTrait.php

It does almost nothing except choosing between Twig and the old templating system. I don't get the point of importing this trait instead of injecting directly an instance of\Twig_Environment in the constructor.

However I do get the interest of using thisControllerTrait because it imports automatically all common dependencies of a controller (thanks to setter autowiring).

To summarize, with this all-in-one trait I need to be aware of its existence and I can create easily controllers with all common dependencies autowired. With separated traits, I must be aware of all the existing traits and this not different than being aware of all dependencies directly (that can be autowired too).

But - I maybe missing something and I would have this merged before the feature freeze.@derrabus and you find a benefit to separated traits. What can we do to make you reconsider your 👎? Do you want I that I split this all-in-one trait in separate traits like in#16863?

From a developer perspective I'm not really sure whether having multiple traits as in#16863 or only having one trait is preferable. Having multiple traits looks preferable to me if your controller/service/action class would only need one trait (for instance to followADR).

In addition, the traits are great for testing because you canchange method visibility. If developers start working with Symfony, the abstract controller probably is one of the first things they will see while navigating through the source code. So in my opinion it should reflect the best practice, is there a specific one related to using traits? 👍

TheController class does not have a single resposibility. It's a collection of glue code, a façade to all kinds of framework functionality. This makes it easy to use for unexperienced developers, but also allows rapid prototyping. It is also a good compromise, if you need to code a small app that will be in use for a couple of weeks/months and scrapped afterwards. Both are problems many agencies have, for instance.

Using theController class is not wrong per se. But I think, we both agree, that you should not use it if high maintainability and framework decoupling are your goals.

Controller is not and never will be a class with a fixed set of dependencies. This is why theController class opts-out of the dependency injection pattern pulls its dependencies on demand. This way, more of this glue code may be added in a later Symfony release with very low impact on existing applications.

I still think that autowiring theController class is a very bad idea. Whether you do this via a trait or directly does not matter to me. Let me explain why.

By eagerly injecting all possible dependencies, you will inevitably inject services that won't be used by the controller.

• Some of these dependencies will already be present (likerouter for instance), so it won't hurt to inject them.
• Others might not be present at all because the corresponding bundle is missing (Security, Monolog, Doctrine) or the specific feature has been disabled (templating,serializer). This will work out as long as the autowiring does not fail on missing dependencies.
• Others need to be instantiated first which might have a performance impact. SeeTemplateListener: Don't pull the templating service from the container if it's not used sensiolabs/SensioFrameworkExtraBundle#369 for such a case. Any later version of the framework might add additional dependencies, so each new framework release might slow down existing controllers even more.

Until now, performance was not an issue with theController class. But by autowiring all kinds of services instead of just the container, you're introducing performance issues. This is why I already said in my PR that I think autowiring won't be helpful for aControllerTrait.

So now this PR does an interesting trick. The trait is theoretically autowirable, but this autowiring is not actually done yet because theController class still receives the container and the trait still has a fallback to the container. So it's a kind-of-hybrid approach. This leaves all existing implementations unaffected by the change. Butif you autowire a class using the trait, you will gain the negative impacts mentioned above with (unless I'm missing something essential) no real benefit.

Now talking about splitting the trait by topic/dependency. Yes, you're right, those traits do almost nothing. And this is exactly the point. Traits should imho only implement glue code that I would otherwise copy&paste. As soon as it contains real logic, class composition should be preferred over a trait.

So what's the benefit of such a small trait then? First of all, reusability for other classes than controllers:

• A trait with templating glue code might not only be helpful in controllers, but also in view listeners like FrameworkExtraBundle'sTemplateViewListener.
• In one of my projects I already use a trait that provides the very handyredirectToRoute() method, so I can use it in some event listeners subscribed to thekernel.request event or in custom authentication modules.

Getting back to controllers, setter autowiring would again start make sense to me on the small traits. I would onlyuse a trait withrouter glue code in a controller class that actually calls methods of that trait. I could then implement a controller without container dependency and just by adding the trait, the router would be magically present. Not sure if I would make use of this, but I kind-of like the idea.

Looks like you missedhttp://symfony.com/blog/new-in-symfony-3-3-getter-injection :)

@nicolas-grekas I did; thanks! 👍

@dunglas Can you also add a note in the CHANGELOG (specifying that this new feature requires PHP 7, and perhaps mentioning the few differences with the Controller base class)? Thanks.

done

👍

Thank you@dunglas.

see#22157 as this was more or less reverted by that PR.