From 5a9851524231154bfa44deb1a3604300a266cbab Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Fri, 23 Feb 2018 11:25:56 +0100 Subject: [PATCH] [Routing] allow no-slash root on imported routes --- .../Configurator/ImportConfigurator.php | 33 ++++++++++++++----- .../Routing/Loader/XmlFileLoader.php | 13 ++++++-- .../Routing/Loader/YamlFileLoader.php | 15 +++++++-- .../Loader/schema/routing/routing-1.0.xsd | 1 + .../import_with_no_trailing_slash/routing.xml | 10 ++++++ .../import_with_no_trailing_slash/routing.yml | 10 ++++++ .../Routing/Tests/Fixtures/php_dsl.php | 6 +++- .../Routing/Tests/Fixtures/php_dsl_sub.php | 1 + .../Tests/Fixtures/php_dsl_sub_root.php | 10 ++++++ .../Tests/Loader/PhpFileLoaderTest.php | 7 ++++ .../Tests/Loader/XmlFileLoaderTest.php | 9 +++++ .../Tests/Loader/YamlFileLoaderTest.php | 10 +++++- 12 files changed, 109 insertions(+), 16 deletions(-) create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.xml create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.yml create mode 100644 src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub_root.php diff --git a/src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php b/src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php index baacbe9592..92e7efde46 100644 --- a/src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php +++ b/src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Routing\Loader\Configurator; +use Symfony\Component\Routing\Route; use Symfony\Component\Routing\RouteCollection; /** @@ -40,16 +41,18 @@ class ImportConfigurator * * @return $this */ - final public function prefix($prefix, string $namePrefix = '') + final public function prefix($prefix, bool $trailingSlashOnRoot = true) { - if ('' !== $namePrefix) { - $this->route->addNamePrefix($namePrefix); - } - if (!$prefix) { - return $this; - } if (!\is_array($prefix)) { $this->route->addPrefix($prefix); + if (!$trailingSlashOnRoot) { + $rootPath = (new Route(trim(trim($prefix), '/').'/'))->getPath(); + foreach ($this->route->all() as $route) { + if ($route->getPath() === $rootPath) { + $route->setPath(rtrim($rootPath, '/')); + } + } + } } else { foreach ($prefix as $locale => $localePrefix) { $prefix[$locale] = trim(trim($localePrefix), '/'); @@ -61,13 +64,13 @@ class ImportConfigurator $localizedRoute = clone $route; $localizedRoute->setDefault('_locale', $locale); $localizedRoute->setDefault('_canonical_route', $name); - $localizedRoute->setPath($localePrefix.$route->getPath()); + $localizedRoute->setPath($localePrefix.(!$trailingSlashOnRoot && '/' === $route->getPath() ? '' : $route->getPath())); $this->route->add($name.'.'.$locale, $localizedRoute); } } elseif (!isset($prefix[$locale])) { throw new \InvalidArgumentException(sprintf('Route "%s" with locale "%s" is missing a corresponding prefix in its parent collection.', $name, $locale)); } else { - $route->setPath($prefix[$locale].$route->getPath()); + $route->setPath($prefix[$locale].(!$trailingSlashOnRoot && '/' === $route->getPath() ? '' : $route->getPath())); $this->route->add($name, $route); } } @@ -75,4 +78,16 @@ class ImportConfigurator return $this; } + + /** + * Sets the prefix to add to the name of all child routes. + * + * @return $this + */ + final public function namePrefix(string $namePrefix) + { + $this->route->addNamePrefix($namePrefix); + + return $this; + } } diff --git a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php index 81a4c94ce0..d1ee7b24fa 100644 --- a/src/Symfony/Component/Routing/Loader/XmlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/XmlFileLoader.php @@ -158,6 +158,7 @@ class XmlFileLoader extends FileLoader $host = $node->hasAttribute('host') ? $node->getAttribute('host') : null; $schemes = $node->hasAttribute('schemes') ? preg_split('/[\s,\|]++/', $node->getAttribute('schemes'), -1, PREG_SPLIT_NO_EMPTY) : null; $methods = $node->hasAttribute('methods') ? preg_split('/[\s,\|]++/', $node->getAttribute('methods'), -1, PREG_SPLIT_NO_EMPTY) : null; + $trailingSlashOnRoot = $node->hasAttribute('trailing-slash-on-root') ? XmlUtils::phpize($node->getAttribute('trailing-slash-on-root')) : true; list($defaults, $requirements, $options, $condition, /* $paths */, $prefixes) = $this->parseConfigs($node, $path); @@ -172,6 +173,14 @@ class XmlFileLoader extends FileLoader if ('' !== $prefix || !$prefixes) { $subCollection->addPrefix($prefix); + if (!$trailingSlashOnRoot) { + $rootPath = (new Route(trim(trim($prefix), '/').'/'))->getPath(); + foreach ($subCollection->all() as $route) { + if ($route->getPath() === $rootPath) { + $route->setPath(rtrim($rootPath, '/')); + } + } + } } else { foreach ($prefixes as $locale => $localePrefix) { $prefixes[$locale] = trim(trim($localePrefix), '/'); @@ -181,7 +190,7 @@ class XmlFileLoader extends FileLoader $subCollection->remove($name); foreach ($prefixes as $locale => $localePrefix) { $localizedRoute = clone $route; - $localizedRoute->setPath($localePrefix.$route->getPath()); + $localizedRoute->setPath($localePrefix.(!$trailingSlashOnRoot && '/' === $route->getPath() ? '' : $route->getPath())); $localizedRoute->setDefault('_locale', $locale); $localizedRoute->setDefault('_canonical_route', $name); $subCollection->add($name.'.'.$locale, $localizedRoute); @@ -189,7 +198,7 @@ class XmlFileLoader extends FileLoader } elseif (!isset($prefixes[$locale])) { throw new \InvalidArgumentException(sprintf('Route "%s" with locale "%s" is missing a corresponding prefix when imported in "%s".', $name, $locale, $path)); } else { - $route->setPath($prefixes[$locale].$route->getPath()); + $route->setPath($prefixes[$locale].(!$trailingSlashOnRoot && '/' === $route->getPath() ? '' : $route->getPath())); $subCollection->add($name, $route); } } diff --git a/src/Symfony/Component/Routing/Loader/YamlFileLoader.php b/src/Symfony/Component/Routing/Loader/YamlFileLoader.php index 30d66d3611..91f38e83cc 100644 --- a/src/Symfony/Component/Routing/Loader/YamlFileLoader.php +++ b/src/Symfony/Component/Routing/Loader/YamlFileLoader.php @@ -28,7 +28,7 @@ use Symfony\Component\Config\Loader\FileLoader; class YamlFileLoader extends FileLoader { private static $availableKeys = array( - 'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods', 'defaults', 'requirements', 'options', 'condition', 'controller', 'name_prefix', + 'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods', 'defaults', 'requirements', 'options', 'condition', 'controller', 'name_prefix', 'trailing_slash_on_root', ); private $yamlParser; @@ -155,6 +155,7 @@ class YamlFileLoader extends FileLoader $condition = isset($config['condition']) ? $config['condition'] : null; $schemes = isset($config['schemes']) ? $config['schemes'] : null; $methods = isset($config['methods']) ? $config['methods'] : null; + $trailingSlashOnRoot = $config['trailing_slash_on_root'] ?? true; if (isset($config['controller'])) { $defaults['_controller'] = $config['controller']; @@ -167,6 +168,14 @@ class YamlFileLoader extends FileLoader if (!\is_array($prefix)) { $subCollection->addPrefix($prefix); + if (!$trailingSlashOnRoot) { + $rootPath = (new Route(trim(trim($prefix), '/').'/'))->getPath(); + foreach ($subCollection->all() as $route) { + if ($route->getPath() === $rootPath) { + $route->setPath(rtrim($rootPath, '/')); + } + } + } } else { foreach ($prefix as $locale => $localePrefix) { $prefix[$locale] = trim(trim($localePrefix), '/'); @@ -178,13 +187,13 @@ class YamlFileLoader extends FileLoader $localizedRoute = clone $route; $localizedRoute->setDefault('_locale', $locale); $localizedRoute->setDefault('_canonical_route', $name); - $localizedRoute->setPath($localePrefix.$route->getPath()); + $localizedRoute->setPath($localePrefix.(!$trailingSlashOnRoot && '/' === $route->getPath() ? '' : $route->getPath())); $subCollection->add($name.'.'.$locale, $localizedRoute); } } elseif (!isset($prefix[$locale])) { throw new \InvalidArgumentException(sprintf('Route "%s" with locale "%s" is missing a corresponding prefix when imported in "%s".', $name, $locale, $file)); } else { - $route->setPath($prefix[$locale].$route->getPath()); + $route->setPath($prefix[$locale].(!$trailingSlashOnRoot && '/' === $route->getPath() ? '' : $route->getPath())); $subCollection->add($name, $route); } } diff --git a/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd b/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd index dd2477999d..9f5f235e85 100644 --- a/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd +++ b/src/Symfony/Component/Routing/Loader/schema/routing/routing-1.0.xsd @@ -67,6 +67,7 @@ + diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.xml b/src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.xml new file mode 100644 index 0000000000..f4b23a23ab --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.xml @@ -0,0 +1,10 @@ + + + + + + + diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.yml b/src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.yml new file mode 100644 index 0000000000..f840ecf8b4 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/import_with_no_trailing_slash/routing.yml @@ -0,0 +1,10 @@ +app: + resource: ../controller/routing.yml + name_prefix: a_ + prefix: /slash + +api: + resource: ../controller/routing.yml + name_prefix: b_ + prefix: /no-slash + trailing_slash_on_root: false diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl.php b/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl.php index 0b8fa0a9eb..fbcab15a3a 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl.php @@ -16,7 +16,11 @@ return function (RoutingConfigurator $routes) { ->requirements(array('id' => '\d+')); $routes->import('php_dsl_sub.php') - ->prefix('/zub', 'z_'); + ->namePrefix('z_') + ->prefix('/zub'); + + $routes->import('php_dsl_sub_root.php') + ->prefix('/bus', false); $routes->add('ouf', '/ouf') ->schemes(array('https')) diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub.php b/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub.php index 9eb444ded0..08b05633b1 100644 --- a/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub.php +++ b/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub.php @@ -6,6 +6,7 @@ return function (RoutingConfigurator $routes) { $add = $routes->collection('c_') ->prefix('pub'); + $add('root', '/'); $add('bar', '/bar'); $add->collection('pub_') diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub_root.php b/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub_root.php new file mode 100644 index 0000000000..9b309fc132 --- /dev/null +++ b/src/Symfony/Component/Routing/Tests/Fixtures/php_dsl_sub_root.php @@ -0,0 +1,10 @@ +collection('r_'); + + $add('root', '/'); + $add('bar', '/bar/'); +}; diff --git a/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php index c0a72b6f81..8f7c888ab7 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/PhpFileLoaderTest.php @@ -99,6 +99,9 @@ class PhpFileLoaderTest extends TestCase $expectedCollection->add('buz', (new Route('/zub')) ->setDefaults(array('_controller' => 'foo:act')) ); + $expectedCollection->add('c_root', (new Route('/sub/pub/')) + ->setRequirements(array('id' => '\d+')) + ); $expectedCollection->add('c_bar', (new Route('/sub/pub/bar')) ->setRequirements(array('id' => '\d+')) ); @@ -106,8 +109,11 @@ class PhpFileLoaderTest extends TestCase ->setHost('host') ->setRequirements(array('id' => '\d+')) ); + $expectedCollection->add('z_c_root', new Route('/zub/pub/')); $expectedCollection->add('z_c_bar', new Route('/zub/pub/bar')); $expectedCollection->add('z_c_pub_buz', (new Route('/zub/pub/buz'))->setHost('host')); + $expectedCollection->add('r_root', new Route('/bus')); + $expectedCollection->add('r_bar', new Route('/bus/bar/')); $expectedCollection->add('ouf', (new Route('/ouf')) ->setSchemes(array('https')) ->setMethods(array('GET')) @@ -115,6 +121,7 @@ class PhpFileLoaderTest extends TestCase ); $expectedCollection->addResource(new FileResource(realpath(__DIR__.'/../Fixtures/php_dsl_sub.php'))); + $expectedCollection->addResource(new FileResource(realpath(__DIR__.'/../Fixtures/php_dsl_sub_root.php'))); $expectedCollection->addResource(new FileResource(realpath(__DIR__.'/../Fixtures/php_dsl.php'))); $this->assertEquals($expectedCollection, $routeCollection); diff --git a/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php index 0b2e1a9d79..08fb6bd7df 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/XmlFileLoaderTest.php @@ -411,4 +411,13 @@ class XmlFileLoaderTest extends TestCase $this->assertNotNull($routeCollection->get('api_app_blog')); $this->assertEquals('/api/blog', $routeCollection->get('api_app_blog')->getPath()); } + + public function testImportRouteWithNoTrailingSlash() + { + $loader = new XmlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/import_with_no_trailing_slash'))); + $routeCollection = $loader->load('routing.xml'); + + $this->assertEquals('/slash/', $routeCollection->get('a_app_homepage')->getPath()); + $this->assertEquals('/no-slash', $routeCollection->get('b_app_homepage')->getPath()); + } } diff --git a/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php b/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php index 3bcfe1b5b6..b2673d435c 100644 --- a/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php +++ b/src/Symfony/Component/Routing/Tests/Loader/YamlFileLoaderTest.php @@ -209,7 +209,6 @@ class YamlFileLoaderTest extends TestCase $this->assertCount(3, $routes); } - public function testImportingRoutesFromDefinition() { $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/localized'))); @@ -275,4 +274,13 @@ class YamlFileLoaderTest extends TestCase $this->assertEquals('DefaultController::defaultAction', $routes->get('home.nl')->getDefault('_controller')); $this->assertEquals('DefaultController::defaultAction', $routes->get('not_localized')->getDefault('_controller')); } + + public function testImportRouteWithNoTrailingSlash() + { + $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/import_with_no_trailing_slash'))); + $routeCollection = $loader->load('routing.yml'); + + $this->assertEquals('/slash/', $routeCollection->get('a_app_homepage')->getPath()); + $this->assertEquals('/no-slash', $routeCollection->get('b_app_homepage')->getPath()); + } }