Add support for "controller" keyword for configuring routes controllers #23227

voronkovich · 2017-06-19T17:25:32Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

This PR adds a syntax sugar for configuring routes controllers in more user-friendly way by using a controller keyword.

# Current syntax
# It looks strange and exposes Symfony's internals
blog_show:
    path:     /blog/{slug}
    defaults: { _controller: AppBundle:Blog:show }

# Suggested syntax
blog_show:
    path:       /blog/{slug}
    controller: AppBundle:Blog:show

<?xml version="1.0" encoding="UTF-8" ?>
<routes xmlns="http://symfony.com/schema/routing" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

    <!-- Current syntax -->
    <route id="blog_list" path="/blog">
        <default key="_controller">AppBundle:Blog:list</default>
    </route>

    <!-- Suggested syntax -->
    <route id="blog_list" path="/blog" controller="AppBundle:Blog:list" />
</routes>

ogizanagi · 2017-06-19T21:02:51Z

src/Symfony/Component/Routing/Tests/Fixtures/controller_aware.yml

+
+app_blog:
+    path: /blog
+    controller: AppBundle:Homepage:show


Any reason for this not to throw, but let the defaults override it?
IMHO, it'd be better to throw an exception when both controller attribute and _controller default are set, in order to avoid some potential WTF moments.

@ogizanagi, I think you're right. I've changed the code to throw the exception when both controller attribute and _controller default are set.

stof · 2017-06-20T12:15:02Z

src/Symfony/Component/Routing/Loader/ControllerAwareXmlFileLoader.php

+ *
+ * @author Oleg Voronkovich <oleg-voronkovich@yandex.ru>
+ */
+class ControllerAwareXmlFileLoader extends XmlFileLoader


Why using a separate file loader instead of adding the feature in the existing one ? AFAICT, adding the new feature is fully backward compatible.

I asked myself the same. But I came to the conclusion it's legit, as the controller parameter is "tied" to the HttpKernel usage of the Routing component.
But then, I wonder if this rather belong to the HttpKernel component or the FrameworkBundle instead?

However, RouteCollectionBuilder::add() also sets the controller and is in the component itself...
Eventually, it's probably not worth it to create separate loaders for this.

@stof, The routing component doesn’t know about "controllers", "views", "models" and all things like that.

The Routing component maps an HTTP request to a set of configuration variables.

The controller keyword is valueless when you use the routing component as a standalone library. So I thinks it's better to extend the existing loaders than change them.

stof · 2017-06-20T12:16:01Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -27,7 +27,7 @@
 */
 class YamlFileLoader extends FileLoader
 {
-    private static $availableKeys = array(
+    protected static $availableKeys = array(


This introduces inheritance extension points, which then have to be maintained forever. And inheritance extension points are the worse to maintain for BC. So -1

javiereguiluz · 2017-06-20T13:11:03Z

I'm not sure about this proposal. It's better for DX ... but it introduces another way to configure something that you can already configure it in several ways.

stof · 2017-06-20T13:46:51Z

Thus, it might then confuse people about why there is _controller in attributes of the Request object

voronkovich · 2017-06-20T14:18:20Z

@stof, I think it's not a problem. Most users prefer to configure routes using annotations and don't know about the Request's _controller parameter.

fabpot · 2017-07-03T10:25:50Z

I like the idea. Back then, I didn't know if we would have more than _controller. 5 years later, I think it's a good time to actually recognize that using _controller is not so nice and move it to a first class citizen.

nicolas-grekas · 2017-07-03T10:29:47Z

Same to me, DX wise, it's way easier to use.
About the implem, I think we should do that in the existing loaders.
I get the argument that the router doesn't know about what a "controller" is, but a loader just maps some input to an output. Adding this case doesn't mean the routing component becomes controller-aware all of a sudden. This will ease the patch and its maintenance.

szymach · 2017-07-03T10:32:39Z

Out of curiosity, is there a way to not define a controller and have the route work? Aside from it being just a placeholder, like for example the logout route. If there is no other way to use routes that to map them to controllers, it should just by a first-class citizen.

stof · 2017-07-03T11:36:26Z

I totally agree about using the existing loader. It also means that other projects uisng the component without FrameworkBundle (Silex, Drupal, etc...) get the same experience.

voronkovich · 2017-07-03T19:53:46Z

@stof, @nicolas-grekas, Okay, maybe you're right. I've made changes to use the existing loaders instead of creating the new ones.

fabpot · 2017-07-04T05:16:20Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

+     * A controller's key name (e.g. '_controller').
+     *
+     * @var string
+     */


We don't document private properties. This can be removed

fabpot · 2017-07-04T05:16:26Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

+    private $controllerKey;
+
+    /**
+     * Constructor.


Can eb removed

fabpot · 2017-07-04T05:16:34Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

+    /**
+     * Constructor.
+     *
+     * @param FileLocatorInterface $locator       A FileLocatorInterface instance


This can be removed as well

fabpot · 2017-07-04T05:16:47Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

+                throw new \InvalidArgumentException(sprintf(
+                    'The routing file "%s" must not specify both the "controller" attribute and the defaults key "%s" for %s.',
+                    $path, $this->controllerKey, $name
+                ));


should be on one line

fabpot · 2017-07-04T05:17:00Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

+     *
+     * @var mixed
+     */
+    private $controllerKey;


phpdocs to be removed

fabpot · 2017-07-04T05:17:05Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

+    private $controllerKey;
+
+    /**
+     * Constructor.


to be removed

fabpot · 2017-07-04T05:17:08Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

+    /**
+     * Constructor.
+     *
+     * @param FileLocatorInterface $locator       A FileLocatorInterface instance


to be removed

fabpot · 2017-07-04T05:17:29Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

+            throw new \InvalidArgumentException(sprintf(
+                'The routing file "%s" must not specify both the "controller" key and the defaults key "%s" for "%s".',
+                $path, $this->controllerKey, $name
+            ));


should be on one line

voronkovich · 2017-07-04T13:42:20Z

@fabpot, I've fixed all the CS issues you pointed out.

nicolas-grekas · 2017-07-04T15:34:33Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

+    /**
+     * @param string $controllerKey A controller's key name
+     */
+    public function __construct(FileLocatorInterface $locator, $controllerKey = 'controller')


why not _controller as default?
why do we need this to be configurable?

nicolas-grekas · 2017-07-04T15:35:12Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml

@@ -26,11 +26,13 @@
        <service id="routing.loader.xml" class="Symfony\Component\Routing\Loader\XmlFileLoader">
            <tag name="routing.loader" />
            <argument type="service" id="file_locator" />
+            <argument>_controller</argument>


if _controller is going to be the default in the loaders constructors, then this should be removed (same below)

voronkovich · 2017-07-04T16:46:53Z

@nicolas-grekas, Fixed. I've removed the controllerKey property. Now the _controller key is hardcoded in the loaders.

nicolas-grekas · 2017-07-11T08:00:22Z

W don't merge PRs with merge commit, please rebase instead.

voronkovich · 2017-07-11T08:49:25Z

@nicolas-grekas, sorry, I used the GitHub's web interface to resolve conflict and didn't know that it creates merge commit. I've removed that commit and rebased the PR. Thanks!

nicolas-grekas · 2017-07-11T08:54:08Z

src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php

@@ -13,8 +13,8 @@

 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Config\FileLocator;
-use Symfony\Component\Routing\Loader\YamlFileLoader;


please keep original ordering (that's our policy: alpha order for new items, "as is" for existing ones)

nicolas-grekas · 2017-07-11T09:01:52Z

src/Symfony/Component/Routing/CHANGELOG.md

@@ -6,6 +6,7 @@ CHANGELOG

 * Added support for prioritized routing loaders.
 * Add matched and default parameters to redirect responses
+ * Added support for a `controller` keyword for configuring routes controllers in YAML and XML configurations. 


routes controllers (route with no "s")

nicolas-grekas · 2017-07-11T09:02:08Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

@@ -11,11 +11,11 @@

 namespace Symfony\Component\Routing\Loader;

-use Symfony\Component\Routing\RouteCollection;


please keep original ordering

nicolas-grekas · 2017-07-11T09:03:24Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

@@ -229,6 +229,16 @@ private function parseConfigs(\DOMElement $node, $path)
            }
        }

+        if ($controller = $node->getAttribute('controller')) {
+            if (isset($defaults['_controller'])) {
+                $name = $node->hasAttribute('id') ? sprintf('"%s"', $node->getAttribute('id')) : sprintf('the %s tag', $node->tagName);


missing quotes around the 2nd %s

nicolas-grekas · 2017-07-11T09:04:08Z

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

+            if (isset($defaults['_controller'])) {
+                $name = $node->hasAttribute('id') ? sprintf('"%s"', $node->getAttribute('id')) : sprintf('the %s tag', $node->tagName);
+
+                throw new \InvalidArgumentException(sprintf('The routing file "%s" must not specify both the "controller" attribute and the defaults key "_controller" for %s.', $path, $name));


would be great to formulate the message so that $path is last

@nicolas-grekas, I just copied an existing message from here: https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Routing/Loader/YamlFileLoader.php#L191

Route "%s" must must not specify both the "controller" attribute and the defaults key "_controller" in file "%s"' ?

@sstok, yes, but all other messages have a format with a file path at the beginning. I think we should keep the consistency.

nicolas-grekas · 2017-07-11T09:04:36Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -11,12 +11,12 @@

 namespace Symfony\Component\Routing\Loader;

-use Symfony\Component\Routing\RouteCollection;


original ordering

nicolas-grekas · 2017-07-11T09:06:03Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -116,6 +116,10 @@ protected function parseRoute(RouteCollection $collection, $name, array $config,
        $methods = isset($config['methods']) ? $config['methods'] : array();
        $condition = isset($config['condition']) ? $config['condition'] : null;

+        if ($controller = isset($config['controller']) ? $config['controller'] : null) {


if (!empty($config['controller'])) { $defaults['_controller'] = $config['controller']; }

nicolas-grekas · 2017-07-11T09:06:20Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -141,6 +145,10 @@ protected function parseImport(RouteCollection $collection, array $config, $path
        $schemes = isset($config['schemes']) ? $config['schemes'] : null;
        $methods = isset($config['methods']) ? $config['methods'] : null;

+        if ($controller = isset($config['controller']) ? $config['controller'] : null) {


xabbuh · 2017-07-15T06:53:04Z

src/Symfony/Component/Routing/Tests/Fixtures/controller/routing.xml

+    xsi:schemaLocation="http://symfony.com/schema/routing
+        http://symfony.com/schema/routing/routing-1.0.xsd">
+
+    <route id="app_homepage" path="/" controller="AppBundle:Homepage:show" />


I think we should also have another imported route that does not specify the controller attribute.

And what about a case where an imported route makes use of the _controller default?

xabbuh · 2017-07-15T06:53:47Z

src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php

+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessageRegExp /The routing file "[^"]*" must not specify both the "controller" attribute and the defaults key "_controller" for the "import" tag/
+     */
+    public function testImportWithOverridedController()


xabbuh · 2017-07-15T06:54:10Z

src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php

+     * @expectedException \InvalidArgumentException
+     * @expectedExceptionMessageRegExp /The routing file "[^"]*" must not specify both the "controller" key and the defaults key "_controller" for "_static"/
+     */
+    public function testImportWithOverridedController()


xabbuh · 2017-07-15T06:54:21Z

src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php

+    public function testOverrideControllerInDefaults()
+    {
+        $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
+        $routeCollection = $loader->load('override_defaults.yml');


$routeCollection is not used

xabbuh · 2017-07-15T06:54:31Z

src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php

+    public function testImportWithOverridedController()
+    {
+        $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
+        $routeCollection = $loader->load('import_override_defaults.yml');


$routeCollection is not used

xabbuh · 2017-07-15T06:54:48Z

src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php

+    public function testImportWithOverridedController()
+    {
+        $loader = new XmlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
+        $routeCollection = $loader->load('import_override_defaults.xml');


$routeCollection is not used

xabbuh · 2017-07-15T06:54:55Z

src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php

+    public function testOverrideControllerInDefaults()
+    {
+        $loader = new XmlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
+        $routeCollection = $loader->load('override_defaults.xml');


$routeCollection is not used

xabbuh · 2017-07-15T07:00:06Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -141,6 +145,10 @@ protected function parseImport(RouteCollection $collection, array $config, $path
        $schemes = isset($config['schemes']) ? $config['schemes'] : null;
        $methods = isset($config['methods']) ? $config['methods'] : null;

+        if (!empty($config['controller'])) {


Why not isset() instead?

xabbuh · 2017-07-15T07:02:08Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -116,6 +116,10 @@ protected function parseRoute(RouteCollection $collection, $name, array $config,
        $methods = isset($config['methods']) ? $config['methods'] : array();
        $condition = isset($config['condition']) ? $config['condition'] : null;

+        if (!empty($config['controller'])) {


Why not isset() instead?

ro0NL

removed my downvote :) this is solved rather elegant actually 👍

ro0NL · 2017-07-15T08:13:21Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

@@ -204,5 +212,8 @@ protected function validate($config, $name, $path)
                $name, $path
            ));
        }
+        if (isset($config['controller']) && isset($config['defaults']['_controller'])) {


~~can be written as isset($a, $b)~~

Sadly you can't. See #22684.

You're right, i've forgot about that. 👍

voronkovich · 2017-07-15T18:30:49Z

@xabbuh, thanks for the code review! I've fixed all issues you pointed out and added the missing test cases.

xabbuh · 2017-07-17T07:07:55Z

src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php

+
+        $route = $routeCollection->get('app_homepage');
+
+        $this->assertSame('FrameworkBundle:Template:template', $route->getDefault('_controller'));


here should now have an assertion for the other import route to have the _controller attribute to be set to the value configured in the import.xml file (same for the YamlFileLoaderTest)

@xabbuh, I've added the missing assertions.

xabbuh

What about a case where the importing file makes use of the new controller option while the imported file makes use of the _controller attribute (and vice versa)? Should that be a supported use case?

voronkovich · 2017-07-17T13:59:31Z

@xabbuh, I think you're right! I've added the test case you suggested.

nicolas-grekas · 2017-08-16T16:33:58Z

Thank you @voronkovich.

…outes controllers (voronkovich) This PR was merged into the 3.4 branch. Discussion ---------- Add support for "controller" keyword for configuring routes controllers | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT This PR adds a syntax sugar for configuring routes controllers in more user-friendly way by using a `controller` keyword. ```yaml # Current syntax # It looks strange and exposes Symfony's internals blog_show: path: /blog/{slug} defaults: { _controller: AppBundle:Blog:show } # Suggested syntax blog_show: path: /blog/{slug} controller: AppBundle:Blog:show ``` ```xml <?xml version="1.0" encoding="UTF-8" ?> <routes xmlns="http://symfony.com/schema/routing" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">  <route id="blog_list" path="/blog"> <default key="_controller">AppBundle:Blog:list</default> </route>  <route id="blog_list" path="/blog" controller="AppBundle:Blog:list" /> </routes> ``` Commits ------- 06bbee8 [Routing] Use "controller" keyword for configuring routes controllers

nicolas-grekas · 2017-08-16T16:34:20Z

@voronkovich can you please open a doc PR/issue?

voronkovich · 2017-08-16T16:46:46Z

@nicolas-grekas, the issue is already created (thank you, @HeahDude!): symfony/symfony-docs#8240

Pierstoval · 2017-08-20T11:05:47Z

@szymach commented on 3 Jul
Out of curiosity, is there a way to not define a controller and have the route work? Aside from it being just a placeholder, like for example the logout route. If there is no other way to use routes that to map them to controllers, it should just by a first-class citizen.

FYI, the defaults: { _controller: ... } is not mandatory. It's just "default" values. _controller is only used by the kernel to know what callable will be executed to render the response. If you create routes with no controller and handle the responses in kernel listeners instead, it'll be ok.

carsonbot added Status: Needs Review Feature labels Jun 19, 2017

voronkovich changed the title ~~Use "controller" keyword for configuring routes controllers~~ Add support for "controller" keyword for configuring routes controllers Jun 19, 2017

ogizanagi reviewed Jun 19, 2017

View reviewed changes

stof requested changes Jun 20, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Jul 3, 2017

fabpot requested changes Jul 4, 2017

View reviewed changes

fabpot approved these changes Jul 4, 2017

View reviewed changes

nicolas-grekas reviewed Jul 4, 2017

View reviewed changes

nicolas-grekas reviewed Jul 11, 2017

View reviewed changes

xabbuh reviewed Jul 15, 2017

View reviewed changes

ro0NL reviewed Jul 15, 2017

View reviewed changes

xabbuh reviewed Jul 17, 2017

View reviewed changes

xabbuh approved these changes Jul 17, 2017

View reviewed changes

HeahDude mentioned this pull request Jul 30, 2017

Mention the new "controller" attribute for XML and YAML routes symfony/symfony-docs#8240

Closed

[Routing] Use "controller" keyword for configuring routes controllers

06bbee8

nicolas-grekas merged commit 06bbee8 into symfony:3.4 Aug 16, 2017

voronkovich deleted the routing-controller branch August 20, 2017 11:27

Haehnchen mentioned this pull request Sep 17, 2017

[Symfony 3.4] Add support for "controller" keyword for configuring routes controllers Haehnchen/idea-php-symfony2-plugin#1023

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

javiereguiluz mentioned this pull request Oct 20, 2017

[DX][FrameworkBundle] Simpler route configuration for templates and redirections #24640

Closed

Pierstoval mentioned this pull request Dec 15, 2017

Use route.controller instead of route.defaults._controller symfony/recipes#315

Closed

Tobion mentioned this pull request Feb 9, 2018

Annotation routing in the core #25103

Closed

		@@ -11,11 +11,11 @@

		namespace Symfony\Component\Routing\Loader;

		use Symfony\Component\Routing\RouteCollection;

		@@ -11,12 +11,12 @@

		namespace Symfony\Component\Routing\Loader;

		use Symfony\Component\Routing\RouteCollection;


		$route = $routeCollection->get('app_homepage');

		$this->assertSame('FrameworkBundle:Template:template', $route->getDefault('_controller'));

Add support for "controller" keyword for configuring routes controllers #23227

Add support for "controller" keyword for configuring routes controllers #23227

Conversation

voronkovich commented Jun 19, 2017

ogizanagi Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Jun 20, 2017

stof commented Jun 20, 2017

voronkovich commented Jun 20, 2017

fabpot commented Jul 3, 2017

nicolas-grekas commented Jul 3, 2017 • edited Loading

szymach commented Jul 3, 2017

stof commented Jul 3, 2017

voronkovich commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voronkovich commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voronkovich commented Jul 4, 2017

nicolas-grekas commented Jul 11, 2017

voronkovich commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL left a comment • edited Loading

Choose a reason for hiding this comment

ro0NL Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voronkovich commented Jul 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh left a comment

Choose a reason for hiding this comment

voronkovich commented Jul 17, 2017

nicolas-grekas commented Aug 16, 2017

nicolas-grekas commented Aug 16, 2017

voronkovich commented Aug 16, 2017

Pierstoval commented Aug 20, 2017

ogizanagi Jun 19, 2017 •

edited

Loading

ogizanagi Jun 20, 2017 •

edited

Loading

nicolas-grekas commented Jul 3, 2017 •

edited

Loading

nicolas-grekas Jul 11, 2017 •

edited

Loading

ro0NL left a comment •

edited

Loading

ro0NL Jul 15, 2017 •

edited

Loading