-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Routing][DX] Add full route definition for invokable controller/class #21723
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
Conversation
Please, to add |
I've removed the route name constraint because it's auto generated if use Symfony\Component\Routing\Annotation\Route;
/**
* @Route("/hello")
*/
class Hello
{
public function __invoke()
{
return new Response('Hello World!');
}
} |
$this->reader | ||
->expects($this->once()) | ||
->method('getMethodAnnotations') | ||
->will($this->returnValue(array())) |
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.
Don't you think a test case with an annotation on method would be great to avoid a potential regression?
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.
Sure, thanks!
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.
Test added!
|
||
namespace Symfony\Component\Routing\Tests\Fixtures\AnnotatedClasses; | ||
|
||
class BazClass |
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.
I know this is useless but having related annotation here would be great for people that read tests.
@@ -206,7 +212,7 @@ protected function getDefaultRouteName(\ReflectionClass $class, \ReflectionMetho | |||
return $name; | |||
} | |||
|
|||
protected function getGlobals(\ReflectionClass $class) | |||
protected function getGlobals($annot) |
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.
You should type hint here.
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.
Reverted for now! This method is called from SensioFrameworkExtraBundle
.
45e9302
to
c2375ec
Compare
It would be nice to make the |
To make the
What is the best place to do that? |
Not sure if it makes sense, but I think |
@chalasr thanks, it's clearer now, I just created a PR (sensiolabs/SensioFrameworkExtraBundle#464) to make it possible (combining both PRs): /**
* @Route("/hello", name="hello")
*/
class Hello
{
private $logger;
public function __construct(LoggerInterface $logger)
{
$this->logger = $logger;
}
public function __invoke()
{
$this->logger->info('log entry...');
return new Response('Controller as service!');
}
} |
I've finished here for now, It's ready for review |
This PR was squashed before being merged into the 4.0.x-dev branch (closes #464). Discussion ---------- Default FQCN for invokable controllers This PR implements the [@dunglas's idea](symfony/symfony#21723 (comment)) to make the `service` argument optional for invokable controllers, it allows to omit this argument when using the new convention of using FQN as id. **Before:** ``` use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; /** * @route(service="AppBundle\Controller\Hello") */ class Hello { private $logger; public function __construct(LoggerInterface $logger) { $this->logger = $logger; } /** * @route("/hello/{name}", name="hello") */ public function __invoke($name = 'World') { $this->logger->info('log entry...'); return new Response(sprintf('Hello %s!', $name)); } } ``` **After:** ``` use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; class Hello { private $logger; public function __construct(LoggerInterface $logger) { $this->logger = $logger; } /** * @route("/hello/{name}", name="hello") */ public function __invoke($name = 'World') { $this->logger->info('log entry...'); return new Response(sprintf('Hello %s!', $name)); } } ``` **TODO** - [x] Add Tests - [x] Fix fabbot.io Commits ------- 7463c50 Default FQCN for invokable controllers
@dunglas done! (sensiolabs/SensioFrameworkExtraBundle#464 merged!) |
Thank you @yceruto. |
@yceruto Can you please submit a PR for the docs? Thanks. |
…controller/class (yceruto) This PR was merged into the 3.3-dev branch. Discussion ---------- [Routing][DX] Add full route definition for invokable controller/class | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT | Doc PR | _not yet_ Currently the [`@Route`][1] annotation can be set on the class (for global parameters only). This PR allows you to define the full route annotation for _single_ controllers on the class. Here a common use case of [ADR pattern][3] applied to Symfony: **Before:** ``` use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; /** * @route(service="AppBundle\Controller\Hello") */ class Hello { private $logger; public function __construct(LoggerInterface $logger) { $this->logger = $logger; } /** * @route("/hello/{name}", name="hello") */ public function __invoke($name = 'World') { $this->logger->info('log entry...'); return new Response(sprintf('Hello %s!', $name)); } } ``` **After:** ``` use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route; /** * @route("/hello/{name}", name="hello", service="AppBundle\Controller\Hello") */ class Hello { private $logger; public function __construct(LoggerInterface $logger) { $this->logger = $logger; } public function __invoke($name = 'World') { $this->logger->info('log entry...'); return new Response(sprintf('Hello %s!', $name)); } } ``` This feature does not break any behavior before and works under these conditions: * The class cannot contain other methods with `@Route` annotation (otherwise, this works as before: used for global parameters). * <del>The class `@Route` must have the `name` option defined (otherwise, the route is ignored).</del> This one is auto-generated if `null`. * The class must be invokable: [`__invoke()` method][2] (otherwise, the route is ignored). Btw, this PR fix the inconsistency with other route definitions (xml, yml) where the `_controller` parameter points to the class name only (i.e. without method). [1]: https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Routing/Annotation/Route.php [2]: http://php.net/manual/en/language.oop5.magic.php#object.invoke [3]: https://github.com/pmjones/adr Commits ------- 34e360a Add full route definition to invokable class
Sure, tomorrow I'll do this PR. |
Currently the
@Route
annotation can be set on the class (for global parameters only). This PR allows you to define the full route annotation for invokable controllers on the class.Here a common use case of ADR pattern applied to Symfony:
Before:
After:
This feature does not break any behavior before and works under these conditions:
@Route
annotation (otherwise, this works as before: used for global parameters).The classThis one is auto-generated if@Route
must have thename
option defined (otherwise, the route is ignored).null
.__invoke()
method (otherwise, the route is ignored).Btw, this PR fix the inconsistency with other route definitions (xml, yml) where the
_controller
parameter points to the class name only (i.e. without method).