Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromyceruto:class_route
Feb 28, 2017

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedFeb 23, 2017
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT
Doc PRnot yet

Currently the@Route annotation 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:

useSensio\Bundle\FrameworkExtraBundle\Configuration\Route;/** * @Route(service="AppBundle\Controller\Hello") */class Hello{private$logger;publicfunction__construct(LoggerInterface$logger)    {$this->logger =$logger;    }/**     * @Route("/hello/{name}", name="hello")     */publicfunction__invoke($name ='World')    {$this->logger->info('log entry...');returnnewResponse(sprintf('Hello %s!',$name));    }}

After:

useSensio\Bundle\FrameworkExtraBundle\Configuration\Route;/** * @Route("/hello/{name}", name="hello", service="AppBundle\Controller\Hello") */class Hello{private$logger;publicfunction__construct(LoggerInterface$logger)    {$this->logger =$logger;    }publicfunction__invoke($name ='World')    {$this->logger->info('log entry...');returnnewResponse(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).
  • The class@Route must have thename option defined (otherwise, the route is ignored). This one is auto-generated ifnull.
  • The class must be invokable:__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).

patie, jvasseur, dunglas, sstok, chalasr, and ogizanagi reacted with thumbs up emoji
@ycerutoyceruto changed the titleAdd full route definition for invokable controller/class[Route] Add full route definition for invokable controller/classFeb 23, 2017
@yceruto
Copy link
MemberAuthor

yceruto commentedFeb 23, 2017
edited
Loading

Please, to addRouting andDX to label list, thanks.

@ycerutoyceruto changed the title[Route] Add full route definition for invokable controller/class[Routing] Add full route definition for invokable controller/classFeb 23, 2017
@ycerutoyceruto changed the title[Routing] Add full route definition for invokable controller/class[Routing][DX] Add full route definition for invokable controller/classFeb 23, 2017
@yceruto
Copy link
MemberAuthor

yceruto commentedFeb 23, 2017
edited
Loading

I've removed the route name constraint because it's auto generated ifnull. Now the minor route definition can be:

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()))
Copy link
Contributor

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?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure, thanks!

Copy link
MemberAuthor

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
Copy link
Contributor

@Nek-Nek-Feb 23, 2017
edited
Loading

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)
Copy link
Contributor

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.

Copy link
MemberAuthor

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.

@ycerutoycerutoforce-pushed theclass_route branch 2 times, most recently from45e9302 toc2375ecCompareFebruary 23, 2017 16:30
@dunglas
Copy link
Member

It would be nice to make theservice argument optional. It should default to the class FQN, it allows to omit this argument when using the new convention of using FQN as id (and it's actually howActionBundle works).

yceruto, fabpot, ogizanagi, and chalasr reacted with thumbs up emojisstok reacted with hooray emoji

@yceruto
Copy link
MemberAuthor

yceruto commentedFeb 25, 2017
edited
Loading

To make theservice argument optional (default FQCN)we have to check before, whether this class is registered or not, otherwiseAnnotatedRouteControllerLoader doesn't know about it and the results (AppBundle\Controller\Hello:__invoke) could be aYou have requested a non-existent service "AppBundle\Controller\Hello" exception:

protected function configureRoute(Route $route, \ReflectionClass $class, \ReflectionMethod $method, $annot){    // controller    $classAnnot = $this->reader->getClassAnnotation($class, $this->routeAnnotationClass);    if ($classAnnot instanceof FrameworkExtraBundleRoute && $service = $classAnnot->getService()) {        $route->setDefault('_controller', $service.':'.$method->getName());    } else {        $route->setDefault('_controller', $class->getName().'::'.$method->getName());    }    //...}

What is the best place to do that?

@nicolas-grekasnicolas-grekas added this to the3.x milestoneFeb 25, 2017
@chalasr
Copy link
Member

What is the best place to do that?

Not sure if it makes sense, but I think_controller should be set to the controller FQCN, which would ends inControllerResolver.php#L89, which should be the one responsible for validating it.

yceruto reacted with thumbs up emoji

@yceruto
Copy link
MemberAuthor

yceruto commentedFeb 26, 2017
edited
Loading

@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
Copy link
MemberAuthor

I've finished here for now, It's ready for review

fabpot added a commit to sensiolabs/SensioFrameworkExtraBundle that referenced this pull requestFeb 28, 2017
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
Copy link
MemberAuthor

@fabpot
Copy link
Member

Thank you@yceruto.

yceruto reacted with hooray emoji

@fabpot
Copy link
Member

@yceruto Can you please submit a PR for the docs? Thanks.

@fabpotfabpot merged commit34e360a intosymfony:masterFeb 28, 2017
fabpot added a commit that referenced this pull requestFeb 28, 2017
…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
@ycerutoyceruto deleted the class_route branchMarch 1, 2017 00:12
@yceruto
Copy link
MemberAuthor

Sure, tomorrow I'll do this PR.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@Nek-Nek-Nek- left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

7 participants

@yceruto@dunglas@chalasr@fabpot@Nek-@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp