Problem/Motivation

When an internal URL that contains an array query parameter is entered into a core Link field, those query parameters are duplicated when rendered. For example an entered value of /search?f[0]=test:facet would be rendered as /search?f[0]=test:facet&f[1]=test:facet (the characters are decoded for readability).

The value of the URL does not change. This only affects the rendered HTML.

This issue does not seem to affect external URLs.

Steps to reproduce

Here are the steps to reproduce this issue on the vanilla standard profile:

  1. Add a link field to the Article content type.
  2. Create a new Article node. In the article's link field enter a value with an internal URL like /?a[0]=test.
  3. View the new article node.

Expected result: The rendered link will have an href attribute containing the URL /?a[0]=test.
Actual result: The rendered link has an href attribute containing the URL /?a[0]=test&a[1]=test.

Of course, the actual URLs will have encoded query strings. Note that if you do not enter a value for the link field's title, then the title will be the correct URL, but the href will be incorrect.

MRs

MR 5333 is for 11.x
MR 798 can be closed

Proposed resolution

Unset the query key from the URL options array before rendering.

Remaining tasks

  1. Verify the number of test permutations as mentioned in comment 13.5.
  2. Review

User interface changes

API changes

Data model changes

Original report

Checking D7 Link #2333119: Output broken when using array parameters in query on D8 there are some issues with array query parameters.

- '?a[]=0&b[]=0&b[]=1'
- '?a[0]=0&b[0]=0&b[1]=1',

their link when viewing are rendered (URL encoding removed for readability) with duplicated content.

a[0]=0&a[1]=0&b[0]=0&b[1]=1&b[2]=0&b[3]=1

The title rendering is unreadable including for other tests (which is probably a different issue?)

- '?filter[a][b]=c',
- '?a[b[c]]=d',

Issue fork drupal-2885351

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom created an issue. See original summary.

clemens.tolboom’s picture

The last submitted patch, 2: query_string_duplications-2885351-2-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 2: query_string_duplications-2885351-2-dup-fix.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Nikolay Shapovalov’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

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.

Nikolay Shapovalov’s picture

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

svendecabooter’s picture

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

ifrik’s picture

Status: Needs review » Reviewed & tested by the community

Patch #6 works for me.

RTBCing it to get this moving a bit more.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: query_string_duplications-2885351-6.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Testbot had a hiccup re-rtbcing

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Siavash’s picture

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.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

PapaGrande’s picture

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?

larowlan’s picture

paulocs made their first commit to this issue’s fork.

paulocs’s picture

Assigned: Unassigned » paulocs

paulocs’s picture

Assigned: paulocs » Unassigned
paulocs’s picture

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

psf_’s picture

Status: Needs work » Needs review

Current MR !798 state works for me.

paulocs’s picture

Issue tags: -Needs tests

Removing "Needs test" tag.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

cpierce42’s picture

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

tinto’s picture

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?

clemens.tolboom’s picture

@tinto (and @cpierce42) you should not check #6 but the new "Issue fork drupal-2885351" just under the file list.

You can follow the 'About issue forks' just above #1 leading to https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Can we get an updated issue summary please

What is the proposed solution and steps to reproduce?

mrshowerman’s picture

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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dcam’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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.

dcam’s picture

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.

smustgrave’s picture

Status: Needs review » Needs work

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

dcam’s picture

Issue summary: View changes
Status: Needs work » Needs review

@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.

smustgrave’s picture

Status: Needs review » Needs work

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.

dcam’s picture

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.

dcam’s picture

Status: Needs work » Needs review

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

smustgrave’s picture

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

Nikolay Shapovalov’s picture

@smustgrave I will check this today.

Nikolay Shapovalov’s picture

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.

aopes12’s picture

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.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
2.12 KB

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.

Nikolay Shapovalov’s picture

Status: Needs work » Needs review

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

smustgrave’s picture

Status: Needs review » Needs work

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

Nikolay Shapovalov’s picture

Status: Needs work » Needs review

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.

smustgrave’s picture

Issue summary: View changes

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

borisson_’s picture

Added a few questions to the MR.

Nikolay Shapovalov’s picture

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

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This looks great,

larowlan changed the visibility of the branch 11.x to hidden.

larowlan changed the visibility of the branch 10.2.x to hidden.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Left a couple of suggestions in the MR

Feel free to self-RTBC

Nikolay Shapovalov’s picture

Status: Needs work » Needs review

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

smustgrave’s picture

Status: Needs review » Needs work

Seems to have a failing tests.

Nikolay Shapovalov’s picture

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.

Nikolay Shapovalov’s picture

Status: Needs work » Needs review

Tests passed after rebase to recent HEAD.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Pushed the simplification I was advocating for in the MR thread

smustgrave’s picture

Status: Needs review » Needs work

Left some small comments.

Nikolay Shapovalov’s picture

Status: Needs work » Needs review

MR updated.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

  • larowlan committed 7dcd798e on 10.2.x
    Issue #2885351 by Nikolay Shapovalov, paulocs, clemens.tolboom, larowlan...

  • larowlan committed 56e7439d on 11.x
    Issue #2885351 by Nikolay Shapovalov, paulocs, clemens.tolboom, larowlan...

larowlan’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

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

Also opened #3418184: Attempt to move large parts of LinkFieldTest to a kernel test as I think we can unpick some of that test into kernel tests.

smustgrave credited arisen.

smustgrave’s picture

Status: Fixed » Closed (fixed)

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

mark_fullmer’s picture

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!