feature #26085 Deprecate bundle:controller:action and service:method …

…notation (Tobion)

This PR was squashed before being merged into the 4.1-dev branch (closes #26085).

Discussion
----------

Deprecate bundle:controller:action and service:method notation

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #25910
| License       | MIT
| Doc PR        |

The `a::b` notation had some awkward limitations. It supported `MyControllerClass::method` where `MyControllerClass` is either plain class or a service with the same name but the class must exists. This meant it did NOT support `my_service_controller_id::method` because the `class_exists` check would fail at the wrong point in time. But it did support services where class name == id, i.e. the new auto registration based psr naming. This made it very confusing.

I enhanced the `a::b` notation to be very straight forward:
- if `a` exists as a service then use `a` as a service
- otherwise try to use `a` as a class, i.e. `new $a()`
- otherwise check if a::b is a static method (only relevant when the class is abstract or has private contructor). this was potentially supported when using array controller syntax. it now works the same when using the `::` string syntax, like in php itself. since it only happens when nothing else works, it does not have any performance impact.

The old `a:b` syntax is deprecated and just forwards to `a::b` now internally, just as bundle:controller:action.
In general I was able to refactor the logic quite a bit because it always goes through `instantiateController` now.
Spotting deprecated usages is very easy as all outdated routing configs will trigger a deprecation with the DelegatingLoader and it will be normalized in the dumped routes. So you don't get a deprecation again in the ControllerResolver. But if the controller does not come from routing, e.g. twigs render controller function, then it will still be triggered there.

- [x] deprecate `a:b:c`
- [x] deprecate `a:b`
- [x] update existing references to `a::b`
- [x] fix tests
- [x] fix/add support for static controllers
- [x] add support for closures as controllers
- [x] update Symfony\Component\Routing\Loader\ObjectRouteLoader
- [x] deprecate \Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser but we still need to use it in several places for BC.
- [x] add changelog/upgrade
- [x] update controller.service_arguments logic for double colon controller syntax

Commits
-------

f8a609c Deprecate bundle:controller:action and service:method notation

UPGRADE-4.1.md

@@ -15,6 +15,38 @@ EventDispatcher
 FrameworkBundle
 ---------------
 
+ * Deprecated `bundle:controller:action` and `service:action` syntaxes to reference controllers. Use `serviceOrFqcn::method`
+   instead where `serviceOrFqcn` is either the service ID when using controllers as services or the FQCN of the controller.
+
+   Before:
+
+   ```yml
+   bundle_controller:
+       path: /
+       defaults:
+           _controller: FrameworkBundle:Redirect:redirect
+
+   service_controller:
+       path: /
+       defaults:
+           _controller: app.my_controller:myAction
+   ```
+
+   After:
+
+   ```yml
+   bundle_controller:
+       path: /
+       defaults:
+           _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
+
+   service_controller:
+       path: /
+       defaults:
+           _controller: app.my_controller::myAction
+  ```
+
+ * Deprecated `Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser`
  * A `RouterInterface` that does not implement the `WarmableInterface` is deprecated.
  * The `RequestDataCollector` class has been deprecated. Use the `Symfony\Component\HttpKernel\DataCollector\RequestDataCollector` class instead.
 

UPGRADE-5.0.md

@@ -14,6 +14,38 @@ EventDispatcher
 FrameworkBundle
 ---------------
 
+ * Removed support for `bundle:controller:action` and `service:action` syntaxes to reference controllers. Use `serviceOrFqcn::method`
+   instead where `serviceOrFqcn` is either the service ID when using controllers as services or the FQCN of the controller.
+
+   Before:
+
+   ```yml
+   bundle_controller:
+       path: /
+       defaults:
+           _controller: FrameworkBundle:Redirect:redirect
+
+   service_controller:
+       path: /
+       defaults:
+           _controller: app.my_controller:myAction
+   ```
+
+   After:
+
+   ```yml
+   bundle_controller:
+       path: /
+       defaults:
+           _controller: Symfony\Bundle\FrameworkBundle\Controller\RedirectController::redirectAction
+
+   service_controller:
+       path: /
+       defaults:
+           _controller: app.my_controller::myAction
+  ```
+
+ * Removed `Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser`
  * Using a `RouterInterface` that does not implement the `WarmableInterface` is not supported anymore.
  * The `RequestDataCollector` class has been removed. Use the `Symfony\Component\HttpKernel\DataCollector\RequestDataCollector` class instead.
 

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

@@ -11,6 +11,9 @@ CHANGELOG
  * Using a `RouterInterface` that does not implement the `WarmableInterface` is deprecated.
  * The `RequestDataCollector` class has been deprecated. Use the `Symfony\Component\HttpKernel\DataCollector\RequestDataCollector` class instead.
  * The `RedirectController` class allows for 307/308 HTTP status codes
+ * Deprecated `bundle:controller:action` syntax to reference controllers. Use `serviceOrFqcn::method` instead where `serviceOrFqcn`
+   is either the service ID or the FQCN of the controller.
+ * Deprecated `Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser`
 
 4.0.0
 -----

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

@@ -12,16 +12,13 @@
 namespace Symfony\Bundle\FrameworkBundle\Command;
 
 use Symfony\Bundle\FrameworkBundle\Console\Helper\DescriptorHelper;
-use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
-use Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException;
 use Symfony\Component\Routing\RouterInterface;
-use Symfony\Component\Routing\Route;
 
 /**
  * A console command for retrieving information about routes.
@@ -83,20 +80,13 @@ protected function execute(InputInterface $input, OutputInterface $output)
                 throw new \InvalidArgumentException(sprintf('The route "%s" does not exist.', $name));
             }
 
-            $callable = $this->extractCallable($route);
-
             $helper->describe($io, $route, array(
                 'format' => $input->getOption('format'),
                 'raw_text' => $input->getOption('raw'),
                 'name' => $name,
                 'output' => $io,
-                'callable' => $callable,
             ));
         } else {
-            foreach ($routes as $route) {
-                $this->convertController($route);
-            }
-
             $helper->describe($io, $routes, array(
                 'format' => $input->getOption('format'),
                 'raw_text' => $input->getOption('raw'),
@@ -105,41 +95,4 @@ protected function execute(InputInterface $input, OutputInterface $output)
             ));
         }
     }
-
-    private function convertController(Route $route)
-    {
-        if ($route->hasDefault('_controller')) {
-            $nameParser = new ControllerNameParser($this->getApplication()->getKernel());
-            try {
-                $route->setDefault('_controller', $nameParser->build($route->getDefault('_controller')));
-            } catch (\InvalidArgumentException $e) {
-            }
-        }
-    }
-
-    private function extractCallable(Route $route)
-    {
-        if (!$route->hasDefault('_controller')) {
-            return;
-        }
-
-        $controller = $route->getDefault('_controller');
-
-        if (1 === substr_count($controller, ':')) {
-            list($service, $method) = explode(':', $controller);
-            try {
-                return sprintf('%s::%s', get_class($this->getApplication()->getKernel()->getContainer()->get($service)), $method);
-            } catch (ServiceNotFoundException $e) {
-            }
-        }
-
-        $nameParser = new ControllerNameParser($this->getApplication()->getKernel());
-        try {
-            $shortNotation = $nameParser->build($controller);
-            $route->setDefault('_controller', $shortNotation);
-
-            return $controller;
-        } catch (\InvalidArgumentException $e) {
-        }
-    }
 }

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php

@@ -95,9 +95,6 @@ protected function describeRoute(Route $route, array $options = array())
             array('Defaults', $this->formatRouterConfig($route->getDefaults())),
             array('Options', $this->formatRouterConfig($route->getOptions())),
         );
-        if (isset($options['callable'])) {
-            $tableRows[] = array('Callable', $options['callable']);
-        }
 
         $table = new Table($this->getOutput());
         $table->setHeaders($tableHeaders)->setRows($tableRows);

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php

@@ -19,6 +19,8 @@
  * (Bundle\BlogBundle\Controller\PostController::indexAction).
  *
  * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @deprecated since Symfony 4.1
  */
 class ControllerNameParser
 {
@@ -41,6 +43,10 @@ public function __construct(KernelInterface $kernel)
      */
     public function parse($controller)
     {
+        if (2 > func_num_args() || func_get_arg(1)) {
+            @trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.1.', __CLASS__), E_USER_DEPRECATED);
+        }
+
         $parts = explode(':', $controller);
         if (3 !== count($parts) || in_array('', $parts, true)) {
             throw new \InvalidArgumentException(sprintf('The "%s" controller is not a valid "a:b:c" controller string.', $controller));
@@ -86,6 +92,8 @@ public function parse($controller)
      */
     public function build($controller)
     {
+        @trigger_error(sprintf('The %s class is deprecated since Symfony 4.1.', __CLASS__), E_USER_DEPRECATED);
+
         if (0 === preg_match('#^(.*?\\\\Controller\\\\(.+)Controller)::(.+)Action$#', $controller, $match)) {
             throw new \InvalidArgumentException(sprintf('The "%s" controller is not a valid "class::method" string.', $controller));
         }

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php

@@ -37,16 +37,13 @@ protected function createController($controller)
     {
         if (false === strpos($controller, '::') && 2 === substr_count($controller, ':')) {
             // controller in the a:b:c notation then
-            $controller = $this->parser->parse($controller);
-        }
-
-        $resolvedController = parent::createController($controller);
+            $deprecatedNotation = $controller;
+            $controller = $this->parser->parse($deprecatedNotation, false);
 
-        if (1 === substr_count($controller, ':') && is_array($resolvedController)) {
-            $resolvedController[0] = $this->configureController($resolvedController[0]);
+            @trigger_error(sprintf('Referencing controllers with %s is deprecated since Symfony 4.1. Use %s instead.', $deprecatedNotation, $controller), E_USER_DEPRECATED);
         }
 
-        return $resolvedController;
+        return parent::createController($controller);
     }
 
     /**

src/Symfony/Bundle/FrameworkBundle/EventListener/ResolveControllerNameSubscriber.php

@@ -20,6 +20,8 @@
  * Guarantees that the _controller key is parsed into its final format.
  *
  * @author Ryan Weaver <ryan@knpuniversity.com>
+ *
+ * @deprecated since Symfony 4.1
  */
 class ResolveControllerNameSubscriber implements EventSubscriberInterface
 {
@@ -35,7 +37,7 @@ public function onKernelRequest(GetResponseEvent $event)
         $controller = $event->getRequest()->attributes->get('_controller');
         if (is_string($controller) && false === strpos($controller, '::') && 2 === substr_count($controller, ':')) {
             // controller in the a:b:c notation then
-            $event->getRequest()->attributes->set('_controller', $this->parser->parse($controller));
+            $event->getRequest()->attributes->set('_controller', $this->parser->parse($controller, false));
         }
     }
 

src/Symfony/Bundle/FrameworkBundle/Kernel/MicroKernelTrait.php

@@ -64,7 +64,7 @@ public function registerContainerConfiguration(LoaderInterface $loader)
         $loader->load(function (ContainerBuilder $container) use ($loader) {
             $container->loadFromExtension('framework', array(
                 'router' => array(
-                    'resource' => 'kernel:loadRoutes',
+                    'resource' => 'kernel::loadRoutes',
                     'type' => 'service',
                 ),
             ));

src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php

@@ -73,14 +73,29 @@ public function load($resource, $type = null)
         }
 
         foreach ($collection->all() as $route) {
-            if (!is_string($controller = $route->getDefault('_controller')) || !$controller) {
+            if (!is_string($controller = $route->getDefault('_controller'))) {
                 continue;
             }
 
-            try {
-                $controller = $this->parser->parse($controller);
-            } catch (\InvalidArgumentException $e) {
-                // unable to optimize unknown notation
+            if (false !== strpos($controller, '::')) {
+                continue;
+            }
+
+            if (2 === substr_count($controller, ':')) {
+                $deprecatedNotation = $controller;
+
+                try {
+                    $controller = $this->parser->parse($controller, false);
+
+                    @trigger_error(sprintf('Referencing controllers with %s is deprecated since Symfony 4.1. Use %s instead.', $deprecatedNotation, $controller), E_USER_DEPRECATED);
+                } catch (\InvalidArgumentException $e) {
+                    // unable to optimize unknown notation
+                }
+            }
+
+            if (1 === substr_count($controller, ':')) {
+                $controller = str_replace(':', '::', $controller);
+                @trigger_error(sprintf('Referencing controllers with a single colon is deprecated since Symfony 4.1. Use %s instead.', $controller), E_USER_DEPRECATED);
             }
 
             $route->setDefault('_controller', $controller);

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerNameParserTest.php

@@ -16,6 +16,9 @@
 use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser;
 use Symfony\Component\HttpKernel\Kernel;
 
+/**
+ * @group legacy
+ */
 class ControllerNameParserTest extends TestCase
 {
     protected $loader;

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerResolverTest.php

@@ -20,6 +20,7 @@
 use Symfony\Component\DependencyInjection\ContainerAwareInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Controller\ControllerResolverInterface;
 use Symfony\Component\HttpKernel\Tests\Controller\ContainerControllerResolverTest;
 
 class ControllerResolverTest extends ContainerControllerResolverTest
@@ -32,6 +33,7 @@ public function testGetControllerOnContainerAware()
 
         $controller = $resolver->getController($request);
 
+        $this->assertInstanceOf('Symfony\Bundle\FrameworkBundle\Tests\Controller\ContainerAwareController', $controller[0]);
         $this->assertInstanceOf('Symfony\Component\DependencyInjection\ContainerInterface', $controller[0]->getContainer());
         $this->assertSame('testAction', $controller[1]);
     }
@@ -48,6 +50,10 @@ public function testGetControllerOnContainerAwareInvokable()
         $this->assertInstanceOf('Symfony\Component\DependencyInjection\ContainerInterface', $controller->getContainer());
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Referencing controllers with FooBundle:Default:test is deprecated since Symfony 4.1. Use Symfony\Bundle\FrameworkBundle\Tests\Controller\ContainerAwareController::testAction instead.
+     */
     public function testGetControllerWithBundleNotation()
     {
         $shortName = 'FooBundle:Default:test';
@@ -81,7 +87,7 @@ class_exists(AbstractControllerTest::class);
         $resolver = $this->createControllerResolver(null, $container);
 
         $request = Request::create('/');
-        $request->attributes->set('_controller', TestAbstractController::class.':testAction');
+        $request->attributes->set('_controller', TestAbstractController::class.'::testAction');
 
         $this->assertSame(array($controller, 'testAction'), $resolver->getController($request));
         $this->assertSame($container, $controller->getContainer());
@@ -117,7 +123,7 @@ class_exists(AbstractControllerTest::class);
         $resolver = $this->createControllerResolver(null, $container);
 
         $request = Request::create('/');
-        $request->attributes->set('_controller', DummyController::class.':fooAction');
+        $request->attributes->set('_controller', DummyController::class.'::fooAction');
 
         $this->assertSame(array($controller, 'fooAction'), $resolver->getController($request));
         $this->assertSame($container, $controller->getContainer());
@@ -157,13 +163,13 @@ class_exists(AbstractControllerTest::class);
         $resolver = $this->createControllerResolver(null, $container);
 
         $request = Request::create('/');
-        $request->attributes->set('_controller', DummyController::class.':fooAction');
+        $request->attributes->set('_controller', DummyController::class.'::fooAction');
 
         $this->assertSame(array($controller, 'fooAction'), $resolver->getController($request));
         $this->assertSame($controllerContainer, $controller->getContainer());
     }
 
-    protected function createControllerResolver(LoggerInterface $logger = null, Psr11ContainerInterface $container = null, ControllerNameParser $parser = null)
+    protected function createControllerResolver(LoggerInterface $logger = null, Psr11ContainerInterface $container = null, ControllerNameParser $parser = null): ControllerResolverInterface
     {
         if (!$parser) {
             $parser = $this->createMockParser();

src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/SubRequestController.php

@@ -33,7 +33,7 @@ public function indexAction($handler)
         // ...to check that the FragmentListener still references the right Request
         // when rendering another fragment after the error occurred
         // should render en/html instead of fr/json
-        $content .= $handler->render(new ControllerReference('TestBundle:SubRequest:fragment'));
+        $content .= $handler->render(new ControllerReference(self::class.'::fragmentAction'));
 
         // forces the LocaleListener to set fr for the locale...
         // should render fr/json

src/Symfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/SubRequestServiceResolutionController.php

@@ -24,7 +24,7 @@ class SubRequestServiceResolutionController implements ContainerAwareInterface
     public function indexAction()
     {
         $request = $this->container->get('request_stack')->getCurrentRequest();
-        $path['_controller'] = 'TestBundle:SubRequestServiceResolution:fragment';
+        $path['_controller'] = self::class.'::fragmentAction';
         $subRequest = $request->duplicate(array(), null, $path);
 
         return $this->container->get('http_kernel')->handle($subRequest, HttpKernelInterface::SUB_REQUEST);