[FrameworkBundle] Introduce autowirable ControllerTrait #18193

dunglas · 2016-03-16T13:39:15Z

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 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 form the symfony-demo can now looks like:

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.

linaori · 2016-03-16T13:52:27Z

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTraitTest.php

+        createForm as public;
+        createFormBuilder as public;
+        getDoctrine as public;
+        isCsrfTokenValid as public;


Would it be an idea (while at it), to split them?

ControllerSessionTrait

ControllerSecurityTrait

ControllerResponseTrait

ControllerFormTrait

ControllerRouterTrait

ControllerDoctrineTrait

Missed one?

As explained in the description, I think this is useless. They are proxy methods to the service itself. If you don't want the convenience of this "all-in-one" trait, just inject the service and use it.

HeahDude · 2016-03-17T06:30:13Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php

+     *
+     * @return Response A Response instance
+     */
+    protected function forward($controller, array $path = array(), array $query = array())


So I'll need to rebase #17589, when this one gets merged.

dunglas · 2016-03-17T09:14:41Z

Tests will be green when #18206 will be merged.

derrabus · 2016-03-17T10:39:17Z

@dunglas PR #16863 does not deprecate the Controller class either.

dunglas · 2016-03-17T10:42:55Z

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

derrabus · 2016-03-17T10:44:03Z

@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.

derrabus · 2016-03-17T10:47:17Z

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerTraitTest.php

+    public function testRedirect()
+    {
+        $controller = new TestTrait();
+        $response = $controller->redirect('http://dunglas.fr', 301);


I'd prefer neutral URLs in test cases, like http://example.com for instance.

I agree. We should make this a requirement. And we could normalize person names too. Although they are tests and not "real code", it would be great to be consistent.

Why? It's just tests (never displayed to the end user), they are already a lot of things like that in the code base and it's fun to see some "Bernhard" or "Fabien" things in test. It "humanizes" the project, I prefer that to all those "Acme Inc" and "example.com" BS.

https://twitter.com/search?q=vairelles%20symfony This what I mean by "humanize".

example.com and example.org are reserved for documentation and examples: http://www.iana.org/domains/reserved They are a global standard. They are guaranteed to work forever and it will always be safe to use them. No other domain (not even symfony.com itself) can guarantee that.

It doesn't matter here. The URL is never accessed. Even if the domain doesn't exist the test will still be green.

Even if it does not matter, using example.com for better consistency would be good.

derrabus · 2016-03-17T11:00:24Z

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.

dunglas · 2016-03-17T11:14:15Z

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

…s (dunglas) This PR was merged into the 2.3 branch. Discussion ---------- [FrameworkBundle][2.3] Add tests for the Controller class | Q | A | ------------- | --- | Branch? | 2.3 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Backport tests of #18193 to the `abstract` Controller. Commits ------- ca56be1 [FrameworkBundle] Add tests for the Controller class

…s (dunglas) This PR was merged into the 2.7 branch. Discussion ---------- [FrameworkBundle][2.7] Add tests for the Controller class | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Backport tests of #18193 to the `abstract` Controller. Commits ------- 514a060 [FrameworkBundle] Add tests for the Controller class

…s (dunglas) This PR was squashed before being merged into the 2.8 branch (closes #18206). Discussion ---------- [FrameworkBundle][2.8] Add tests for the Controller class | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Backport tests of #18193 to the `abstract` Controller. Commits ------- 5ee9f93 [FrameworkBundle][2.8] Add tests for the Controller class

dunglas · 2016-03-27T16:31:03Z

ping @symfony/deciders

weaverryan · 2016-03-29T13:40:40Z

👎 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 the Controller alone. I see you made the trait look for a $this->container property as a fallback, but it's just weird - checking for a property and 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 the most common services - like router, logger - not serializer, form.factory, etc.

Cheers!

dunglas · 2016-03-29T14:12:02Z

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.

weaverryan · 2016-03-29T21:47:20Z

@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 a ton less work) and (B) familiarity/consistency: people can use the same shortcut methods - like generateUrl() - that they use in normal controllers.

dunglas · 2016-03-30T06:45:45Z

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 the json one in 3.1), and it will be also available in the traditional abstract controller. The same apply for bug fixes and so on.

For B, IMO there are 2 kind of users:

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 the ControllerTrait because it's the easiest and quickest to use (and it works exactly like the good old abstract controller)
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 the RenderHelperTrait 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 this ControllerTrait 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?

rvanlaak · 2016-03-30T08:54:10Z

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 follow ADR).

In addition, the traits are great for testing because you can change 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? 👍

derrabus · 2016-03-30T14:42:25Z

The Controller 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 the Controller 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 the Controller 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 the Controller 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 (like router 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. See TemplateListener: 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 the Controller 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 a ControllerTrait.

So now this PR does an interesting trick. The trait is theoretically autowirable, but this autowiring is not actually done yet because the Controller 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. But if 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's TemplateViewListener.
In one of my projects I already use a trait that provides the very handy redirectToRoute() method, so I can use it in some event listeners subscribed to the kernel.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 only use a trait with router 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.

nicolas-grekas · 2017-02-27T06:53:05Z

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

robfrawley · 2017-02-27T06:54:13Z

@nicolas-grekas I did; thanks! 👍

fabpot · 2017-03-02T19:30:06Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php

+     */
+    protected function getRouter(): RouterInterface
+    {
+        throw new \LogicException(sprintf('An instance of "%s" must be provided.', RouterInterface::class));


I would remove the exceptions here as PHP would throw an error anyway thanks to the type hint.

Done.

I've also removed the conditional behavior of the json() method to make the Serializer mandatory:

it's a cleaner design (the behavior is predictable)

it allows to not catch a TypeError

having a proxy method doing return new JsonResponse($data, $status, $headers) is useless

fabpot · 2017-03-02T19:49:40Z

@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.

dunglas · 2017-03-02T20:09:16Z

done

fabpot · 2017-03-02T20:10:19Z

👍

nicolas-grekas

👍

fabpot · 2017-03-02T20:20:31Z

Thank you @dunglas.

@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

fabpot · 2017-03-25T19:43:02Z

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

@dunglas

…hemN) This PR was squashed before being merged into the master branch (closes #7657). Discussion ---------- [FrameworkBundle] Document the AbstractController Document symfony/symfony#18193. I'm not sure it should be merged right now though as it's an experimental feature. \cc @dunglas Commits ------- 4ac5da7 Fix 88a4806 Fix cf2ae91 [FrameworkBundle] Document the AbstractController

dunglas added the FrameworkBundle label Mar 16, 2016

carsonbot added the Status: Needs Review label Mar 16, 2016

linaori reviewed Mar 16, 2016
View reviewed changes

javiereguiluz added the Feature label Mar 16, 2016

This was referenced Mar 16, 2016

[FrameworkBundle][2.3] Add tests for the Controller class #18203

Merged

[FrameworkBundle][2.7] Add tests for the Controller class #18204

Merged

[FrameworkBundle][2.8] Add tests for the Controller class #18206

Closed

HeahDude reviewed Mar 17, 2016
View reviewed changes

derrabus reviewed Mar 17, 2016
View reviewed changes

dunglas mentioned this pull request Mar 20, 2016

[WIP] Support for controller autoregistration and autowiring symfony/symfony-standard#959

Closed

4 tasks

dunglas closed this Mar 29, 2016

dunglas mentioned this pull request Mar 29, 2016

[DependencyInjection] Autowiring: add setter injection support #17608

Merged

dunglas reopened this Mar 30, 2016

dunglas added 4 commits March 1, 2017 22:16

[FrameworkBundle] Introduce autowirable ControllerTrait

8e65425

Make ControllerTrait compatible with getter injection

ef6d676

Mark experimental and remove Templating support

ef99840

Fix tests

f1cb9fa

dunglas force-pushed the autowirable_controller branch from e5fbe6d to 01b183a Compare March 1, 2017 21:31

Add annotation

dcc7e09

dunglas force-pushed the autowirable_controller branch from 01b183a to dcc7e09 Compare March 1, 2017 22:00

fabpot reviewed Mar 2, 2017

View reviewed changes

Remove exceptions and the conditional behavior of json()

f2e54f6

Add author

662eac4

dunglas force-pushed the autowirable_controller branch from 4439218 to 2c69e1e Compare March 2, 2017 20:10

Add a mention in CHANGELOG

8972503

dunglas force-pushed the autowirable_controller branch from 2c69e1e to 8972503 Compare March 2, 2017 20:12

nicolas-grekas approved these changes Mar 2, 2017

View reviewed changes

fabpot closed this in 50b9126 Mar 2, 2017

nicolas-grekas mentioned this pull request Mar 6, 2017

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

Merged

3 tasks

GuilhemN mentioned this pull request Mar 19, 2017

[FrameworkBundle] Document the AbstractController symfony/symfony-docs#7657

Merged

This was referenced Mar 20, 2017

Create traits for common tasks in controller #22061

Closed

[FrameworkBundle] Introduce AbstractController, replacing ControllerTrait #22157

Merged

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] Introduce autowirable ControllerTrait #18193

[FrameworkBundle] Introduce autowirable ControllerTrait #18193

dunglas commented Mar 16, 2016 •

edited

Loading

linaori Mar 16, 2016

dunglas Mar 16, 2016

HeahDude Mar 17, 2016

dunglas commented Mar 17, 2016

derrabus commented Mar 17, 2016

dunglas commented Mar 17, 2016

derrabus commented Mar 17, 2016

derrabus Mar 17, 2016

javiereguiluz Mar 17, 2016

dunglas Mar 17, 2016

dunglas Mar 17, 2016

javiereguiluz Mar 17, 2016

dunglas Mar 17, 2016

fabpot Mar 27, 2016

derrabus commented Mar 17, 2016

dunglas commented Mar 17, 2016

dunglas commented Mar 27, 2016

weaverryan commented Mar 29, 2016

dunglas commented Mar 29, 2016

weaverryan commented Mar 29, 2016

dunglas commented Mar 30, 2016

rvanlaak commented Mar 30, 2016

derrabus commented Mar 30, 2016

nicolas-grekas commented Feb 27, 2017

robfrawley commented Feb 27, 2017

fabpot Mar 2, 2017

dunglas Mar 2, 2017

fabpot commented Mar 2, 2017

dunglas commented Mar 2, 2017

fabpot commented Mar 2, 2017

nicolas-grekas left a comment

fabpot commented Mar 2, 2017

fabpot commented Mar 25, 2017

[FrameworkBundle] Introduce autowirable ControllerTrait #18193

[FrameworkBundle] Introduce autowirable ControllerTrait #18193

Conversation

dunglas commented Mar 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Mar 17, 2016

derrabus commented Mar 17, 2016

dunglas commented Mar 17, 2016

derrabus commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Mar 17, 2016

dunglas commented Mar 17, 2016

dunglas commented Mar 27, 2016

weaverryan commented Mar 29, 2016

dunglas commented Mar 29, 2016

weaverryan commented Mar 29, 2016

dunglas commented Mar 30, 2016

rvanlaak commented Mar 30, 2016

derrabus commented Mar 30, 2016

nicolas-grekas commented Feb 27, 2017

robfrawley commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 2, 2017

dunglas commented Mar 2, 2017

fabpot commented Mar 2, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Mar 2, 2017

fabpot commented Mar 25, 2017

dunglas commented Mar 16, 2016 •

edited

Loading