[FrameworkBundle] Split abstract Controller class into traits

[derrabus]

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

The Idea

This PR came to my mind during the SymfonyCon hack day. Following the ContainerAware class that has been transformed into a trait, I'd like to do the same with FrameworkBundle's Controller class. As far as I can tell, there is no reason anymore why Controller has to be a class (see this comment on why I would favor a trait over an abstract class here).

The current Controller class covers many different topics. It contains utility methods for security, rendering, routing and more. Before traits, it was not possible to separate those topics. Now, we could split the class into multiple focused traits. This would increase reusability and enable usage outside of a controller context, for instance inside an event listener.

Like it is currently done in the Controller class, the traits won't contain real functionality. Instead, they will provide shortcut functions to ease the integration of various Symfony components (for instance Routing and HttpFoundation in the examples below).

Examples

The following example shows an event listener using the redirectToRoute method that can be found in the Controller class.

class MyListener implements EventSubscriberInterface, ContainerAwareInterface
{
    use ContainerAwareTrait;
    use RouterHelperTrait;

    public static function getSubscribedEvents()
    {
        return [
            KernelEvents::REQUEST => 'onKernelRequest'
        ];
    }

    public function onKernelRequest(GetResponseEvent $event)
    {
        // Some code to decide if we want to do a redirect

        $event->setResponse(
            $this->redirectToRoute('acme.my_route')
        );
    }
}

In this example, we use the method inside a controller that is registered as a service and therefore should not depend on the container.

class MyController
{
    use RouterHelperTrait;

    public function __construct(UrlGeneratorInterface $router)
    {
        $this->router = $router;
    }

    public function someAction(Request $request)
    {
        // Some code to decide if we want to do a redirect

        return $this->redirectToRoute('acme.my_route');
    }
}

BC Breaks

The traits will introduce protected fields and methods to manage their dependencies. If a class extending Controller already contains methods and properties with the same name, this change might introduce conflicts.

[Koc]

Alternative approach http://www.whitewashing.de/2013/06/27/extending_symfony2__controller_utilities.html

[Tobion]

Making the controller a trait is what I suggested in #12595 (comment)

But maybe we should also split the traits by topic like RouterTrait as Silex is doing it.

[dunglas]

There is another approach I want to discuss since several time but I did not have the time to blog about it.

It's a derivate of/similar to the ADR pattern, applied to Symfony.

1. An action is single class with a __invoke() method containing all it's logic (should be only some lines of glue code to call the domain and the responder).
2. Actions are registered as a service (it can be automated using a compiler pass and some conventions, similar to how we discover controllers right now, with the ability to override the definition if needed)
3. On such action services, autowiring is enabled. It means that almost as easy as current controllers to use for the developper, but with an explicit dependency graph and a better reusability. Only things that are really necessary are injected.

In fact, it's already doable to use such system with Symfony. We do it in API Platform master. Here is one action for instance: https://github.com/dunglas/DunglasApiBundle/blob/master/Action/PostCollectionAction.php

In this implementation I also rely a lof on kernel events to be able to plug and play some sort of logic (persistence, validation...) in a decoupled manner but it's out of scope.

[derrabus]

@Tobion Splitting the trait is what I've suggested as a next step in my initial post. It absolutely makes sense to set the good old Controller class on a diet.

@dunglas I like your idea, but I wouldn't see it as a direct alternative to this approach. Maybe, we should discuss it in a different ticket. The idea behind the Controller class is imho to enable rapid development and a fast ramp-up for new/unexperienced developers. If clean sparation of concerns and high maintainability is your goal, controllers as services or maybe your approach would be the way to go. But if we split the ControllerTrait, the resulting smaller blocks could be interesting for a controller-as-a-services approach as well.

[sstok]

@dunglas I really like the idea of having a single action per class, but registering them as services will immensely increase the container size and services list. Or this is no real problem?

[dunglas]

@sstok It needs to be benchmarked but even if you have 200 controllers in your app I'm not sure it will make a big difference from a performance point of view.

[javiereguiluz]

In my opinion, this proposal hurts DX (developer experience) very seriously for Symfony newcomers. Implementing the interface and using a trait looks confusing and splitting the trait into "microtraits" looks overkill to me.

[derrabus]

@javiereguiluz I take this concern very serious, because DX (especially for unexperienced developers) is imho the most important reason, why we have that Controller base class.

First of all, splitting the trait would not hurt DX here, since we would still have the ControllerTrait that aggregates the smaller traits.

At the moment, we still need the ContainerAwareInterface interface because ControllerResolver uses it as a marker to decide if the container needs to be injected into the instantiated controller, see: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php#L83-L85

A trait cannot add an interface to a class, so we need to add it manually at the moment. Maybe we can find a better way than relying on the interface to autowire the container to the controller?

[GuilhemN]

Why not keeping the Controller class until we find a way to detect when the container must be injected in a controller using the trait?
This would let newcomers have an easy way to create controllers and when they'll discover symfony deeply, they'll be able to use the ControllerTrait instead.

[derrabus]

@Ener-Getick My intention was not to create too many ways of doing the same thing.

But maybe both, the Controller class and the trait have their use cases. Undeprecating the class would be fine for me. The class could still consume the trait, though.

[DavidBadura]

👍 for both way

[GuilhemN]

And what do you think about renaming the trait to something which would assume there are sub traits like FullControllerTrait?
IMO it would be easier then to create specific traits.

[Pierstoval]

I think this PR is a very good idea, turning many things into traits is good because I see many newcomers in PHP and almost all of them don't know anything about traits (they're more focused on the "awesome short-array syntax like in JS" than the awesome features like traits, variadic functions or generators).

It sounds very DX to me so I agree 👍

@dunglas You made me discover the ADR pattern and even if it looks very good (and much better than MVC to me), I don't know if we could use it for Symfony 3. Maybe for Symfony 4 actually, but it's not for now (and has to be discussed by the core team). Still,

@derrabus In your PR you should change this :

Deprecations? no

Because it's a deprecation actually :)

[derrabus]

@Pierstoval Done, thanks. :-)

[kozlice]

I'm sceptical on this.

Traits are mixins, which are supposed to provide implementation of additional interfaces, e.g. ~AwareInterface. They are not a replacement of inheritance, and Controller, IMO, must remain an abstract class.

[stof]

I don't know if we could use it for Symfony 3

@Pierstoval You can already use it even in Symfony 2 (I think it is even possible in the 2.0.0 release):

• create a controller class containing a single action (using __invoke for the action is possible in recent versions of Symfony but not in older versions, but it can be another name)
• return a value object from this action (anything which is not null or a Response)
• register a listener on the kernel.view event to handle the conversion of the return value into a response

Btw, this listener is the way SensioFrameworkExtraBundle and FOSRestBundle are already handling this (the View class of FOSRestBundle is similar to the concept defined in ADR)

[Pierstoval]

@stof Thanks a lot for the enlightenment!

[tommiv]

What are the advantages of this approach?

As far as I can tell, there is no reason anymore why Controller has to be a class.

Actually, I don't see any reason why it ​_shouldn't_​ be a class. Could someone explain why trait is a better choice for this kind of functionality?

[GuilhemN]

@tommiv a class can have multiple traits but only one parent. So a controller can extend Controller or another class but not both.
If Controller becomes a trait it can do both.

[tommiv]

@Ener-Getick is this really necessary? Yes, if you inherit Controller you can't inherit another class (however you can inherit Controller in base class and extend it then). But you still can use as many traits as you want, until you fall into some problem related to multiple inheritance. But in this case you don't need to deprecate anything and do breaking API changes.

[GuilhemN]

@tommiv I'm in favor of having a class (implementing the ContainerAwareInterface) and a trait.
Both have their use cases. We can even imagine a trait designed for setter injections and a class designed to be easier for newcomers (with the service container).

[derrabus]

Because the question was raised why we should favor traits over an abstract base class:

The Controller class does not define a type. It even does not define a public interface (apart from the container-awareness which is not a functionality specific to the controller). It does not contain any real functionality either. It is more or less a collection of glue code snippets making some components easier to use. In fact, you do not have to extend Controller to build a controller (read this or @dunglas' comment), neither does a class that extends Controller have to be used as a controller. After all, it's just glue code and that render() method for instance might be handy inside an event listener as well (especially if you make use of the widely underestimated kernel.view event).

Using type inheritance for code reuse is bad in gerneral. It violates the Liskov substitution principle and in languages with single inheritance, it locks you into a fixed inheritance chain. If you have more than one piece of code that you want to reuse, you will create either a huge Adam class for everything or deep and complicated inheritance trees. The alternative is class composition, but this strategy creates the need to copy&paste boilerplates of glue code like the code we find inside the Controller class.

Before traits, php did not have a way to avoid duplication of glue code other than inhertitance. With traits being available, we have a way more lightweight mechanism of distributing glue code. And we can build smaller reusable packages instead of one big class. The removal of the ContainerAware class in favor of a ContainerAwareTrait is a good example for this.

Why do we have that Controller class at all if there's no need to extend it?

It is helpful for rapid prototyping. If I don't care much about the quality of the code because I'm going to throw it away anyway, this class enables me to get the job done very quickly.

As @javiereguiluz noted, the class lowers the entrance barrier for beginners. Someone who has never seen a Symfony application could easily start to build one. Just extend the Controller class and you're ready to go. You don't need to learn Symfony's component architecture for this. And you don't need to learn what dependency injection is before you write your first Symfony code. It probably won't be clean or maintainable code, but it's a good start to learn the framework.

And of course, you don't need to know what a trait is and what that ContainerAwareInterfacedoes to get started. This is why I now consider to remove the deprecation of the class.

[tommiv]

@derrabus Thanks for the explanation. It really makes sense now.

[derrabus]

This is what I'd like to do to go on with this PR:

• Resolve the conflicts with the master branch.
• Undeprecate the Controller class.
• Split the trait into smaller reusable parts.

If, after splitting it, we still need a ControllerTrait or if it's enough to have a Controller class that aggregates all the small traits, is still something we could discuss, I guess.

[kozlice]

@derrabus I see your point. It makes sense, but I think we should go in a little different way:

• Split Controller into several traits (as @Tobion suggested), grouping methods by topic (security-related, response-, form-, router-, etc.), e.g. SecurityHelperTrait, FormHelperTrait. They might be used not only inside controller: I've written a plenty of services which would be simpler if I had those shortcut methods.
• Keep the default Controller and make it use all of these new traits. @javiereguiluz made a good point about keeping things simple for newcomers, and you've made a good point too about rapid prototyping. In addition to that, there are simple projects, where you don't need to (and don't want to) bother yourself with complicated architecture and performance. Caring about DX is one of the things I like about Symfony, so the simple way of just extending the framework's default Controller (and getting all of the default helpers in your class) should be preserved.
• Maybe introduce marker-/tag-like ControllerInterface. We can't define methods since controllers are designed to serve multiple routes, but you might still want to check if class's role is controller (without using reflection).

[Pierstoval]

@kozlice I really like the good idea of splitting traits, but when not extending the default controller, it would "force" the user to implement/use a ContainerAwareInterface class/trait/interface to be sure each helper is available. How would you do that in a trait?

[derrabus]

@kozlice I don't disagree with you about the first and second bullet point of your list.

But I don't see the point in introducing a marker interface. Right now, you can use any plain php class as a controller. Changing that would be a BC break without any benefit.

[TomasVotruba]

What needs to be done here to move this forward? I can help if you need.

[derrabus]

@TomasVotruba I can always rebase the PR against the current master, if there is interest in this feature. Continuously keeping it mergable is painful because the Controller class, which is replaced in this PR, is changed often.

But right now, I don't feel like the core team is supporting my idea.

[TomasVotruba]

@derrabus Thanks for update. I have the same feelings. What a pity.
I came across this issue in almost every company as it empowers bad coding habits: "If Symfony has such a big a controller, why could not we? I think it's good for us".

Maybe writing article about this issue could spread the idea.

[ro0NL]

So what about ControllerUtils as a service (in favor of Controller as a base class or trait)? I'd like that.

As right now, we have tens of controllers injecting the same services over and over again (plus additional deps). Making me seriously considering switching back to the controller base class, only for convenience. Or adding the utils class it in user-land.

[HeahDude]

As right now, we have tens of controllers injecting the same services over and over again (plus additional deps)

@ro0NL I like to have my own BaseController class (often for each sub namespace), but this is a matter of personal preferences.

So what about ControllerUtils as a service

It does not make sense to me to have one service that get injected all the container to implements some services shortcuts.

We "tolerate" it from the controller because it ease starting to develop Symfony application, a simple app can be just a config file and some controller actions, in such a case it's "convenient" to get any service you need from there being "container aware".

[ro0NL]

It does not make sense to me to have one service that get injected all the container to implements some services shortcuts.

Not my approach :)

class ControllerUtils { 
   public function __construct(UrlGeneratorInterface $urlGenerator/**, etc. */) { }
   public function redirectToRoute() { }
   // etc.
}

$this->controllerUtils->redirectToRoute() vs. $this->redirectToRoute().

The service definition is provided by the framework (compare it to the AuthenticationUtils class).

[Pierstoval]

Actually, we should not put the different parameters in the constructor, but in calls options instead.

This could be good also to have a specific controller service tag when we create our controllers as services, because tags are compiled once in the container and can allow us to take benefit for tons of cool things, whether they could be in traits or abstract classes.
And using tags allows to use the constructor with custom arguments too

[ro0NL]

Not sure i follow.. but i propose a very low-level solution

use SF\ControllerUtils;
use My\Dep;

class MyController {
   public function __construct(Dep $dep, ControllerUtils $controllerUtils) {}
   // profit
}

services:
   my_controller:
      class: MyController
      arguments: ['@my_dep', '@controller.utils']

[HeahDude]

This could be a simple abstract definition.

services:
    my_controller:
        class: MyController
        parent: controller.utils
        calls:
            - ['setMyDep', ['@my_dep']]

[Pierstoval]

This could be a simple abstract definition.

services:
    my_controller:
        class: MyController
        parent: controller.utils
        calls:
            - ['setMyDep', ['@my_dep']]

This is precisely what I was thinking about, and using calls is much better in this case, because arguments are appended to constructor instead of replaced, which is not very nice for readability & debugging (and also hard for understanding...)

[HeahDude]

Yeah but the down side is that makes classes definition mutable... This kind of setters should return the old value IMO, to be consistent and able to retrieve their original state, like handlers do.

[ro0NL]

I favor a solution that doesnt require setter-injection, nor a base class, nor a trait.

[TomasVotruba]

I would like to try this feature in practice before merging, since it is great change that may go wrong.

So I've implemented these traits for Controllers in bundle:

• http://www.tomasvotruba.cz/blog/2016/12/12/decouple-controller-from-symfony-using-smart-traits/
• https://github.com/Symplify/ControllerAutowire

Feel free to try it and write your experience and what was good and bad.

It might help to discuss this PR in more real-world way.

[nicolas-grekas]

Personally, I like the ControllerTrait, but I'm skeptical about the smaller traits.
Since #18193 is more advanced on the ControllerTrait topic (works with setter autowiring + made the container injection optional), I think we can postpone this one until #18193 is merged. Maybe close it also if the only things remaining (smaller traits) are controversial (closing would be my pref for now, reopening/resubmitting later if the need still exists for someone.)

[TomasVotruba]

Have you tried it in practice? What are the results?

[fabpot]

Closing in favor of #18193