From 154810119d90ab5d610128cde6d4039b7807fe76 Mon Sep 17 00:00:00 2001 From: Thomas Calvet Date: Wed, 17 Jul 2019 14:55:17 +0200 Subject: [PATCH] [Routing] Deprecate ServiceRouterLoader and ObjectRouteLoader in favor of ContainerLoader and ObjectLoader --- UPGRADE-4.4.md | 7 + .../Bundle/FrameworkBundle/CHANGELOG.md | 1 + .../Resources/config/routing.xml | 5 + .../Bundle/FrameworkBundle/composer.json | 2 +- src/Symfony/Component/Routing/CHANGELOG.md | 6 + .../Routing/Loader/ContainerLoader.php | 45 ++++++ .../ServiceRouterLoader.php | 5 + .../Component/Routing/Loader/ObjectLoader.php | 84 +++++++++++ .../Routing/Loader/ObjectRouteLoader.php | 46 ++---- .../Tests/Fixtures/TestObjectRouteLoader.php | 24 ++++ .../Tests/Loader/ContainerLoaderTest.php | 36 +++++ .../ServiceRouterLoaderTest.php | 29 ++++ .../Routing/Tests/Loader/ObjectLoaderTest.php | 131 ++++++++++++++++++ .../Tests/Loader/ObjectRouteLoaderTest.php | 34 ++--- 14 files changed, 398 insertions(+), 57 deletions(-) create mode 100644 src/Symfony/Component/Routing/Loader/ContainerLoader.php create mode 100644 src/Symfony/Component/Routing/Loader/ObjectLoader.php create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/TestObjectRouteLoader.php create mode 100644 src/Symfony/Component/Routing/Tests/Loader/ContainerLoaderTest.php create mode 100644 src/Symfony/Component/Routing/Tests/Loader/DependencyInjection/ServiceRouterLoaderTest.php create mode 100644 src/Symfony/Component/Routing/Tests/Loader/ObjectLoaderTest.php diff --git a/UPGRADE-4.4.md b/UPGRADE-4.4.md index ce0742c031..492591a5d4 100644 --- a/UPGRADE-4.4.md +++ b/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 -------- diff --git a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md index 2800b5987d..6e551fb3aa 100644 --- a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md +++ b/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 ----- diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml index 54e16f5b4b..21530280d3 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml @@ -41,6 +41,11 @@ + + The "%service_id%" service is deprecated since Symfony 4.4, use "routing.loader.container" instead. + + + diff --git a/src/Symfony/Bundle/FrameworkBundle/composer.json b/src/Symfony/Bundle/FrameworkBundle/composer.json index 5f9cc55393..927f153c3b 100644 --- a/src/Symfony/Bundle/FrameworkBundle/composer.json +++ b/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", diff --git a/src/Symfony/Component/Routing/CHANGELOG.md b/src/Symfony/Component/Routing/CHANGELOG.md index 05ae44b5f1..36b82dec30 100644 --- a/src/Symfony/Component/Routing/CHANGELOG.md +++ b/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 ----- diff --git a/src/Symfony/Component/Routing/Loader/ContainerLoader.php b/src/Symfony/Component/Routing/Loader/ContainerLoader.php new file mode 100644 index 0000000000..948da7b101 --- /dev/null +++ b/src/Symfony/Component/Routing/Loader/ContainerLoader.php @@ -0,0 +1,45 @@ + + * + * 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 + */ +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); + } +} diff --git a/src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php b/src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php index 0276719c10..a04a19c3c3 100644 --- a/src/Symfony/Component/Routing/Loader/DependencyInjection/ServiceRouterLoader.php +++ b/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 + * + * @deprecated since Symfony 4.4, use Symfony\Component\Routing\Loader\ContainerLoader instead. */ class ServiceRouterLoader extends ObjectRouteLoader { diff --git a/src/Symfony/Component/Routing/Loader/ObjectLoader.php b/src/Symfony/Component/Routing/Loader/ObjectLoader.php new file mode 100644 index 0000000000..e7d9efa1eb --- /dev/null +++ b/src/Symfony/Component/Routing/Loader/ObjectLoader.php @@ -0,0 +1,84 @@ + + * + * 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 + */ +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()); + } +} diff --git a/src/Symfony/Component/Routing/Loader/ObjectRouteLoader.php b/src/Symfony/Component/Routing/Loader/ObjectRouteLoader.php index 8f0680f02a..2bed560322 100644 --- a/src/Symfony/Component/Routing/Loader/ObjectRouteLoader.php +++ b/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 + * + * @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); } } diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/TestObjectRouteLoader.php b/src/Symfony/Component/Routing/Tests/Fixtures/TestObjectRouteLoader.php new file mode 100644 index 0000000000..d272196dd6 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/TestObjectRouteLoader.php @@ -0,0 +1,24 @@ + + * + * 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; + } +} diff --git a/src/Symfony/Component/Routing/Tests/Loader/ContainerLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/ContainerLoaderTest.php new file mode 100644 index 0000000000..5f74111d1b --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Loader/ContainerLoaderTest.php @@ -0,0 +1,36 @@ + + * + * 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], + ]; + } +} diff --git a/src/Symfony/Component/Routing/Tests/Loader/DependencyInjection/ServiceRouterLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/DependencyInjection/ServiceRouterLoaderTest.php new file mode 100644 index 0000000000..497ce2f3b3 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Loader/DependencyInjection/ServiceRouterLoaderTest.php @@ -0,0 +1,29 @@ + + * + * 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()); + } +} diff --git a/src/Symfony/Component/Routing/Tests/Loader/ObjectLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/ObjectLoaderTest.php new file mode 100644 index 0000000000..1267f540d0 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Loader/ObjectLoaderTest.php @@ -0,0 +1,131 @@ + + * + * 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; + } +} diff --git a/src/Symfony/Component/Routing/Tests/Loader/ObjectRouteLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/ObjectRouteLoaderTest.php index a286436de5..52e4be8157 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/ObjectRouteLoaderTest.php +++ b/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;