Issue #2809789 by pwolanin, dawehner: UrlGenerator fails to urlencode aliases,...

Issue #2809789 by pwolanin, dawehner: UrlGenerator fails to urlencode aliases, incorrectly encodes "system" path

core/lib/Drupal/Core/Routing/NullGenerator.php

@@ -52,7 +52,7 @@ protected function processRoute($name, Route $route, array &$parameters, Bubblea
   /**
    * {@inheritdoc}
    */
-  protected function getInternalPathFromRoute($name, Route $route, $parameters = array(), $query_params = array()) {
+  protected function getInternalPathFromRoute($name, Route $route, $parameters = array(), &$query_params = array()) {
     return $route->getPath();
   }
 

core/lib/Drupal/Core/Routing/RedirectDestination.php

@@ -64,7 +64,7 @@ public function get() {
         $this->destination = $query->get('destination');
       }
       else {
-        $this->destination = $this->urlGenerator->generateFromRoute('<current>', [], ['query' => UrlHelper::buildQuery(UrlHelper::filterQueryParameters($query->all()))]);
+        $this->destination = $this->urlGenerator->generateFromRoute('<current>', [], ['query' => UrlHelper::filterQueryParameters($query->all())]);
       }
     }
 

core/lib/Drupal/Core/Routing/UrlGenerator.php

@@ -149,12 +149,14 @@ public function getPathFromRoute($name, $parameters = array()) {
    *   The route parameters passed to the generator. Parameters that do not
    *   match any variables will be added to the result as query parameters.
    * @param array $query_params
-   *   Query parameters passed to the generator as $options['query'].
+   *   Query parameters passed to the generator as $options['query']. This may
+   *   be modified if there are extra parameters not used as route variables.
    * @param string $name
    *   The route name or other identifying string from ::getRouteDebugMessage().
    *
    * @return string
-   *   The url path, without any base path, including possible query string.
+   *   The url path, without any base path, without the query string, not URL
+   *   encoded.
    *
    * @throws MissingMandatoryParametersException
    *   When some parameters are missing that are mandatory for the route.
@@ -162,7 +164,7 @@ public function getPathFromRoute($name, $parameters = array()) {
    *   When a parameter value for a placeholder is not correct because it does
    *   not match the requirement.
    */
-  protected function doGenerate(array $variables, array $defaults, array $tokens, array $parameters, array $query_params, $name) {
+  protected function doGenerate(array $variables, array $defaults, array $tokens, array $parameters, array &$query_params, $name) {
     $variables = array_flip($variables);
     $mergedParams = array_replace($defaults, $this->context->getParameters(), $parameters);
 
@@ -208,30 +210,8 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
       $url = '/';
     }
 
-    // The contexts base URL is already encoded (see Symfony\Component\HttpFoundation\Request)
-    $url = str_replace($this->decodedChars[0], $this->decodedChars[1], rawurlencode($url));
-
-    // Drupal paths rarely include dots, so skip this processing if possible.
-    if (strpos($url, '/.') !== FALSE) {
-      // the path segments "." and ".." are interpreted as relative reference when
-      // resolving a URI; see http://tools.ietf.org/html/rfc3986#section-3.3
-      // so we need to encode them as they are not used for this purpose here
-      // otherwise we would generate a URI that, when followed by a user agent
-      // (e.g. browser), does not match this route
-      $url = strtr($url, array('/../' => '/%2E%2E/', '/./' => '/%2E/'));
-      if ('/..' === substr($url, -3)) {
-        $url = substr($url, 0, -2) . '%2E%2E';
-      }
-      elseif ('/.' === substr($url, -2)) {
-        $url = substr($url, 0, -1) . '%2E';
-      }
-    }
-
-    // Add a query string if needed, including extra parameters.
+    // Add extra parameters to the query parameters.
     $query_params += array_diff_key($parameters, $variables, $defaults);
-    if ($query_params && $query = UrlHelper::buildQuery($query_params)) {
-      $url .= '?' . $query;
-    }
 
     return $url;
   }
@@ -251,9 +231,10 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
    *   $parameters merged in.
    *
    * @return string
-   *   The url path corresponding to the route, without the base path.
+   *   The URL path corresponding to the route, without the base path, not URL
+   *   encoded.
    */
-  protected function getInternalPathFromRoute($name, SymfonyRoute $route, $parameters = array(), $query_params = array()) {
+  protected function getInternalPathFromRoute($name, SymfonyRoute $route, $parameters = array(), &$query_params = array()) {
     // The Route has a cache of its own and is not recompiled as long as it does
     // not get modified.
     $compiledRoute = $route->compile();
@@ -274,15 +255,13 @@ public function generate($name, $parameters = array(), $referenceType = self::AB
    */
   public function generateFromRoute($name, $parameters = array(), $options = array(), $collect_bubbleable_metadata = FALSE) {
     $options += array('prefix' => '');
+    if (!isset($options['query']) || !is_array($options['query'])) {
+      $options['query'] = array();
+    }
+
     $route = $this->getRoute($name);
     $generated_url = $collect_bubbleable_metadata ? new GeneratedUrl() : NULL;
 
-    $query_params = [];
-    // Symfony adds any parameters that are not path slugs as query strings.
-    if (isset($options['query']) && is_array($options['query'])) {
-      $query_params = $options['query'];
-    }
-
     $fragment = '';
     if (isset($options['fragment'])) {
       if (($fragment = trim($options['fragment'])) != '') {
@@ -292,7 +271,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
 
     // Generate a relative URL having no path, just query string and fragment.
     if ($route->getOption('_no_path')) {
-      $query = $query_params ? '?' . http_build_query($query_params, '', '&') : '';
+      $query = $options['query'] ? '?' . UrlHelper::buildQuery($options['query']) : '';
       $url = $query . $fragment;
       return $collect_bubbleable_metadata ? $generated_url->setGeneratedUrl($url) : $url;
     }
@@ -302,13 +281,32 @@ public function generateFromRoute($name, $parameters = array(), $options = array
 
     $name = $this->getRouteDebugMessage($name);
     $this->processRoute($name, $route, $parameters, $generated_url);
-    $path = $this->getInternalPathFromRoute($name, $route, $parameters, $query_params);
+    $path = $this->getInternalPathFromRoute($name, $route, $parameters, $options['query']);
     // Outbound path processors might need the route object for the path, e.g.
     // to get the path pattern.
     $options['route'] = $route;
     if ($options['path_processing']) {
       $path = $this->processPath($path, $options, $generated_url);
     }
+    // The contexts base URL is already encoded
+    // (see Symfony\Component\HttpFoundation\Request).
+    $path = str_replace($this->decodedChars[0], $this->decodedChars[1], rawurlencode($path));
+
+    // Drupal paths rarely include dots, so skip this processing if possible.
+    if (strpos($path, '/.') !== FALSE) {
+      // the path segments "." and ".." are interpreted as relative reference when
+      // resolving a URI; see http://tools.ietf.org/html/rfc3986#section-3.3
+      // so we need to encode them as they are not used for this purpose here
+      // otherwise we would generate a URI that, when followed by a user agent
+      // (e.g. browser), does not match this route
+      $path = strtr($path, array('/../' => '/%2E%2E/', '/./' => '/%2E/'));
+      if ('/..' === substr($path, -3)) {
+        $path = substr($path, 0, -2) . '%2E%2E';
+      }
+      elseif ('/.' === substr($path, -2)) {
+        $path = substr($path, 0, -1) . '%2E';
+      }
+    }
 
     if (!empty($options['prefix'])) {
       $path = ltrim($path, '/');
@@ -316,6 +314,8 @@ public function generateFromRoute($name, $parameters = array(), $options = array
       $path = '/' . str_replace('%2F', '/', rawurlencode($prefix)) . $path;
     }
 
+    $query = $options['query'] ? '?' . UrlHelper::buildQuery($options['query']) : '';
+
     // The base_url might be rewritten from the language rewrite in domain mode.
     if (isset($options['base_url'])) {
       $base_url = $options['base_url'];
@@ -329,7 +329,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
         }
       }
 
-      $url = $base_url . $path . $fragment;
+      $url = $base_url . $path . $query . $fragment;
       return $collect_bubbleable_metadata ? $generated_url->setGeneratedUrl($url) : $url;
     }
 
@@ -337,7 +337,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
 
     $absolute = !empty($options['absolute']);
     if (!$absolute || !$host = $this->context->getHost()) {
-      $url = $base_url . $path . $fragment;
+      $url = $base_url . $path . $query . $fragment;
       return $collect_bubbleable_metadata ? $generated_url->setGeneratedUrl($url) : $url;
     }
 
@@ -363,29 +363,16 @@ public function generateFromRoute($name, $parameters = array(), $options = array
     if ($collect_bubbleable_metadata) {
       $generated_url->addCacheContexts(['url.site']);
     }
-    $url = $scheme . '://' . $host . $port . $base_url . $path . $fragment;
+    $url = $scheme . '://' . $host . $port . $base_url . $path . $query . $fragment;
     return $collect_bubbleable_metadata ? $generated_url->setGeneratedUrl($url) : $url;
-
   }
 
   /**
    * Passes the path to a processor manager to allow alterations.
    */
   protected function processPath($path, &$options = array(), BubbleableMetadata $bubbleable_metadata = NULL) {
-    // Router-based paths may have a querystring on them.
-    if ($query_pos = strpos($path, '?')) {
-      // We don't need to do a strict check here because position 0 would mean we
-      // have no actual path to work with.
-      $actual_path = substr($path, 0, $query_pos);
-      $query_string = substr($path, $query_pos);
-    }
-    else {
-      $actual_path = $path;
-      $query_string = '';
-    }
-    $path = $this->pathProcessor->processOutbound($actual_path === '/' ? $actual_path : rtrim($actual_path, '/'), $options, $this->requestStack->getCurrentRequest(), $bubbleable_metadata);
-    $path .= $query_string;
-    return $path;
+    $actual_path = $path === '/' ? $path : rtrim($path, '/');
+    return $this->pathProcessor->processOutbound($actual_path, $options, $this->requestStack->getCurrentRequest(), $bubbleable_metadata);
   }
 
   /**

core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php

@@ -21,7 +21,8 @@ interface UrlGeneratorInterface extends VersatileGeneratorInterface {
    *   \Symfony\Component\Routing\Generator\UrlGeneratorInterface::generate().
    *
    * @return string
-   *   The internal Drupal path corresponding to the route.
+   *   The internal Drupal path corresponding to the route. This string is
+   *   not urlencoded and will be an empty string for the front page.
    */
   public function getPathFromRoute($name, $parameters = array());
 

core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php

@@ -2,7 +2,6 @@
 
 namespace Drupal\language\Plugin\LanguageNegotiation;
 
-use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Entity\ContentEntityInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\PathProcessor\OutboundPathProcessorInterface;
@@ -118,20 +117,8 @@ public function processOutbound($path, &$options = [], Request $request = NULL,
         unset($options['language']);
       }
 
-      if (isset($options['query']) && is_string($options['query'])) {
-        $query = [];
-        parse_str($options['query'], $query);
-        $options['query'] = $query;
-      }
-      else {
-        $options['query'] = [];
-      }
-
       if (!isset($options['query'][static::QUERY_PARAMETER])) {
-        $query_addon = [static::QUERY_PARAMETER => $langcode];
-        $options['query'] += $query_addon;
-        // @todo Remove this once https://www.drupal.org/node/2507005 lands.
-        $path .= (strpos($path, '?') !== FALSE ? '&' : '?') . UrlHelper::buildQuery($query_addon);
+        $options['query'][static::QUERY_PARAMETER] = $langcode;
       }
 
       if ($bubbleable_metadata) {

core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php

@@ -102,11 +102,6 @@ public function processOutbound($path, &$options = array(), Request $request = N
       // enabled, and the corresponding option has been set, we must preserve
       // any explicit user language preference even with cookies disabled.
       if ($this->queryRewrite) {
-        if (isset($options['query']) && is_string($options['query'])) {
-          $query = array();
-          parse_str($options['query'], $query);
-          $options['query'] = $query;
-        }
         if (!isset($options['query'][$this->queryParam])) {
           $options['query'][$this->queryParam] = $this->queryValue;
         }

core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php

@@ -3,6 +3,7 @@
 namespace Drupal\system\Tests\Path;
 
 use Drupal\Core\Database\Database;
+use Drupal\Core\Url;
 use Drupal\simpletest\WebTestBase;
 use Drupal\taxonomy\Entity\Term;
 
@@ -74,6 +75,10 @@ function testUrlAlter() {
     $this->drupalGet("community/" . $term->id());
     $this->assertText($term_name, 'The community/{tid} path gets resolved correctly');
     $this->assertUrlOutboundAlter("/forum/" . $term->id(), "/community/" . $term->id());
+
+    // Test outbound query string altering.
+    $url = Url::fromRoute('user.login');
+    $this->assertIdentical(\Drupal::request()->getBaseUrl() . '/user/login?foo=bar', $url->toString());
   }
 
   /**

core/modules/system/tests/modules/path_encoded_test/path_encoded_test.info.yml

+name: 'Path encoded character test'
+type: module
+description: 'Support module for testing path aliases on a route with encoded characters in the path.'
+package: Testing
+version: VERSION
+core: 8.x

core/modules/system/tests/modules/path_encoded_test/path_encoded_test.routing.yml

+path_encoded_test.colon:
+  path: '/hi/llamma:party'
+  defaults:
+    _title: 'Colon'
+    _controller: '\Drupal\path_encoded_test\Controller\PathEncodedTestController::simple'
+  requirements:
+    _access: 'TRUE'
+
+path_encoded_test.atsign:
+  path: '/bloggy/@Dries'
+  defaults:
+    _title: 'At sign'
+    _controller: '\Drupal\path_encoded_test\Controller\PathEncodedTestController::simple'
+  requirements:
+    _access: 'TRUE'
+
+path_encoded_test.parens:
+  path: '/cat(box)'
+  defaults:
+    _title: 'At sign'
+    _controller: '\Drupal\path_encoded_test\Controller\PathEncodedTestController::simple'
+  requirements:
+    _access: 'TRUE'

core/modules/system/tests/modules/path_encoded_test/src/Controller/PathEncodedTestController.php

+<?php
+
+namespace Drupal\path_encoded_test\Controller;
+
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * Returns responses for path_encoded_test routes.
+ */
+class PathEncodedTestController {
+
+  /**
+   * Returns a HTML simple response.
+   *
+   * @return \Symfony\Component\HttpFoundation\Response
+   */
+  public function simple() {
+    return new Response('<html><body>PathEncodedTestController works</body></html>');
+  }
+
+}

core/modules/system/tests/modules/url_alter_test/src/PathProcessorTest.php

@@ -49,6 +49,11 @@ public function processOutbound($path, &$options = array(), Request $request = N
       }
     }
 
+    // Verify that $options are alterable.
+    if ($path == '/user/login') {
+      $options['query']['foo'] = 'bar';
+    }
+
     // Rewrite forum/ to community/.
     return preg_replace('@^/forum(.*)@', '/community$1', $path);
   }

core/tests/Drupal/FunctionalTests/Routing/PathEncodedTest.php

+<?php
+
+namespace Drupal\FunctionalTests\Routing;
+
+use Drupal\Core\Url;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests url generation and routing for route paths with encoded characters.
+ *
+ * @group routing
+ */
+class PathEncodedTest extends BrowserTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['system', 'path_encoded_test'];
+
+  public function testGetEncoded() {
+    $route_paths = [
+      'path_encoded_test.colon' => '/hi/llamma:party',
+      'path_encoded_test.atsign' => '/bloggy/@Dries',
+      'path_encoded_test.parens' => '/cat(box)',
+    ];
+    foreach ($route_paths as $route_name => $path) {
+      $this->drupalGet(Url::fromRoute($route_name));
+      $this->assertSession()->pageTextContains('PathEncodedTestController works');
+    }
+  }
+
+  public function testAliasToEncoded() {
+    $route_paths = [
+      'path_encoded_test.colon' => '/hi/llamma:party',
+      'path_encoded_test.atsign' => '/bloggy/@Dries',
+      'path_encoded_test.parens' => '/cat(box)',
+    ];
+    /** @var \Drupal\Core\Path\AliasStorageInterface $alias_storage */
+    $alias_storage = $this->container->get('path.alias_storage');
+    $aliases = [];
+    foreach ($route_paths as $route_name => $path) {
+      $aliases[$route_name] = $this->randomMachineName();
+      $alias_storage->save($path, '/' . $aliases[$route_name]);
+    }
+    foreach ($route_paths as $route_name => $path) {
+      // The alias may be only a suffix of the generated path when the test is
+      // run with Drupal installed in a subdirectory.
+      $this->assertRegExp('@/' . rawurlencode($aliases[$route_name]) . '$@', Url::fromRoute($route_name)->toString());
+      $this->drupalGet(Url::fromRoute($route_name));
+      $this->assertSession()->pageTextContains('PathEncodedTestController works');
+    }
+  }
+
+}

core/tests/Drupal/KernelTests/Core/Path/UrlAlterTest.php

+<?php
+
+namespace Drupal\KernelTests\Core\Path;
+
+use Drupal\Core\Url;
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * Tests the capability to alter URLs.
+ *
+ * @group Path
+ *
+ * @see \Drupal\Core\Routing\UrlGenerator::processPath
+ */
+class UrlAlterTest extends KernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static $modules = ['path', 'url_alter_test', 'user'];
+
+  public function testUrlWithQueryString() {
+    // Test outbound query string altering.
+    $url = Url::fromRoute('user.login');
+    $this->assertEquals(\Drupal::request()->getBaseUrl() . '/user/login?foo=bar', $url->toString());
+  }
+
+}

core/tests/Drupal/Tests/Core/Routing/RedirectDestinationTest.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\Core\Routing;
 
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Routing\RedirectDestination;
 use Drupal\Tests\UnitTestCase;
 use Symfony\Component\HttpFoundation\Request;
@@ -51,7 +52,7 @@ protected function setupUrlGenerator() {
       ->willReturnCallback(function($route, $parameters, $options) {
         $query_string = '';
         if (!empty($options['query'])) {
-          $query_string = '?' . $options['query'];
+          $query_string = '?' . UrlHelper::buildQuery($options['query']);
         }
 
         return '/current-path' . $query_string;

core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php

@@ -68,6 +68,13 @@ class UrlGeneratorTest extends UnitTestCase {
    */
   protected $context;
 
+  /**
+   * The path processor.
+   *
+   * @var \Drupal\Core\PathProcessor\PathProcessorManager
+   */
+  protected $processorManager;
+
   /**
    * {@inheritdoc}
    */
@@ -156,6 +163,7 @@ protected function setUp() {
     $processor = new PathProcessorAlias($this->aliasManager);
     $processor_manager = new PathProcessorManager();
     $processor_manager->addOutbound($processor, 1000);
+    $this->processorManager = $processor_manager;
 
     $this->routeProcessorManager = $this->getMockBuilder('Drupal\Core\RouteProcessor\RouteProcessorManager')
       ->disableOriginalConstructor()
@@ -363,6 +371,13 @@ public function providerTestAliasGenerationWithOptions() {
       ['query' => ['page' => '1/2'], 'fragment' => 'bottom'],
       '/test/two/7?page=1/2#bottom',
     ];
+    // A NULL query string.
+    $data['query-with-NULL'] = [
+      'test_2',
+      ['narf' => '7'],
+      ['query' => NULL, 'fragment' => 'bottom'],
+      '/test/two/7#bottom',
+    ];
     return $data;
   }
 
@@ -489,6 +504,26 @@ public function providerTestNoPath() {
     ];
   }
 
+  /**
+   * @covers \Drupal\Core\Routing\UrlGenerator::generateFromRoute
+   *
+   * Note: We use absolute covers to let
+   * \Drupal\Tests\Core\Render\MetadataBubblingUrlGeneratorTest work.
+   */
+  public function testGenerateWithPathProcessorChangingQueryParameter() {
+    $path_processor = $this->getMock(OutboundPathProcessorInterface::CLASS);
+    $path_processor->expects($this->atLeastOnce())
+      ->method('processOutbound')
+      ->willReturnCallback(function ($path, &$options = array(), Request $request = NULL, BubbleableMetadata $bubbleable_metadata = NULL) {
+        $options['query'] = ['zoo' => 5];
+        return $path;
+      });
+    $this->processorManager->addOutbound($path_processor);
+
+    $options = [];
+    $this->assertGenerateFromRoute('test_2', ['narf' => 5], $options, '/goodbye/cruel/world?zoo=5', (new BubbleableMetadata())->setCacheMaxAge(Cache::PERMANENT));
+  }
+
   /**
    * Asserts \Drupal\Core\Routing\UrlGenerator::generateFromRoute()'s output.
    *