Query string duplications

Provide more tests.
Add tests for link formatter.
Replace options with empty array instead of delete.

I'm not sure about
'?x=1&x=2'
render as
'?x=2'

So I add this to test as is.

If you don't want to patch core.
You can use https://www.drupal.org/project/link_formatter_query_fix to fix this issue.

I had a similar issue when linking to a View with exposed filters.
The patch in #6 successfully resolves this issue.

Patch #6 works for me.

RTBCing it to get this moving a bit more.

Testbot had a hiccup re-rtbcing

1. The test coverage is looking pretty extensive - nice one!

2. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -484,6 +487,192 @@ public function testLinkFormatter() {
   +    // Create an entity with three link field values:
   +    // - The first field item uses a URL only.
   +    // - The second field item uses a URL and link text.
   +    // - The third field item uses a fragment-only URL with text.
   

   I'm not sure what you mean by "three link field values". We seem to have 1 field with count($test_urls) number of values.

3. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -484,6 +487,192 @@ public function testLinkFormatter() {
   +    // For consistency in assertion code below, the URL is assigned to the title
   +    // variable for the first field.
   

   Is this true?

4. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -484,6 +487,192 @@ public function testLinkFormatter() {
   +    foreach ($test_urls as $key => &$test_url) {
   

   Why by reference? I'm not sure that that is necessary.

5. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -484,6 +487,192 @@ public function testLinkFormatter() {
   +    // dependency on 'url_only', so we have a total of ~10 cases.
   

   We have 7 * 7 permutations as far as I can see. Is this correct?

6. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -484,6 +487,192 @@ public function testLinkFormatter() {
   +  /**
   +   * Get array of url with complex query parameters for render check.
   +   */
   +  protected function getUrlWithComplexQuery() {
   ...
   +
   +  /**
   +   * Get list of url with complex query parameters for input check.
   +   */
   +  protected function getUrlWithComplexQueryInputList() {
   

   We need to document return parameters.

7. +++ b/core/modules/link/tests/src/Functional/LinkFieldTest.php
   @@ -484,6 +487,192 @@ public function testLinkFormatter() {
   +        'inputByUser' => '?a[]=1&a[]=2',
   +        'title' => '?a[]=1&a[]=2',
   

   inputByUser and title always seem to be the same. Is the title value necessary. There's docs in the test method that suggest not.

Shouldn't this be higher priority? Seems like a pretty vital issue. Most sites will need to link to listing pages and this is duplicating every query string.

Patch #6 solved my issue with views filter query parameters getting mangled from /foo?bar[3]=3 to /foo?bar%5B0%5D=3&type%5B1%5D=3, but I wonder what consequences there are from removing the URL options.

@zniki.ru, can you address @alexpott's feedback for the tests?

I addressed everything pointed by alexpott except the correct number of permutations mentioned in 13.5 as I'm not sure.

Current MR !798 state works for me.

Removing "Needs test" tag.

Patch in comment #6 works for me. Not marking tested and reviewed as I have only tested in D 9.3.13

I've tried to apply patch #6 against my local Drupal 9.4.x install, but it fails to apply with the following errors:

error: patch failed: core/modules/link/tests/src/Functional/LinkFieldTest.php:136
error: core/modules/link/tests/src/Functional/LinkFieldTest.php: patch does not apply

Can anyone else please confirm if this patch works for them?

Can we get an updated issue summary please

What is the proposed solution and steps to reproduce?

Resetting all the keys in the #options array isn't the best idea; some settings may get lost (see also #3350477: Linkit for Link Field: "rel" and "target" settings are removed for internal links).
Since this issue is about duplicate query params, we should only reset the query part.

For some reason, my push to the issue fork didn't appear in the MR, therefore uploading patch.

Leaving in NW as per #36.

We're also experiencing this issue on a site where we're putting URLs to faceted searches into link fields.

The issue summary has been updated per #36. The status is still NW because comment 13.5 hasn't been addressed yet.

RE comment 13.5: 10 test cases is sort of correct. There are 10 display setting test cases that are tested for each URL:

1. trim_length = NULL
2. trim_length = 6
3. rel = NULL
4. rel = nofollow
5. target = NULL
6. target = _blank
7. url_only = FALSE
8. url_only = FALSE and url_plain = TRUE
9. url_only = TRUE
10. url_only = TRUEand url_plain = TRUE

So with 7 URLs there are 70 total permutations.

My solution was to remove the comment about there being 10 test cases. In the first place, the comment seems unnecessary. Secondly, it could easily get out of date if additional test cases are added. Let's leave it off.

Ran the tests only

Failed asserting that '\n
            <div>b3niWFPU</div>\n
      \n
      <div>\n
              <div><a href="?a%5B0%5D=1&amp;a%5B1%5D=2&amp;a%5B2%5D=1&amp;a%5B3%5D=2">?a[]=1&amp;a[]=2</a></div>\n
              <div><a href="?b%5B0%5D=1&amp;b%5B1%5D=2&amp;b%5B2%5D=1&amp;b%5B3%5D=2">?b[0]=1&amp;b[1]=2</a></div>\n
              <div><a href="?c%5B0%5D=1&amp;c%5B1%5D=2&amp;c%5B2%5D=1&amp;c%5B3%5D=2&amp;d=3">?c[]=1&amp;d=3&amp;c[]=2</a></div>\n
              <div><a href="?e%5Bf%5D%5Bg%5D=h">?e[f][g]=h</a></div>\n
              <div><a href="?i%5Bj%5Bk%5D=l">?i[j[k]]=l</a></div>\n
              <div><a href="?x=2">?x=1&amp;x=2</a></div>\n
              <div><a href="?z%5B0%5D=2&amp;z%5B1%5D=2">?z[0]=1&amp;z[0]=2</a></div>\n
          </div>\n
  ' contains "<a href="?a%5B0%5D=1&amp;a%5B1%5D=2">?a[]=1&amp;a[]=2</a>".

Which is good.

But following the steps in the issue summary I get /?a%5B0%5D=test no matter with or without the patch. So I'm not getting the same failure as mentioned. So issue summary may need to be updated

@smustgrave I have updated the issue summary with some clarification. The URL in the link's href attribute is what will be incorrect. If you don't enter a value into the optional link title, then the URL will be printed there and it will be correct. So you'll have to inspect the link destination. Please let me know if you didn't understand this part of the issue.

Sorry I wasn't clear in my original post but /?a%5B0%5D=test is the href value I get with or without the patch.

I don't know what the difference is that's causing our results to differ. I'm not doing anything special. I install Drupal 11 or 10, add a Link field with the default options to articles, create an article with a link that has the URL /?a[0]=test, then view the node. The duplication happens on both versions 11 and 10.

Here's a screen capture showing the bug: https://youtu.be/daWj49IyjHM. Maybe this will help.

Maybe if someone else could test it? I can't replicate the issue.

@smustgrave I will check this today.

I confirm that issue exists in latest stable Drupal 10 version. And Drupal 11.x dev.
@smustgrave did you disable markup caching at /admin/config/development/settings ?

Thanks a lot @mrshowerman for your comment #37.
I think we need to add comment to code base to make it clear.

Thank @dcam for your changes, I think it make sense to remove number of tests.

I ran into the same issue. I linked a query parameter in my menu which directs user to a selected facet and the double query parameter bug was causing two chips to display instead of one. In order to fix this I used a URL Redirect (302) which redirected to my query parameter and the double param bug did not happen. This may be a temporary solution for now but I did want to share.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I accidentally added unwanted changes to MR.
Great that we have Needs Review Queue Bot.
MR updated and waiting for review.

That MR is for 9.3.x and D9 is EOL. Needs to be for 11.x

Thanks @smustgrave
I created new branch and new MR https://git.drupalcode.org/project/drupal/-/merge_requests/5333 because I was not able to change target old MR.
Not sure if it was right move, let me know if there is better way to do it next time.

nope you did it right. Hiding the patch files for clarity.

Added a few questions to the MR.

@borisson_ thanks for review, I made changes to MR.
MR is waiting for new review round.

This looks great,

Left a couple of suggestions in the MR

Feel free to self-RTBC

@larowlan, thanks for your suggestions, I applied one suggestion, but I still think $options is required. Please check comment #37.

Seems to have a failing tests.

I believe tests are failling not because of changes in this MR.
I checked logs, and one time it was
Drupal\Tests\forum\Kernel\Migrate\d6\MigrateBlockTest #3393800: Kernel tests can't use path aliases on entities
other time it was
Drupal\Tests\Core\DependencyInjection\YamlFileLoaderTest

Looks like random fail, I rerun tests.

Tests passed after rebase to recent HEAD.

Leaning on @larowlan and @borission_ previous review. But appears feedback has been addressed and response to @larowlan for keeping $options.

Pushed the simplification I was advocating for in the MR thread

Left some small comments.

MR updated.

Feedback appears to be addressed.

Committed to 11.x
As this fixes a bug and there is little risk of disruption, backported to 10.2.x

Also opened #3418184: Refactor LinkFieldTest, convert some methods to a Kernel test as I think we can unpick some of that test into kernel tests.

Closed #3191456: Links with query params with integer keys reset the keys as a dup

Just calling out that the final implementation of this change, with the line

$element[$delta]['#attributes'] = $item->_attributes;

does appear to cause downstream problems for classes that extend the LinkFormatter class -- the problematic behavior mrshowerman warned of in https://www.drupal.org/project/drupal/issues/2885351#comment-14985734

In the case of the Linkit contrib module, this code change does unset the rel and _target parameters, as described in #3350477: Linkit for Link Field: "rel" and "target" settings are removed for internal links. The contrib module can fix it on that end, but I wanted to call out, for due diligence, that this possibly could cause issues with other classes that extend the LinkFormatter class. Thanks!

Thank you for making sure Drupal is secure and that bugs are fixed. I have some constructive feedback about this particular fix. Because this update breaks some theme functionality, I feel that it probably could have been included in a minor release instead of patch-level. I think better release notes and documentation were/are needed to help developers and site maintainers avoid problems. I see in other tickets that have spun off of this that suggested solutions have been provided, but because so many themes rely on the now-changed structure, it would be nice if more communication could happen so deploying the update doesn't break site functionality. But, it's understandable that perhaps the breakage was unforeseen or unanticipated. I wonder if the Drupal release notes could be updated with suggested remediations for this change? Not sure how to go about suggesting that, though, other than to mention it here. :-)

@paramnida, thanks a lot for your feedback. Sorry that this change affects you in this way. I also don't know if it possible to update CR.

I don't think the issue needs change record as behaviour was wrong and it's sad that someone is using wrong expectations.
Having CR for bug reports is useless cognitive load imo