Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
yceruto commentedFeb 23, 2017 • 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.
Please, to add |
yceruto commentedFeb 23, 2017 • 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.
I've removed the route name constraint because it's auto generated if useSymfony\Component\Routing\Annotation\Route;/** * @Route("/hello") */class Hello{publicfunction__invoke() {returnnewResponse('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!
| namespaceSymfony\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.
| } | ||
| protectedfunctiongetGlobals(\ReflectionClass$class) | ||
| protectedfunctiongetGlobals($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 fromSensioFrameworkExtraBundle.
45e9302 toc2375ecComparedunglas commentedFeb 24, 2017
It would be nice to make the |
yceruto commentedFeb 25, 2017 • 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.
To make the What is the best place to do that? |
chalasr commentedFeb 26, 2017
Not sure if it makes sense, but I think |
yceruto commentedFeb 26, 2017 • 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.
@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;publicfunction__construct(LoggerInterface$logger) {$this->logger =$logger; }publicfunction__invoke() {$this->logger->info('log entry...');returnnewResponse('Controller as service!'); }} |
yceruto commentedFeb 27, 2017
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 controllersThis 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.ioCommits-------7463c50 Default FQCN for invokable controllers
yceruto commentedFeb 28, 2017
@dunglas done! (sensiolabs/SensioFrameworkExtraBundle#464 merged!) |
fabpot commentedFeb 28, 2017
Thank you@yceruto. |
fabpot commentedFeb 28, 2017
@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/adrCommits-------34e360a Add full route definition to invokable class
yceruto commentedMar 1, 2017
Sure, tomorrow I'll do this PR. |
Uh oh!
There was an error while loading.Please reload this page.
Currently the
@Routeannotation can be set on the class (for global parameters only). This PR allows you to define the full route annotation forinvokable controllers on the class.Here a common use case ofADR pattern applied to Symfony:
Before:
After:
This feature does not break any behavior before and works under these conditions:
@Routeannotation (otherwise, this works as before: used for global parameters).The classThis one is auto-generated if@Routemust have thenameoption 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
_controllerparameter points to the class name only (i.e. without method).