Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle] Enable controller service with#[Route] attribute#60081
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
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.
Good idea!
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.
Seems like a good idea to me.
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Just saying: It might be interesting to check the perf overhead of scanning all methods instead of just all classes.
This attribute is not the only one that triggers this scan so my comment is not a blocker, but still something I have in mind when reviewing :)
Uh oh!
There was an error while loading.Please reload this page.
antonkomarev commentedMar 29, 2025
@nicolas-grekas is there caching of attribute usage? Then it will only affect cache warm up. |
Yes, I'm thinking about optimizing the compile time indeed. At runtime, it doesn't matter. |
Thank you@GromNaN. |
981b556 intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
I just realized that this PR is missing some integration withhttps://github.com/symfony/symfony/pull/52471/files |
I wasn't sure for security reasons. Requiring the |
Which security problem could this lead to? If a method is registered as a controller, it's safe to be used as a fragment to me. |
nicolas-grekas commentedApr 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Here is the patch that should do it. I'm not submitting it as a PR because it might be not needed in the end: the error message is self explanatory and explains one should use diff --git a/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php b/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.phpindex 12bcd4c2dd..b8e5623476 100644--- a/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php+++ b/src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php@@ -15,6 +15,7 @@ use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Exception\BadRequestException; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Attribute\AsController;+use Symfony\Component\Routing\Attribute\Route; /** * This implementation uses the '_controller' request attribute to determine@@ -26,7 +27,10 @@ use Symfony\Component\HttpKernel\Attribute\AsController; class ControllerResolver implements ControllerResolverInterface { private array $allowedControllerTypes = [];- private array $allowedControllerAttributes = [AsController::class => AsController::class];+ private array $allowedControllerAttributes = [+ AsController::class => AsController::class,+ Route::class => Route::class,+ ]; public function __construct( private ?LoggerInterface $logger = null,@@ -228,12 +232,14 @@ class ControllerResolver implements ControllerResolverInterface } $r = null;+ $method = null; if (\is_array($controller)) {- [$class, $name] = $controller;- $name = (\is_string($class) ? $class : $class::class).'::'.$name;+ [$class, $method] = $controller;+ $name = (\is_string($class) ? $class : $class::class).'::'.$method; } elseif (\is_object($controller) && !$controller instanceof \Closure) { $class = $controller;+ $method = '__invoke'; $name = $class::class.'::__invoke'; } else { $r = new \ReflectionFunction($controller);@@ -255,14 +261,16 @@ class ControllerResolver implements ControllerResolverInterface } }- $r ??= new \ReflectionClass($class);+ do {+ $r ??= new \ReflectionClass($class);- foreach ($r->getAttributes() as $attribute) {- if (isset($this->allowedControllerAttributes[$attribute->getName()])) {- return $controller;+ foreach ($r->getAttributes() as $attribute) {+ if (isset($this->allowedControllerAttributes[$attribute->getName()])) {+ return $controller;+ } }- }-+ } while ($method && $r instanceof \ReflectionClass && method_exists($class, $method) && $r = new \ReflectionMethod($class, $method));+ if (str_contains($name, '@anonymous')) { $name = preg_replace_callback('/[a-zA-Z_\x7f-\xff][\\\\a-zA-Z0-9_\x7f-\xff]*+@anonymous\x00.*?\.php(?:0x?|:[0-9]++\$)?[0-9a-fA-F]++/', fn ($m) => class_exists($m[0], false) ? (get_parent_class($m[0]) ?: key(class_implements($m[0])) ?: 'class').'@anonymous' : $m[0], $name); } |
NVM my latest comments, such controllers will be added to the set of allowed controllers thanks to the tag and to#53985 |
It's possible to backport this feature in an older Symfony project:https://gist.github.com/GromNaN/e4c5a34d34529171fe07a76618ba481b |
Uh oh!
There was an error while loading.Please reload this page.
The
#[AsController]attribute has 2 purposes:controller.service_argumentthat configures the service and service argument injection (RegisterControllerArgumentLocatorsPass)In this PR, I propose to add the tag
argument_resolver.serviceon services when the class has the#[Route]attribute. Removing the need for#[AsController]on classes that use the#[Route]attribute.I assume that if there is a route, it is a controller.
Diff (from thedocs):