Hi,

in UnroutedUrlAssembler::buildExternalUrl there is the following code:

$options['query'] = NestedArray::mergeDeep($parsed['query'], $options['query']);
ksort($options['query']);

This results in the follwoing issue:

I have a link field.
Editor ist inserts a link like this: http://www.example.com&c=1&a=1&b=1
Drupal outputs is as: http://www.example.com&a=1&b=1&c=1

I noticed that because services like affiliate.net need to have query paramters at specific places.
So the questions is what is the intention to sort these parameters?
Can this removed?

CommentFileSizeAuthor
#7 interdiff.2986560.4-7.txt2.19 KBkaythay
#7 2986560-7.drupal.UnroutedUrlAssembler-sorts-Query-params-in-buildExternalUrl.patch2.82 KBkaythay
#4 2986560.patch647 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

carstenG created an issue. See original summary.

carstenG’s picture

carstenG CreditAttribution: carstenG at FFW commented
Related issues: +#2955685: Unrouted URLs cannot have have overridden query or fragments
carstenG’s picture

carstenG CreditAttribution: carstenG at FFW commented
Version: 8.5.x-dev » 8.6.x-dev
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Category: Support request » Bug report
Status: Active » Needs review
FileSize
2986560.patch647 bytes

Well let's find out what happens if we don't sort.

Status: Needs review » Needs work

The last submitted patch, 4: 2986560.patch, failed testing. View results

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

#4 shows some mostly trivial test fails b/c we used simple string assertions instead of parsing out the generated URL and comparing the parts.

https://tools.ietf.org/html/rfc3986#section-3.4 says that the query string is non-hierarchical, where in this context "hierarchical" means that order matters.

So, not sure if this is really a bug?

@carstenG, can you fix this with an outbound path processor that reorders the query parameters to what you need?

kaythay’s picture

kaythay CreditAttribution: kaythay at Aten Design Group commented
FileSize
2986560-7.drupal.UnroutedUrlAssembler-sorts-Query-params-in-buildExternalUrl.patch2.82 KB
interdiff.2986560.4-7.txt2.19 KB

Looks like ksort was added in this issue: #2955685: Unrouted URLs cannot have have overridden query or fragments

It is an edge case, but a significant one especially for legacy/enterprise 3rd-party links that require a certain order and/or URLs with nested URLs that also have query parameters that have not been properly encoded.

kaythay’s picture

kaythay CreditAttribution: kaythay at Aten Design Group commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -78,7 +78,6 @@ protected function buildExternalUrl($uri, array $options = [], $collect_bubbleab
     $options['query'] = NestedArray::mergeDeep($parsed['query'], $options['query']);
-    ksort($options['query']);

I honestly can't see a reason why we should sort the query here. The merging is all what the original issue actually cared about.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

#9++ I re-read the original issue and don't remember why we sorted there.

+++ b/core/modules/system/tests/src/Functional/Common/UrlTest.php
@@ -314,7 +314,7 @@ public function testExternalUrls() {
-    $this->assertEqual('https://www.drupal.org/?awesome=drupal&drupal=awesome', $result);
+    $this->assertEqual('https://www.drupal.org/?drupal=awesome&awesome=drupal', $result);

This should prevent another regression. Read the old patch a few times, and it looks like this essentially reverts a change and then simplifies a bit to read better.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

I think we sort incoming query parameters to reduce the potential variations of cache keys, but it makes no sense to do so for outgoing ones.

Committed/pushed to 8.7.x and 8.6.x, thanks!

  • catch committed a95fa0b on 8.7.x 
    Issue #2986560 by kaythay, catch, mpdonadio: UnroutedUrlAssembler sorts...

  • catch committed 316b0bf on 8.6.x 
    Issue #2986560 by kaythay, catch, mpdonadio: UnroutedUrlAssembler sorts...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.