feature #32582 [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | https://github.com/symfony/symfony/pull/30926#discussion_r281170917
| License       | MIT
| Doc PR        | -

This PR aims at deprecating some things to have a more consistent code.

### ServiceRouterLoader

1. This class actually fetches an object from a container. In https://github.com/symfony/symfony/pull/30926#discussion_r281170917, it was suggested that it should be renamed to `ContainerRouteLoader`. Actually I think it's better to rename it to `ContainerLoader` since all others route loaders does not have "Route" in their name. Let's be consistent!

2. This class is in a `DependencyInjection` sub directory for historical reasons. Let's remove that! It accepts any PSR-11 container.

### ObjectRouteLoader

1. This class has "Route" in its name too. Let's rename it!

2. This class is supposed to be an abstract implementation for "object" loaders to reuse, but in its code it has a lot of references to "services". Let's remove those references! That means renaming some methods, altering messages, etc.. That also means removing the `supports` method from it to let extending classes implement it.

3. IMHO, this abstract implementation is useless. We sould just deprecate the whole class and move the implemention in the `ContainerLoader` class.

Commits
-------

154810119d [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader

UPGRADE-4.4.md

@@ -83,6 +83,7 @@ FrameworkBundle
    has been deprecated.
  * The `ControllerResolver` and `DelegatingLoader` classes have been marked as `final`.
  * The `controller_name_converter` and `resolve_controller_name_subscriber` services have been deprecated.
+ * Deprecated `routing.loader.service`, use `routing.loader.container` instead.
 
 HttpClient
 ----------
@@ -129,6 +130,12 @@ PropertyAccess
 
  * Deprecated passing `null` as 2nd argument of `PropertyAccessor::createCache()` method (`$defaultLifetime`), pass `0` instead.
 
+Routing
+-------
+
+ * Deprecated `ServiceRouterLoader` in favor of `ContainerLoader`.
+ * Deprecated `ObjectRouteLoader` in favor of `ObjectLoader`.
+
 Security
 --------
 

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

@@ -10,6 +10,7 @@ CHANGELOG
  * The `ControllerResolver` and `DelegatingLoader` classes have been marked as `final`
  * Added support for configuring chained cache pools
  * Deprecated booting the kernel before running `WebTestCase::createClient()`
+ * Deprecated `routing.loader.service`, use `routing.loader.container` instead.
 
 4.3.0
 -----

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

@@ -41,6 +41,11 @@
         </service>
 
         <service id="routing.loader.service" class="Symfony\Component\Routing\Loader\DependencyInjection\ServiceRouterLoader">
+            <argument type="service" id="service_container" />
+            <deprecated>The "%service_id%" service is deprecated since Symfony 4.4, use "routing.loader.container" instead.</deprecated>
+        </service>
+
+        <service id="routing.loader.container" class="Symfony\Component\Routing\Loader\ContainerLoader">
             <tag name="routing.loader" />
             <argument type="service" id="service_container" />
         </service>

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -27,7 +27,7 @@
         "symfony/polyfill-mbstring": "~1.0",
         "symfony/filesystem": "^3.4|^4.0|^5.0",
         "symfony/finder": "^3.4|^4.0|^5.0",
-        "symfony/routing": "^4.3|^5.0"
+        "symfony/routing": "^4.4|^5.0"
     },
     "require-dev": {
         "doctrine/cache": "~1.0",

src/Symfony/Component/Routing/CHANGELOG.md

@@ -1,6 +1,12 @@
 CHANGELOG
 =========
 
+4.4.0
+-----
+
+ * Deprecated `ServiceRouterLoader` in favor of `ContainerLoader`.
+ * Deprecated `ObjectRouteLoader` in favor of `ObjectLoader`.
+
 4.3.0
 -----
 

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

@@ -0,0 +1,45 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Loader;
+
+use Psr\Container\ContainerInterface;
+
+/**
+ * A route loader that executes a service from a PSR-11 container to load the routes.
+ *
+ * @author Ryan Weaver <ryan@knpuniversity.com>
+ */
+class ContainerLoader extends ObjectLoader
+{
+    private $container;
+
+    public function __construct(ContainerInterface $container)
+    {
+        $this->container = $container;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function supports($resource, $type = null)
+    {
+        return 'service' === $type;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    protected function getObject(string $id)
+    {
+        return $this->container->get($id);
+    }
+}

src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php

@@ -12,12 +12,17 @@
 namespace Symfony\Component\Routing\Loader\DependencyInjection;
 
 use Psr\Container\ContainerInterface;
+use Symfony\Component\Routing\Loader\ContainerLoader;
 use Symfony\Component\Routing\Loader\ObjectRouteLoader;
 
+@trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.4, use "%s" instead.', ServiceRouterLoader::class, ContainerLoader::class), E_USER_DEPRECATED);
+
 /**
  * A route loader that executes a service to load the routes.
  *
  * @author Ryan Weaver <ryan@knpuniversity.com>
+ *
+ * @deprecated since Symfony 4.4, use Symfony\Component\Routing\Loader\ContainerLoader instead.
  */
 class ServiceRouterLoader extends ObjectRouteLoader
 {

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

@@ -0,0 +1,84 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Loader;
+
+use Symfony\Component\Config\Loader\Loader;
+use Symfony\Component\Config\Resource\FileResource;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * A route loader that calls a method on an object to load the routes.
+ *
+ * @author Ryan Weaver <ryan@knpuniversity.com>
+ */
+abstract class ObjectLoader extends Loader
+{
+    /**
+     * Returns the object that the method will be called on to load routes.
+     *
+     * For example, if your application uses a service container,
+     * the $id may be a service id.
+     *
+     * @return object
+     */
+    abstract protected function getObject(string $id);
+
+    /**
+     * Calls the object method that will load the routes.
+     *
+     * @param string      $resource object_id::method
+     * @param string|null $type     The resource type
+     *
+     * @return RouteCollection
+     */
+    public function load($resource, $type = null)
+    {
+        if (!preg_match('/^[^\:]+(?:::(?:[^\:]+))?$/', $resource)) {
+            throw new \InvalidArgumentException(sprintf('Invalid resource "%s" passed to the %s route loader: use the format "object_id::method" or "object_id" if your object class has an "__invoke" method.', $resource, \is_string($type) ? '"'.$type.'"' : 'object'));
+        }
+
+        $parts = explode('::', $resource);
+        $method = $parts[1] ?? '__invoke';
+
+        $loaderObject = $this->getObject($parts[0]);
+
+        if (!\is_object($loaderObject)) {
+            throw new \LogicException(sprintf('%s:getObject() must return an object: %s returned', \get_class($this), \gettype($loaderObject)));
+        }
+
+        if (!\is_callable([$loaderObject, $method])) {
+            throw new \BadMethodCallException(sprintf('Method "%s" not found on "%s" when importing routing resource "%s"', $method, \get_class($loaderObject), $resource));
+        }
+
+        $routeCollection = $loaderObject->$method($this);
+
+        if (!$routeCollection instanceof RouteCollection) {
+            $type = \is_object($routeCollection) ? \get_class($routeCollection) : \gettype($routeCollection);
+
+            throw new \LogicException(sprintf('The %s::%s method must return a RouteCollection: %s returned', \get_class($loaderObject), $method, $type));
+        }
+
+        // make the object file tracked so that if it changes, the cache rebuilds
+        $this->addClassResource(new \ReflectionClass($loaderObject), $routeCollection);
+
+        return $routeCollection;
+    }
+
+    private function addClassResource(\ReflectionClass $class, RouteCollection $collection)
+    {
+        do {
+            if (is_file($class->getFileName())) {
+                $collection->addResource(new FileResource($class->getFileName()));
+            }
+        } while ($class = $class->getParentClass());
+    }
+}

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

@@ -11,16 +11,18 @@
 
 namespace Symfony\Component\Routing\Loader;
 
-use Symfony\Component\Config\Loader\Loader;
-use Symfony\Component\Config\Resource\FileResource;
 use Symfony\Component\Routing\RouteCollection;
 
+@trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.4, use "%s" instead.', ObjectRouteLoader::class, ObjectLoader::class), E_USER_DEPRECATED);
+
 /**
  * A route loader that calls a method on an object to load the routes.
  *
  * @author Ryan Weaver <ryan@knpuniversity.com>
+ *
+ * @deprecated since Symfony 4.4, use ObjectLoader instead.
  */
-abstract class ObjectRouteLoader extends Loader
+abstract class ObjectRouteLoader extends ObjectLoader
 {
     /**
      * Returns the object that the method will be called on to load routes.
@@ -53,32 +55,7 @@ abstract class ObjectRouteLoader extends Loader
             @trigger_error(sprintf('Referencing service route loaders with a single colon is deprecated since Symfony 4.1. Use %s instead.', $resource), E_USER_DEPRECATED);
         }
 
-        $parts = explode('::', $resource);
-        $serviceString = $parts[0];
-        $method = $parts[1] ?? '__invoke';
-
-        $loaderObject = $this->getServiceObject($serviceString);
-
-        if (!\is_object($loaderObject)) {
-            throw new \LogicException(sprintf('%s:getServiceObject() must return an object: %s returned', \get_class($this), \gettype($loaderObject)));
-        }
-
-        if (!\is_callable([$loaderObject, $method])) {
-            throw new \BadMethodCallException(sprintf('Method "%s" not found on "%s" when importing routing resource "%s"', $method, \get_class($loaderObject), $resource));
-        }
-
-        $routeCollection = $loaderObject->$method($this);
-
-        if (!$routeCollection instanceof RouteCollection) {
-            $type = \is_object($routeCollection) ? \get_class($routeCollection) : \gettype($routeCollection);
-
-            throw new \LogicException(sprintf('The %s::%s method must return a RouteCollection: %s returned', \get_class($loaderObject), $method, $type));
-        }
-
-        // make the service file tracked so that if it changes, the cache rebuilds
-        $this->addClassResource(new \ReflectionClass($loaderObject), $routeCollection);
-
-        return $routeCollection;
+        return parent::load($resource, $type);
     }
 
     /**
@@ -89,12 +66,11 @@ abstract class ObjectRouteLoader extends Loader
         return 'service' === $type;
     }
 
-    private function addClassResource(\ReflectionClass $class, RouteCollection $collection)
+    /**
+     * {@inheritdoc}
+     */
+    protected function getObject(string $id)
     {
-        do {
-            if (is_file($class->getFileName())) {
-                $collection->addResource(new FileResource($class->getFileName()));
-            }
-        } while ($class = $class->getParentClass());
+        return $this->getServiceObject($id);
     }
 }

src/Symfony/Component/Routing/Tests/Fixtures/TestObjectRouteLoader.php

@@ -0,0 +1,24 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Tests\Fixtures;
+
+use Symfony\Component\Routing\Loader\ObjectRouteLoader;
+
+class TestObjectRouteLoader extends ObjectRouteLoader
+{
+    public $loaderMap = [];
+
+    protected function getServiceObject($id)
+    {
+        return $this->loaderMap[$id] ?? null;
+    }
+}

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

@@ -0,0 +1,36 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Tests\Loader;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\Routing\Loader\ContainerLoader;
+
+class ContainerLoaderTest extends TestCase
+{
+    /**
+     * @dataProvider supportsProvider
+     */
+    public function testSupports(bool $expected, string $type = null)
+    {
+        $this->assertSame($expected, (new ContainerLoader(new Container()))->supports('foo', $type));
+    }
+
+    public function supportsProvider()
+    {
+        return [
+            [true, 'service'],
+            [false, 'bar'],
+            [false, null],
+        ];
+    }
+}

src/Symfony/Component/Routing/Tests/Loader/DependencyInjection/ServiceRouterLoaderTest.php

@@ -0,0 +1,29 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Tests\Loader;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\Routing\Loader\DependencyInjection\ServiceRouterLoader;
+
+class ServiceRouterLoaderTest extends TestCase
+{
+    /**
+     * @group legacy
+     * @expectedDeprecation The "Symfony\Component\Routing\Loader\DependencyInjection\ServiceRouterLoader" class is deprecated since Symfony 4.4, use "Symfony\Component\Routing\Loader\ContainerLoader" instead.
+     * @expectedDeprecation The "Symfony\Component\Routing\Loader\ObjectRouteLoader" class is deprecated since Symfony 4.4, use "Symfony\Component\Routing\Loader\ObjectLoader" instead.
+     */
+    public function testDeprecationWarning()
+    {
+        new ServiceRouterLoader(new Container());
+    }
+}

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

@@ -0,0 +1,131 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Tests\Loader;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Routing\Loader\ObjectLoader;
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
+
+class ObjectLoaderTest extends TestCase
+{
+    public function testLoadCallsServiceAndReturnsCollection()
+    {
+        $loader = new TestObjectLoader();
+
+        // create a basic collection that will be returned
+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/foo'));
+
+        $loader->loaderMap = [
+            'my_route_provider_service' => new TestObjectLoaderRouteService($collection),
+        ];
+
+        $actualRoutes = $loader->load(
+            'my_route_provider_service::loadRoutes',
+            'service'
+        );
+
+        $this->assertSame($collection, $actualRoutes);
+        // the service file should be listed as a resource
+        $this->assertNotEmpty($actualRoutes->getResources());
+    }
+
+    /**
+     * @expectedException \InvalidArgumentException
+     * @dataProvider getBadResourceStrings
+     */
+    public function testExceptionWithoutSyntax(string $resourceString): void
+    {
+        $loader = new TestObjectLoader();
+        $loader->load($resourceString);
+    }
+
+    public function getBadResourceStrings()
+    {
+        return [
+            ['Foo:Bar:baz'],
+            ['Foo::Bar::baz'],
+            ['Foo:'],
+            ['Foo::'],
+            [':Foo'],
+            ['::Foo'],
+        ];
+    }
+
+    /**
+     * @expectedException \LogicException
+     */
+    public function testExceptionOnNoObjectReturned()
+    {
+        $loader = new TestObjectLoader();
+        $loader->loaderMap = ['my_service' => 'NOT_AN_OBJECT'];
+        $loader->load('my_service::method');
+    }
+
+    /**
+     * @expectedException \BadMethodCallException
+     */
+    public function testExceptionOnBadMethod()
+    {
+        $loader = new TestObjectLoader();
+        $loader->loaderMap = ['my_service' => new \stdClass()];
+        $loader->load('my_service::method');
+    }
+
+    /**
+     * @expectedException \LogicException
+     */
+    public function testExceptionOnMethodNotReturningCollection()
+    {
+        $service = $this->getMockBuilder('stdClass')
+            ->setMethods(['loadRoutes'])
+            ->getMock();
+        $service->expects($this->once())
+            ->method('loadRoutes')
+            ->willReturn('NOT_A_COLLECTION');
+
+        $loader = new TestObjectLoader();
+        $loader->loaderMap = ['my_service' => $service];
+        $loader->load('my_service::loadRoutes');
+    }
+}
+
+class TestObjectLoader extends ObjectLoader
+{
+    public $loaderMap = [];
+
+    public function supports($resource, $type = null)
+    {
+        return 'service';
+    }
+
+    protected function getObject(string $id)
+    {
+        return $this->loaderMap[$id] ?? null;
+    }
+}
+
+class TestObjectLoaderRouteService
+{
+    private $collection;
+
+    public function __construct($collection)
+    {
+        $this->collection = $collection;
+    }
+
+    public function loadRoutes()
+    {
+        return $this->collection;
+    }
+}

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

@@ -12,26 +12,28 @@
 namespace Symfony\Component\Routing\Tests\Loader;
 
 use PHPUnit\Framework\TestCase;
-use Symfony\Component\Routing\Loader\ObjectRouteLoader;
 use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
+use Symfony\Component\Routing\Tests\Fixtures\TestObjectRouteLoader;
 
+/**
+ * @group legacy
+ */
 class ObjectRouteLoaderTest extends TestCase
 {
     /**
-     * @group legacy
      * @expectedDeprecation Referencing service route loaders with a single colon is deprecated since Symfony 4.1. Use my_route_provider_service::loadRoutes instead.
      */
     public function testLoadCallsServiceAndReturnsCollectionWithLegacyNotation()
     {
-        $loader = new ObjectRouteLoaderForTest();
+        $loader = new TestObjectRouteLoader();
 
         // create a basic collection that will be returned
         $collection = new RouteCollection();
         $collection->add('foo', new Route('/foo'));
 
         $loader->loaderMap = [
-            'my_route_provider_service' => new RouteService($collection),
+            'my_route_provider_service' => new TestObjectRouteLoaderRouteService($collection),
         ];
 
         $actualRoutes = $loader->load(
@@ -46,14 +48,14 @@ class ObjectRouteLoaderTest extends TestCase
 
     public function testLoadCallsServiceAndReturnsCollection()
     {
-        $loader = new ObjectRouteLoaderForTest();
+        $loader = new TestObjectRouteLoader();
 
         // create a basic collection that will be returned
         $collection = new RouteCollection();
         $collection->add('foo', new Route('/foo'));
 
         $loader->loaderMap = [
-            'my_route_provider_service' => new RouteService($collection),
+            'my_route_provider_service' => new TestObjectRouteLoaderRouteService($collection),
         ];
 
         $actualRoutes = $loader->load(
@@ -72,7 +74,7 @@ class ObjectRouteLoaderTest extends TestCase
      */
     public function testExceptionWithoutSyntax(string $resourceString): void
     {
-        $loader = new ObjectRouteLoaderForTest();
+        $loader = new TestObjectRouteLoader();
         $loader->load($resourceString);
     }
 
@@ -93,7 +95,7 @@ class ObjectRouteLoaderTest extends TestCase
      */
     public function testExceptionOnNoObjectReturned()
     {
-        $loader = new ObjectRouteLoaderForTest();
+        $loader = new TestObjectRouteLoader();
         $loader->loaderMap = ['my_service' => 'NOT_AN_OBJECT'];
         $loader->load('my_service::method');
     }
@@ -103,7 +105,7 @@ class ObjectRouteLoaderTest extends TestCase
      */
     public function testExceptionOnBadMethod()
     {
-        $loader = new ObjectRouteLoaderForTest();
+        $loader = new TestObjectRouteLoader();
         $loader->loaderMap = ['my_service' => new \stdClass()];
         $loader->load('my_service::method');
     }
@@ -120,23 +122,13 @@ class ObjectRouteLoaderTest extends TestCase
             ->method('loadRoutes')
             ->willReturn('NOT_A_COLLECTION');
 
-        $loader = new ObjectRouteLoaderForTest();
+        $loader = new TestObjectRouteLoader();
         $loader->loaderMap = ['my_service' => $service];
         $loader->load('my_service::loadRoutes');
     }
 }
 
-class ObjectRouteLoaderForTest extends ObjectRouteLoader
-{
-    public $loaderMap = [];
-
-    protected function getServiceObject($id)
-    {
-        return isset($this->loaderMap[$id]) ? $this->loaderMap[$id] : null;
-    }
-}
-
-class RouteService
+class TestObjectRouteLoaderRouteService
 {
     private $collection;