Skip to content

[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

Merged
merged 1 commit into from
Feb 28, 2017

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Feb 23, 2017

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 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:

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).
  • The class @Route must have the name option defined (otherwise, the route is ignored). This one is auto-generated if null.
  • 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).

@yceruto yceruto changed the title Add full route definition for invokable controller/class [Route] Add full route definition for invokable controller/class Feb 23, 2017
@yceruto
Copy link
Member Author

yceruto commented Feb 23, 2017

Please, to add Routing and DX to label list, thanks.

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

yceruto commented Feb 23, 2017

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

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()))
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
Member Author

Choose a reason for hiding this comment

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

Sure, thanks!

Copy link
Member Author

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

@Nek- Nek- Feb 23, 2017

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)
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
Member Author

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.

@yceruto yceruto force-pushed the class_route branch 2 times, most recently from 45e9302 to c2375ec Compare February 23, 2017 16:30
@dunglas
Copy link
Member

dunglas commented Feb 24, 2017

It would be nice to make the service 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 how ActionBundle works).

@yceruto
Copy link
Member Author

yceruto commented Feb 25, 2017

To make the service argument optional (default FQCN) we have to check before, whether this class is registered or not, otherwise AnnotatedRouteControllerLoader doesn't know about it and the results (AppBundle\Controller\Hello:__invoke) could be a You 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-grekas nicolas-grekas added this to the 3.x milestone Feb 25, 2017
@chalasr
Copy link
Member

chalasr commented Feb 26, 2017

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 in ControllerResolver.php#L89, which should be the one responsible for validating it.

@yceruto
Copy link
Member Author

yceruto commented Feb 26, 2017

@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!');
    }
}

@yceruto
Copy link
Member Author

yceruto commented Feb 27, 2017

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

fabpot added a commit to sensiolabs/SensioFrameworkExtraBundle that referenced this pull request Feb 28, 2017
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
@yceruto
Copy link
Member Author

yceruto commented Feb 28, 2017

@dunglas done! (sensiolabs/SensioFrameworkExtraBundle#464 merged!)

@fabpot
Copy link
Member

fabpot commented Feb 28, 2017

Thank you @yceruto.

@fabpot
Copy link
Member

fabpot commented Feb 28, 2017

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

@fabpot fabpot merged commit 34e360a into symfony:master Feb 28, 2017
fabpot added a commit that referenced this pull request Feb 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/adr

Commits
-------

34e360a Add full route definition to invokable class
@yceruto yceruto deleted the class_route branch March 1, 2017 00:12
@yceruto
Copy link
Member Author

yceruto commented Mar 1, 2017

Sure, tomorrow I'll do this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants