-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
base: 7.4
Are you sure you want to change the base?
Conversation
dd43bbf
to
7226057
Compare
src/Symfony/Bundle/FrameworkBundle/Controller/ControllerHelper.php
Outdated
Show resolved
Hide resolved
7226057
to
53e9574
Compare
…ntroller as a standalone service
53e9574
to
4bcbaab
Compare
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 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 magic 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
} |
I agree with Jérôme. I wouldn't want to see code using |
@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. |
(Ignoring the likely too high migration costs, I think AbstractController could be favorably deprecated in favor of ControllerHelper, regardless of considerations related to AutowireMethodOf.) |
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
The advantage of the closure is for mocking, but as soon as you add invokable interfaces you lose this advantage. |
Typing
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. 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.)
You don't, anonymous classes to the rescue. |
There was a problem hiding this 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.
Why don't we create interfaces for those helper methods then and make those autowirable? |
This is how I would approach all these questions and concerns, and then you decide.
We need to consider the audience of this feature. Are the same people who care about decoupling also comfortable discovering through namespaces? Likely.
Nothing prevents Symfony from offering this new option, see how people use it, learn, iterate. It's definitely an improvement, so it's worthwhile.
That's a very important consideration and I agree that maintainers should not be asked to bite off more than they can chew.
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. |
There was a problem hiding this comment.
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?
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:
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.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.
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:A More Modern Approach: The Injectable Helper Service
This PR introduces a
ControllerHelper
service. This class extendsAbstractController
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 theControllerHelper
and access the helper methods through it.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:This solution provides numerous benefits:
AbstractController
or even theControllerHelper
class in its methods. It only depends on the injected callables.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:Then update the previous controller example to use this interface:
Symfony's DI container will automatically create an adapter that implements
RenderInterface
and whose__invoke
method calls therender
method of theControllerHelper
. 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.)