Unrouted URLs cannot have have overridden query or fragments

Nice, thank you for the failing test! (And thanks for creating #2955690: Move Common tests in system.module to BTB too of course!)

This then adds back the solution that I developed in #2955383: List available representations in 406 responses.

I think we should actually adapt the unit test and then maybe add some more test cases.
As part of that I realized that we should probably have support for nested query parameters.

+++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -72,12 +73,13 @@ protected function buildExternalUrl($uri, array $options = [], $collect_bubbleab
-    if (isset($parsed['query'])) {
-      $options['query'] = !isset($options['query'])
-        ? $parsed['query']
-        : $options['query'] + $parsed['query'];
-      ksort($options['query']);
-    }
+
+    $parsed += ['query' => []];
+    $options += ['query' => []];
+
+    $options['query'] = NestedArray::mergeDeep($parsed['query'], $options['query']);
+    ksort($options['query']);

I removed a couple of conditions to make it a bit more readable :)

As part of that I realized that we should probably have support for nested query parameters.

If I understand the code correctly, the override-deep-query-merge testcase tests this? In that case, it looks like this patch is ready.

If I understand the code correctly, the override-deep-query-merge testcase tests this? In that case, it looks like this patch is ready.

You are absolute 100% right here :)

👍

Looks like there's a random fail in there. It passed twice, now it failed.

+++ b/core/modules/system/tests/src/Functional/Common/UrlTest.php
@@ -312,9 +312,21 @@ public function testExternalUrls() {
-    $query = [$this->randomMachineName(5) => $this->randomMachineName(5)];
+    $query = ['z' . $this->randomMachineName(5) => $this->randomMachineName(5)];

That's why I made this change in #4.

This is still blocking #2955383: List available representations in 406 responses.

I didn't want to reroll this so I could re-RTBC this. But it's such a trivial one-line change (see #11) that it should be okay.

That reported failure is … very strange. Because https://www.drupal.org/pift-ci-job/965284 only lists one test run, and it was green. Small DrupalCI/d.o bug?

Tagging API-First Initiative, because it's blocking an API-First Initiative issue.

+++ b/core/modules/system/tests/src/Functional/Common/UrlTest.php
@@ -312,7 +312,7 @@ public function testExternalUrls() {
-    $query = [$this->randomMachineName(5) => $this->randomMachineName(5)];
+    $query = ['z' . $this->randomMachineName(5) => $this->randomMachineName(5)];
     $result = Url::fromUri($url, ['query' => $query])->toString();
     $this->assertEqual($url . '&' . http_build_query($query, '', '&'), $result);

Let's just change this to hardcoded stuff. The randomness here is not useful. So make it something like:

    // Verify query string can be extended in an external URL.
    $url = $test_url . '?drupal=awesome';
    $query = ['awesome' => 'drupal'];
    $result = Url::fromUri($url, ['query' => $query])->toString();
    $this->assertEqual('https://www.drupal.org/?awesome=drupal&drupal=awesome', $result);

+++ b/core/modules/rdf/tests/src/Kernel/Field/LinkFieldRdfaTest.php
@@ -42,7 +42,7 @@ protected function setUp() {
-    $this->testValue = 'http://test.me/foo/bar/neque/porro/quisquam/est/qui-dolorem?foo/bar/neque/porro/quisquam/est/qui-dolorem';
+    $this->testValue = 'http://test.me/foo/bar/neque/porro/quisquam/est/qui-dolorem?path=foo/bar/neque/porro/quisquam/est/qui-dolorem';

So this is just an invalid value - do we have to worry about BC because of this what happens on incorrectly entered external URLs in fields?

Let's just change this to hardcoded stuff. The randomness here is not useful. So make it something like:

👍 Done!

So this is just an invalid value - do we have to worry about BC because of this what happens on incorrectly entered external URLs in fields?

If I'd add these test cases:

      'invalid' => ['http://example.com/test?foo', [], 'http://example.com/test?foo'],
      'invalid path' => ['http://example.com/test?foo/bar', [], 'http://example.com/test?foo/bar'],

they'd indeed fail like this:

--- Expected
+++ Actual
@@ @@
-'http://example.com/test?foo'
+'http://example.com/test?foo='

--- Expected
+++ Actual
@@ @@
-'http://example.com/test?foo/bar'
+'http://example.com/test?foo%2Fbar='

Ideally, I think we'd detect that we're not being given query overrides, and in that case we want to return the original query string verbatim, wrong or not. Would you agree?

Let's just change this to hardcoded stuff. The randomness here is not useful. So make it something like:

Wait, are you @alexpott or not?

Ideally, I think we'd detect that we're not being given query overrides, and in that case we want to return the original query string verbatim, wrong or not. Would you agree?

I'm wondering whether there is a legit usecase for having such weird query parameters with empty values. Technically sure, but practically? It feels no, so I would agree with you.

+++ b/core/modules/system/tests/src/Functional/Common/UrlTest.php
@@ -306,13 +306,13 @@ public function testExternalUrls() {
-    $query = [$this->randomMachineName(5) => $this->randomMachineName(5)];
+    $query = ['z' . $this->randomMachineName(5) => $this->randomMachineName(5)];

Hmm... this was the one were I meant we should remove the random-ness because adding the z is pretty weird and an of itself.

Re...

Wait, are you @alexpott or not?

:D well here the random-ness doesn't actually buy us anything - we're not testing escaping an randomMachineName()'s character set doesn't prove too much.

Re

Ideally, I think we'd detect that we're not being given query overrides, and in that case we want to return the original query string verbatim, wrong or not. Would you agree?

Yes i agree.

I just wanted to fix the tests but I think given that this is a problem of \Drupal\Component\Utility\UrlHelper::parse I'm not sure this is really in scope to be fixed here.

That looks great, I like that we removed the randomness of the test.

I've replace the z test with the suggestion from #17 because this also tests the re-ordering. I've run the test locally and it passes so fixing this on commit.

Committed and pushed a7fdfb6469 to 8.6.x and 9cbeaffb7e to 8.5.x. Thanks!

Yay, this unblocked #2955383: List available representations in 406 responses!

This fix has created two regressions in 8.6.x - see #2986560: UnroutedUrlAssembler sorts Query params in buildExternalUrl() and #2987114: Regression in external URLs in menu links with valueless query parameters

Hmmm.