Skip to content

[FrameworkBundle] Add ControllerHelper; the helpers from AbstractController as a standalone service #60857

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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jun 20, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues -
License MIT

This PR is a follow up of #16863 by @derrabus almost 10 years ago 😱, which was seeking for a solution to reuse helpers provided by AbstractController that'd be free from coupling by inheritance and that'd allow for more granular cherry-picking of the desired helpers.

At the time, granular traits were proposed as the reusability unit.

In this PR, I'm proposing to add a ControllerHelper class that'd allow achieving both goals using functions as the reusability unit instead.

To achieve true decoupling and granular helper injection, one would have to use the #[AutowireMethodOf] attribute (see #54016).

Here is the chain of thoughts and concepts that underpin the proposal. It should be noted that this reasoning should be read as an example that could be extended to any helper-like class, e.g it fits perfectly for cherry-picking query functions from entity repositories.

So, here is the chain for controllers:

  1. The Problem: The Monolithic Base Class

    Symfony's AbstractController offers a convenient set of helper methods for common controller tasks. However, by relying on inheritance, our controllers become tightly coupled to the framework. This can make them more difficult to test in isolation and provides them with a broad set of methods, even when only a few are needed.

  2. The Initial Goal: Reusability without Inheritance

    The long-standing goal has been to decouple controllers from this base class while retaining easy access to its valuable helper methods. The ideal solution would allow for a more granular, "à la carte" selection of these helpers.

  3. The Path Not Taken: Granular Traits

    The original proposal in PR [FrameworkBundle] Split abstract Controller class into traits #16863 explored the use of granular traits (e.g., RenderTrait, RedirectTrait). This was a step towards greater modularity, allowing a developer to use only the necessary functionalities. However, a trait-based approach has its own set of challenges:

    • Implicit Dependencies: The services required by the traits (like the templating engine or the router) are not explicitly declared as dependencies of the controller.
    • A Different Form of Coupling: While avoiding vertical inheritance, traits introduce a form of horizontal coupling.
  4. A More Modern Approach: The Injectable Helper Service

    This PR introduces a ControllerHelper service. This class extends AbstractController to leverage its existing, battle-tested logic but exposes all of its protected methods as public ones. This aligns with modern dependency injection principles, where services are explicitly injected rather than inherited. A controller can now inject the ControllerHelper and access the helper methods through it.

  5. The Final Step: True Granularity with #[AutowireMethodOf]

    While injecting the entire ControllerHelper is a significant improvement, it still provides the controller with access to all helper methods. The introduction of the #[AutowireMethodOf] attribute (see [DependencyInjection] Add #[AutowireMethodOf] attribute to autowire a method of a service as a callable #54016) is the final piece of the puzzle, enabling the ultimate goal of using individual functions as the unit of reuse.

    With #[AutowireMethodOf], we can inject just the specific helper method we need as a callable:

    class MyController
    {
        public function __construct(
            #[AutowireMethodOf(ControllerHelper::class)]
            private \Closure $render,
            #[AutowireMethodOf(ControllerHelper::class)]
            private \Closure $redirectToRoute,
        ) {
        }
    
        public function showProduct(int $id): Response
        {
            if (!$id) {
                return ($this->redirectToRoute)('product_list');
            }
    
            return ($this->render)('product/show.html.twig', ['product_id' => $id]);
        }
    }

    This solution provides numerous benefits:

    • Maximum Decoupling: The controller has no direct dependency on AbstractController or even the ControllerHelper class in its methods. It only depends on the injected callables.
    • Explicit and Granular Dependencies: The controller's constructor clearly and precisely declares the exact functions it needs to operate.
    • Improved Testability (less relevant for controllers but quite nice for dependents of e.g. entity repositories): Mocking the injected callables in unit tests is straightforward and clean.
  6. Bonus: Auto-generated Adapters for Functional Interfaces

    For even better type-safety and application-level contracts, #[AutowireMethodOf] can generate adapters for functional interfaces. One can define an interface within their application's domain to achieve better type-coverage without any new coupling:

    interface RenderInterface
    {
        public function __invoke(string $view, array $parameters = [], ?Response $response = null): Response;
    }

    Then update the previous controller example to use this interface:

             #[AutowireMethodOf(ControllerHelper::class)]
    -        private \Closure $render,
    +        private RenderInterface $render,

    Symfony's DI container will automatically create an adapter that implements RenderInterface and whose __invoke method calls the render method of the ControllerHelper. This gives full static analysis and autocompletion benefits with zero extra boilerplate code.

This pull request, therefore, not only provides a solution to a long-standing desire for more reusable controller logic but does so in a way that is modern, flexible, and fully embraces the power of Symfony's dependency injection container (while still preserving really good usability when the DIC is not used, as is the case when unit testing.)

@nicolas-grekas nicolas-grekas force-pushed the fwb-controller-helper branch from 7226057 to 53e9574 Compare June 20, 2025 15:49
@nicolas-grekas nicolas-grekas force-pushed the fwb-controller-helper branch from 53e9574 to 4bcbaab Compare June 20, 2025 15:55
@GromNaN
Copy link
Member

GromNaN commented Jun 20, 2025

One can define an interface within their application's domain to achieve better type-coverage without any new coupling.

Since the use-case is well-known, this interfaces should be provided by the FrameworkBundle itself. Otherwise the DX is broken as you don't have the signature of the closure for auto-completion and static analysis.

Continuing in this logic, having an invokable class for each helper would be more transparent: unique type for DI of each helper and method signature to call them. Less magic with the AutowireMethodOf attribute.

namespace Symfony\Bundle\FrameworkBundle\Controller\Helper;

class Render
{
    // Explicit dependency injection
    public function __construct(private Environment $twig) {} 

    public function __invoke(string $view, array $parameters = [], ?Response $response = null): Response
    {
        // ...
    }
}

So there is no need to use the attribute.

use Symfony\Bundle\FrameworkBundle\Controller\Helper\Render;
use Symfony\Bundle\FrameworkBundle\Controller\Helper\RedirectToRoute;

class MyController
{
    public function __construct(
        private Render $render,
        private RedirectToRoute $redirectToRoute,
    ) {
    }

    // Same function showProduct
}

@chalasr
Copy link
Member

chalasr commented Jun 20, 2025

I agree with Jérôme. I wouldn't want to see code using ControllerHelper elsewhere than in a Controller, even though it's only about pointing #[AutowireMethodOf] it feels just wrong conceptually.
So I'd either add as many high-level helpers as AbstractController methods in their respective components as suggested or keep things as they are today.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jun 20, 2025

@GromNaN thanks for the comments. This proposal is about providing innovative ways to decouple from helper classes, with AbstractController and entity repositories as primary use cases.

Personally, I don't think maintaining a myriad of helper classes will improve anything: from a maintainer's perspective, it's an explosion of class, and from a DX perspective, it's a discoverability nightmare. Having one compact class with helper methods has been the natural solution for these reasons. This PR doesn't try to disrupt this practice - that's its main point and benefit: providing a solution to granularity without forcing us to use patterns that'd lead to even more classes/files to jungle with.

@nicolas-grekas
Copy link
Member Author

(Ignoring the likely too high migration costs, I think AbstractController could be favorably deprecated in favor of ControllerHelper, regardless of considerations related to AutowireMethodOf.)

@GromNaN
Copy link
Member

GromNaN commented Jun 20, 2025

If all the helper classes are in the same namespace, there is no discoverability issue.

The solution you propose is a less practical than the current AbstractController:

  • You don't know the method signature, and you don't have any easy access to the method (until IDE/tools add support for inspection of AutowireMethodOf). It feels like magic __call.
  • You have to repeat #[AutowireMethodOf(ControllerHelper::class)] private \Closure $<method> for every helper you want to use.
  • The PHP syntax for calling a closure from a property is unusual ($this->redirectToRoute)('product_list').

The advantage of the closure is for mocking, but as soon as you add invokable interfaces you lose this advantage.

@nicolas-grekas
Copy link
Member Author

If all the helper classes are in the same namespace, there is no discoverability issue.

Typing $foo-> and seeing the possibilities is way more DX-friendly than having to see sibling classes in a folder/namespace.

The solution you propose is a less practical than the current AbstractController: [...]

I 💯 agree with you, those drawbacks are real (except the use of the "magic" adjective to highlight that you don't like it - any attributes can be described as such by nature of declarativeness - until the behavior is accepted, so that's not really helpful...).

Yet, all these drawbacks apply to AutowireMethodOf.
And the proposal is about ControllerHelper ;)

What I wrote in the PR description needs some visionary thinking: from an architectural PoV, the design that's allowed by AutowireMethodOf is far superior. The good thing about the drawbacks is that they all can be removed by improving either the tooling or the syntax. We're here to build the future - we're not bound by the current state of the art.

(If you don't share the vision completely, then you can focus on the meat of the proposal: ControllerHelper is already an improvement over AbstractController, eg it can be used with invokable controllers.)

The advantage of the closure is for mocking, but as soon as you add invokable interfaces you lose this advantage.

You don't, anonymous classes to the rescue.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, the new ControllerHelper service, injectable into controllers is an improvement for decoupling from the AbstractController inheritance. And you get all the helpers in a single service.

I hope tooling will improve to detect the signature of the injected closure with the attribute.

@derrabus
Copy link
Member

For even better type-safety and application-level contracts, #[AutowireMethodOf] can generate adapters for functional interfaces. One can define an interface within their application's domain to achieve better type-coverage without any new coupling:

Why don't we create interfaces for those helper methods then and make those autowirable?

@afilina
Copy link
Contributor

afilina commented Jun 21, 2025

This is how I would approach all these questions and concerns, and then you decide.

Typing $foo-> and seeing the possibilities is way more DX-friendly than having to see sibling classes in a folder/namespace.

We need to consider the audience of this feature. Are the same people who care about decoupling also comfortable discovering through namespaces? Likely.

ControllerHelper is already an improvement over AbstractController, eg it can be used with invokable controllers.

Nothing prevents Symfony from offering this new option, see how people use it, learn, iterate. It's definitely an improvement, so it's worthwhile.

from a maintainer's perspective, it's an explosion of class

That's a very important consideration and I agree that maintainers should not be asked to bite off more than they can chew.

Why don't we create interfaces for those helper methods then and make those autowirable?

That is best for long-term maintenance of an app, if you want to be able to keep up with upgrades and BC breaks. We shouldn't inject interfaces we don't own into our controllers. With that in mind, the proposed solution makes it easier for devs to create the individual adapters which implement those interfaces.

Copy link

@phcorp phcorp Jun 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: now that it exists, why don't you use and subscribe to the service controller.helper in the AbstractController? or maybe the AbstractController will be removed?

Copy link
Member Author

@nicolas-grekas nicolas-grekas Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could but there are some challenges, like would we preserve the public API of AbstractController while using the implementation of ControllerHelper? setContainer() could create a ControllerHelper? Possibly, but in the end, what for? The code duplication is totally free, and kept safe by the test suite, so things are simpler without any code reuse IMHO.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jun 22, 2025

For even better type-safety and application-level contracts, #[AutowireMethodOf] can generate adapters for functional interfaces. One can define an interface within their application's domain to achieve better type-coverage without any new coupling:

Why don't we create interfaces for those helper methods then and make those autowirable?

A couple of thoughts I had on this topic:

  • First, these type guarantees are not needed for many projects, so that this concern might be seen as overhead for some.
  • From a design perspective, having the interfaces being owned by the app rather than the framework is the highest form of decoupling. This applies here because contrary to many other interfaces provided by the framework, the one we're talking about don't really serve as abstractions to me. They'd be just some type-coverage helpers.
  • This lead me to wonder if local type aliases wouldn't be the correct solution to this problem - we could declare @phpstan-type on ControllerHelper and users would then @phpstan-import-type Render from ControllerHelper e.g. Of course that wouldn't help for autowiring.
  • In any way - last but not least - this is a nice domain to explore after this PR ;)

@ludekbenedik
Copy link

Hi,
is there a benefit to use this construction

class MyController
{
    public function __construct(
        #[AutowireMethodOf(ControllerHelper::class)]
        private \Closure $render,
        #[AutowireMethodOf(ControllerHelper::class)]
        private \Closure $redirectToRoute,
    ) {
    }

    public function showProduct(int $id): Response
    {
        if (!$id) {
            return ($this->redirectToRoute)('product_list');
        }

        return ($this->render)('product/show.html.twig', ['product_id' => $id]);
    }
}

instead of this construction?

class MyController
{
    public function __construct(
        #[Lazy]
        private UrlGeneratorInterface $urlGenerator,
        #[Lazy]
        private TwigHelper $twigHelper,
    ) {
    }

    public function showProduct(int $id): Response
    {
        if (!$id) {
            return new RedirectResponse($this->urlGenerator->generate('product_list'));
        }

        return $this->twigHelper->render('product/show.html.twig', ['product_id' => $id]);
    }
}

There are some methods which only forward theirs arguments to a service like generateUrl, isGranted, createForm, createFormBuilder, getUser, isCsrfTokenValid and those can be replaced by lazy services.

Separate helpers (for better decoupling) can be created for other methods: TwigHelper::render(), HttpHelper::forward(), SecurityHelper::denyAccessUnlessGranted(), ...

If necessary, RoutingHelper::redirectToRoute() can be created.

This approach is more straightforward. In addition, it offers this use.

class MyController
{
    public function showProduct(
        #[MapEntity(id: 'id')] Product $product,
        RouteHelper $routeHelper,
        TwigHelper $twigHelper,
    ): Response {
        if (!$id) {
            return $routeHelper->redirect('product_list');
        }

        return $twigHelper->render('product/show.html.twig', ['product_id' => $id]);
    }

    public function deleteProduct(
        #[MapEntity(id: 'id')] Product $product,
        SecurityHelper $securityHelper,
        // ...
    ): Response {
        $securityHelper->denyAccessUnlessGranted('delete', $product);
        // ...
    }
}

@nicolas-grekas
Copy link
Member Author

@ludekbenedik sure, that's another possible follow up of this PR. Yet, it's not the one I'd push for personally, here is why:

  • It still builds on coupling: those classes would be in the Symfony namespace and wouldn't be abstract.
  • It still builds on inheritance, just elsewhere: the new helpers would need to be inheritable to still provide extensibility (to make the coupling not a hard one).
  • From a DX PoV, it'd make things harder, because discovering a class somewhere deep in a namespace is way harder than discovering a method in a helper class. That's the very reason why this helper has been created: yes, some methods are proxies - and the only reason why they exist is that: improved DX by discoverability. Let's not dismiss this aspect of the current state.
  • From a maintainers PoV, splitting helpers in many classes will just increase the maintenance surface while all the previous items already make them meh.

We can alleviate the first 2 items by introducing interfaces, but that'd weaken the last 2 items to me. Also I don't think it makes sense to introduce interfaces for that, the reason being that helpers are not abstractions per se.

@OskarStark OskarStark changed the title [FrameworkBundle] Add ControllerHelper; the helpers from AbstractController as a standalone service [FrameworkBundle] Add ControllerHelper; the helpers from AbstractController as a standalone service Jun 23, 2025
@derrabus
Copy link
Member

From a design perspective, having the interfaces being owned by the app rather than the framework is the highest form of decoupling.

Right, but effectively Symfony owns the signatures of those helper methods. Telling app developers to provide their own interface which contains a copy of our signature, looks the same on every project and and has to be kept in sync after updating Symfony, does not change that.

This applies here because contrary to many other interfaces provided by the framework, the one we're talking about don't really serve as abstractions to me.

Of course not. All those helpers simply hide some commonly used glue code which is why I wanted to solve it with traits back then.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jun 23, 2025

The ship has sailed for the traits option - it suffers from difficult DI + files/traits explosion ;)

You're right about app-owned interfaces, but only to a certain degree: it would always be possible for an app to write their own adapters if the ones generated by the DIC don't fit. Yes, that might be some work, but that's what e.g the DDD folks would aim for: being the ones in full control of boundaries.

Note that I'm not telling that every apps should do it this style - like I'm not telling anyone DDD is the only possible approach to software design. But it's still nice to draw a path forward that goes in the direction of more advanced SOLID software.

Personally, I think I'd first stop at the AutowireMethodOf + Closure style and then I'd try to improve the tooling so that static analyzers understand how types should be interpreted. It should be possible to create an implicit type from such arguments that'd forward the signature of the target method to callers of the closure, and to callees of the constructor.
And for IDEs, I'd like to see autocompletion when the $ is typed for that argument.

I might also seek for a syntax that removes the need for repeating AutowireMethodOf. JavaScript has "import foo, bar from Baz". Maybe something like that written using an attribute on the constructor or the class itself. For a future PR :)

Let's settle on the PR as is?

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

Successfully merging this pull request may close these issues.

9 participants