From 94d59fe954437f1c7971f481fe0fd31fc90e99f6 Mon Sep 17 00:00:00 2001 From: Jonathan Cox Date: Mon, 16 Jul 2018 19:49:19 -0400 Subject: [PATCH] [Routing][FrameworkBundle] Allow configurable query encoding type in UrlGenerator --- .../Bundle/FrameworkBundle/CHANGELOG.md | 5 ++ .../DependencyInjection/Configuration.php | 15 ++++++ .../FrameworkExtension.php | 3 ++ .../DependencyInjection/ConfigurationTest.php | 53 +++++++++++++++++++ .../Bundle/FrameworkBundle/composer.json | 2 +- src/Symfony/Component/Routing/CHANGELOG.md | 5 ++ .../Generator/Dumper/PhpGeneratorDumper.php | 6 ++- .../Routing/Generator/UrlGenerator.php | 14 ++++- src/Symfony/Component/Routing/Router.php | 6 ++- .../Dumper/PhpGeneratorDumperTest.php | 28 ++++++++++ .../Tests/Generator/UrlGeneratorTest.php | 23 +++++++- 11 files changed, 151 insertions(+), 9 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md index 6bb39e9f0e411..01f31ba5d2510 100644 --- a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md +++ b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md @@ -1,6 +1,11 @@ CHANGELOG ========= +4.3.0 +----- + + * Allowed configuring query encoding type passed to `http_build_query` in `Symfony\Component\Routing\Generator\UrlGenerator` via a new `framework.router.query_encoding_type` option + 4.2.0 ----- diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index 12f2092320685..b8dc014a64086 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php @@ -468,6 +468,21 @@ private function addRouterSection(ArrayNodeDefinition $rootNode) ->defaultTrue() ->end() ->booleanNode('utf8')->defaultFalse()->end() + ->scalarNode('query_encoding_type') + ->validate() + ->ifNotInArray(array(PHP_QUERY_RFC1738, PHP_QUERY_RFC3986)) + ->thenInvalid( + 'The value %s is not allowed for path "framework.router.query_encoding_type". '. + "Permissible values are PHP_QUERY_RFC1738 and PHP_QUERY_RFC3986, entered as a constant: '!php/const PHP_QUERY_RFC1738'" + ) + ->end() + ->info( + "This value defaults to PHP_QUERY_RFC3986, which makes the UrlGenerator percent-encode spaces (%20) when building query strings.\n". + "It can be set to PHP_QUERY_RFC1738 to make the UrlGenerator encode spaces as plus signs (+) instead.\n". + "It should be declared as a constant, like '!php/const PHP_QUERY_RFC1738'" + ) + ->defaultValue(PHP_QUERY_RFC3986) + ->end() ->end() ->end() ->end() diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index faaebada1592d..60f6e704806a0 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -743,6 +743,9 @@ private function registerRouterConfiguration(array $config, ContainerBuilder $co if (isset($config['type'])) { $argument['resource_type'] = $config['type']; } + if (isset($config['query_encoding_type'])) { + $argument['query_encoding_type'] = $config['query_encoding_type']; + } $router->replaceArgument(2, $argument); $container->setParameter('request_listener.http_port', $config['http_port']); diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php index 9a3b2183e12db..9311ff8b8fa8d 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php @@ -158,6 +158,58 @@ public function provideInvalidAssetConfigurationTests() yield array($createPackageConfig($config), 'You cannot use both "version" and "json_manifest_path" at the same time under "assets" packages.'); } + public function testQueryEncodingTypeDefaultValue() + { + $processor = new Processor(); + $config = $processor->processConfiguration(new Configuration(true), array(array())); + + $this->assertEquals(PHP_QUERY_RFC3986, $config['router']['query_encoding_type']); + } + + public function getTestValidQueryEncodingTypes() + { + return array( + 'PHP_QUERY_RFC1738' => array(PHP_QUERY_RFC1738), + 'PHP_QUERY_RFC3986' => array(PHP_QUERY_RFC3986), + ); + } + + /** + * @dataProvider getTestValidQueryEncodingTypes + */ + public function testValidQueryEncodingType($encodingType) + { + $processor = new Processor(); + + $config = $processor->processConfiguration( + new Configuration(true), + array(array('router' => array('query_encoding_type' => $encodingType, 'resource' => '.'))) + ); + + $this->assertEquals($encodingType, $config['router']['query_encoding_type']); + } + + /** + * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException + */ + public function testInvalidQueryEncodingType() + { + $expectedMessage = 'Invalid configuration for path "framework.router.query_encoding_type": The value '.PHP_INT_MAX." is not allowed for path \"framework.router.query_encoding_type\". Permissible values are PHP_QUERY_RFC1738 and PHP_QUERY_RFC3986, entered as a constant: '!php/const PHP_QUERY_RFC1738'"; + + if (method_exists($this, 'expectException')) { + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage($expectedMessage); + } else { + $this->setExpectedException(InvalidConfigurationException::class, $expectedMessage); + } + + $processor = new Processor(); + $processor->processConfiguration( + new Configuration(true), + array(array('router' => array('query_encoding_type' => PHP_INT_MAX, 'resource' => '.'))) + ); + } + protected static function getBundleDefaultConfig() { return array( @@ -228,6 +280,7 @@ protected static function getBundleDefaultConfig() 'https_port' => 443, 'strict_requirements' => true, 'utf8' => false, + 'query_encoding_type' => PHP_QUERY_RFC3986, ), 'session' => array( 'enabled' => false, diff --git a/src/Symfony/Bundle/FrameworkBundle/composer.json b/src/Symfony/Bundle/FrameworkBundle/composer.json index 8d516821bb138..55d2ede3b51fe 100644 --- a/src/Symfony/Bundle/FrameworkBundle/composer.json +++ b/src/Symfony/Bundle/FrameworkBundle/composer.json @@ -28,7 +28,7 @@ "symfony/polyfill-mbstring": "~1.0", "symfony/filesystem": "~3.4|~4.0", "symfony/finder": "~3.4|~4.0", - "symfony/routing": "^4.1" + "symfony/routing": "^4.3" }, "require-dev": { "doctrine/cache": "~1.0", diff --git a/src/Symfony/Component/Routing/CHANGELOG.md b/src/Symfony/Component/Routing/CHANGELOG.md index 8e359a3722597..17b9d2b2c20f0 100644 --- a/src/Symfony/Component/Routing/CHANGELOG.md +++ b/src/Symfony/Component/Routing/CHANGELOG.md @@ -1,6 +1,11 @@ CHANGELOG ========= +4.3.0 +----- + + * Added a `query_encoding_type` configuration option for `UrlGenerator` to allow encoding query strings according to RFC 1738. + 4.2.0 ----- diff --git a/src/Symfony/Component/Routing/Generator/Dumper/PhpGeneratorDumper.php b/src/Symfony/Component/Routing/Generator/Dumper/PhpGeneratorDumper.php index 6f08a0f395add..8a90b5d02f521 100644 --- a/src/Symfony/Component/Routing/Generator/Dumper/PhpGeneratorDumper.php +++ b/src/Symfony/Component/Routing/Generator/Dumper/PhpGeneratorDumper.php @@ -55,12 +55,14 @@ class {$options['class']} extends {$options['base_class']} { private static \$declaredRoutes; private \$defaultLocale; - - public function __construct(RequestContext \$context, LoggerInterface \$logger = null, string \$defaultLocale = null) + protected \$queryEncodingType; + + public function __construct(RequestContext \$context, LoggerInterface \$logger = null, string \$defaultLocale = null, int \$queryEncodingType = PHP_QUERY_RFC3986) { \$this->context = \$context; \$this->logger = \$logger; \$this->defaultLocale = \$defaultLocale; + \$this->queryEncodingType = \$queryEncodingType; if (null === self::\$declaredRoutes) { self::\$declaredRoutes = {$this->generateDeclaredRoutes()}; } diff --git a/src/Symfony/Component/Routing/Generator/UrlGenerator.php b/src/Symfony/Component/Routing/Generator/UrlGenerator.php index 91794423234fe..ce0b14406595b 100644 --- a/src/Symfony/Component/Routing/Generator/UrlGenerator.php +++ b/src/Symfony/Component/Routing/Generator/UrlGenerator.php @@ -35,6 +35,15 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt */ protected $strictRequirements = true; + /** + * By default, http_build_query() uses PHP_QUERY_RFC1738 as its fourth + * parameter. This implementation passes PHP_QUERY_RFC3986 as its preferred + * encoding type, but it can be configured to use PHP_QUERY_RFC1738. + * + * @var int + */ + protected $queryEncodingType = PHP_QUERY_RFC3986; + protected $logger; private $defaultLocale; @@ -67,12 +76,13 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt '%7C' => '|', ); - public function __construct(RouteCollection $routes, RequestContext $context, LoggerInterface $logger = null, string $defaultLocale = null) + public function __construct(RouteCollection $routes, RequestContext $context, LoggerInterface $logger = null, string $defaultLocale = null, int $queryEncodingType = PHP_QUERY_RFC3986) { $this->routes = $routes; $this->context = $context; $this->logger = $logger; $this->defaultLocale = $defaultLocale; + $this->queryEncodingType = $queryEncodingType; } /** @@ -269,7 +279,7 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa unset($extra['_fragment']); } - if ($extra && $query = http_build_query($extra, '', '&', PHP_QUERY_RFC3986)) { + if ($extra && $query = http_build_query($extra, '', '&', $this->queryEncodingType)) { // "/" and "?" can be left decoded for better user experience, see // http://tools.ietf.org/html/rfc3986#section-3.4 $url .= '?'.strtr($query, array('%2F' => '/')); diff --git a/src/Symfony/Component/Routing/Router.php b/src/Symfony/Component/Routing/Router.php index 3f4838ea023e8..8c0919babe5e0 100644 --- a/src/Symfony/Component/Routing/Router.php +++ b/src/Symfony/Component/Routing/Router.php @@ -117,6 +117,7 @@ public function __construct(LoaderInterface $loader, $resource, array $options = * * resource_type: Type hint for the main resource (optional) * * strict_requirements: Configure strict requirement checking for generators * implementing ConfigurableRequirementsInterface (default is true) + * * query_encoding_type: Configure the encoding type passed by generators to http_build_query * * @param array $options An array of options * @@ -137,6 +138,7 @@ public function setOptions(array $options) 'matcher_cache_class' => 'ProjectUrlMatcher', 'resource_type' => null, 'strict_requirements' => true, + 'query_encoding_type' => PHP_QUERY_RFC3986, ); // check option names and live merge, if errors are encountered Exception will be thrown @@ -321,7 +323,7 @@ public function getGenerator() } if (null === $this->options['cache_dir'] || null === $this->options['generator_cache_class']) { - $this->generator = new $this->options['generator_class']($this->getRouteCollection(), $this->context, $this->logger); + $this->generator = new $this->options['generator_class']($this->getRouteCollection(), $this->context, $this->logger, null, $this->options['query_encoding_type']); } else { $cache = $this->getConfigCacheFactory()->cache($this->options['cache_dir'].'/'.$this->options['generator_cache_class'].'.php', function (ConfigCacheInterface $cache) { @@ -340,7 +342,7 @@ function (ConfigCacheInterface $cache) { require_once $cache->getPath(); } - $this->generator = new $this->options['generator_cache_class']($this->context, $this->logger); + $this->generator = new $this->options['generator_cache_class']($this->context, $this->logger, null, $this->options['query_encoding_type']); } if ($this->generator instanceof ConfigurableRequirementsInterface) { diff --git a/src/Symfony/Component/Routing/Tests/Generator/Dumper/PhpGeneratorDumperTest.php b/src/Symfony/Component/Routing/Tests/Generator/Dumper/PhpGeneratorDumperTest.php index 45464c3a90d53..570fcffbc9e5c 100644 --- a/src/Symfony/Component/Routing/Tests/Generator/Dumper/PhpGeneratorDumperTest.php +++ b/src/Symfony/Component/Routing/Tests/Generator/Dumper/PhpGeneratorDumperTest.php @@ -253,4 +253,32 @@ public function testDumpWithSchemeRequirement() $this->assertEquals('https://localhost/app.php/testing', $absoluteUrl); $this->assertEquals('/app.php/testing', $relativeUrl); } + + public function testDumpWithDefaultQueryEncoding() + { + $this->routeCollection->add('Test1', new Route('/with space')); + + file_put_contents($this->testTmpFilepath, $this->generatorDumper->dump(array('class' => 'Rfc3986UrlGenerator'))); + include $this->testTmpFilepath; + + $rfc3986UrlGenerator = new \Rfc3986UrlGenerator(new RequestContext()); + + $url = $rfc3986UrlGenerator->generate('Test1', array('query' => 'with space', '_fragment' => 'with space')); + + $this->assertEquals('/with%20space?query=with%20space#with%20space', $url); + } + + public function testDumpWithRfc1738QueryEncoding() + { + $this->routeCollection->add('Test1', new Route('/with space')); + + file_put_contents($this->testTmpFilepath, $this->generatorDumper->dump(array('class' => 'Rfc1738UrlGenerator'))); + include $this->testTmpFilepath; + + $rfc1738UrlGenerator = new \Rfc1738UrlGenerator(new RequestContext(), null, null, PHP_QUERY_RFC1738); + + $url = $rfc1738UrlGenerator->generate('Test1', array('query' => 'with space', '_fragment' => 'with space')); + + $this->assertEquals('/with%20space?query=with+space#with%20space', $url); + } } diff --git a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php index d4bf18ccac929..a7bc7c41ea0a6 100644 --- a/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php +++ b/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php @@ -349,6 +349,25 @@ public function testUrlEncoding() ))); } + public function testRfc1738UrlEncoding() + { + // The difference between this and the default encoding will be that spaces in the query string are encoded as + // plus signs (+) and tildes (~) are percent-encoded + $expectedPath = '/app.php/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id' + .'/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id' + .'?query=%40%3A%5B%5D/%28%29%2A%27%22+%2B%2C%3B-._%7E%26%24%3C%3E%7C%7B%7D%25%5C%5E%60%21%3Ffoo%3Dbar%23id'; + + $chars = '@:[]/()*\'" +,;-._~&$<>|{}%\\^`!?foo=bar#id'; + $routes = $this->getRoutes('test', new Route("/$chars/{varpath}", array(), array('varpath' => '.+'))); + + $generator = $this->getGenerator($routes, array(), null, PHP_QUERY_RFC1738); + + $this->assertSame($expectedPath, $generator->generate('test', array( + 'varpath' => $chars, + 'query' => $chars, + ))); + } + public function testEncodingOfRelativePathSegments() { $routes = $this->getRoutes('test', new Route('/dir/../dir/..')); @@ -703,7 +722,7 @@ public function testFragmentsCanBeDefinedAsDefaults() $this->assertEquals('/app.php/testing#fragment', $url); } - protected function getGenerator(RouteCollection $routes, array $parameters = array(), $logger = null) + protected function getGenerator(RouteCollection $routes, array $parameters = array(), $logger = null, $queryEncodingType = PHP_QUERY_RFC3986) { $context = new RequestContext('/app.php'); foreach ($parameters as $key => $value) { @@ -711,7 +730,7 @@ protected function getGenerator(RouteCollection $routes, array $parameters = arr $context->$method($value); } - return new UrlGenerator($routes, $context, $logger); + return new UrlGenerator($routes, $context, $logger, null, $queryEncodingType); } protected function getRoutes($name, Route $route)