UrlGenerator fails to urlencode aliases, incorrectly encodes "system" path

We can also simplify the logic in \Drupal\Core\Routing\UrlGenerator::processPath() by never giving it a path with a query string

Here's a quick first pass at a patch - let's see if it breaks tests (might break unit tests)

NR for bot

CI error for test patch is
11:48:45 exception 'Drupal\simpletest\Exception\MissingGroupException' with message 'Missing @group annotation in Drupal\KernelTests\Core\Path\UrlAlterTest' in /var/www/html/core/modules/simpletest/src/TestDiscovery.php:351

Fixed kernel test - should run but fail on bot

This patch combines the new test code from #10 plus my changes from #5 plus some of dawehner's test fixes from #2507005: UrlGenerator:processPath() doesn't use altered query parameters

Fix to use UrlHelper::buildQuery() instead of http_build_query() and small fix from dawehner.

Also make query params modified by reference in every methods they need to be

So silly - need to fix NullGenerator

Not sure why that worked before - the query option needs to be any array

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -292,7 +270,7 @@ public function generateFromRoute($name, $parameters = array(), $options = array
+      $query = $options['query'] ? '?' . http_build_query($options['query'], '', '&') : '';

This is wrong - need to use the Drupal implementation from UrlHelper. Also we are passing too many params to it.

Need to add some tests for urlaliases for paths that would get incorrectly encoded before

I disagree - the query option was never supported as a string in 8.x. It documented on the method as only being an array and we made a very deliberate choice not to support strings. The bug where a string worked was also limited to only the case of a _no_path option, so not a common use.

Also, this looks like an unintended change?

@@ -514,7 +514,7 @@ 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) {
+      ->willReturnCallback(function ($path, &$options = [], Request $request = NULL, BubbleableMetadata $bubbleable_metadata = NULL) {
         $options['query'] = ['zoo' => 5];
         return $path;
       });

Ok, here's a quick start on a test to show part of the bug - creating a path alias to a route path that has characters that are urlencoded will fail to match on outbound path processing and the alias lookup will fail.

Possibly could be writeen better with a data provider, or maybe as a kernel test, though I think there's some value to demonstrating the actual web request works.

The test-only patch is the same as the interdiff and will fail in the 2nd method at least.

Ah, probably didn't account for the tests being run in a subdirectory - need to account for the fact the alias is the last part of the path, not necessarily the full path

I think this will fix the test fails with the patch (should still fail as test only)

oops - forgot to include the test module in the test-only patch

ok, that shows one of the expected fails. Probably should convert to a kernel test and a data provider to better test a range of outbound path processing matching to find aliases

@catch - the rest of the method is not using short array syntax, so using the long syntax is, I think, the correct code style until we do a mass conversion.

There was a bug fixed in the patch with a caller generating a link without a path passed in a string query option, despite the fact that even Drupal 7 only supports passing it as an array. I am sad and perplexed that the various could slipped in treat is as string again. I strongly oppose supporting a string query param which was not valid in even in Drupal 7.

Patch fixes minor problems noted above.

I think it's important to have this fix in 8.2.x also since the incorrectly encoded aliases can cause a number of problems.

Moving back to 8.2.x per discussion with catch in IRC

@catch - ok, I thought it was already in the issue summary? I can certainly make a change record

@Dawhener - I don't think this change is related, since it seems to be a preexisitng bug in 8.2

@xjm - I will add a change record, but we do need this for 8.2.

@catch - what else is needed in the issue summary?

Adding draft change record, setting to NR for change record.