Query string duplications

Resetting all the keys in the #options array isn't the best idea; some settings may get lost (see also 🐛 Add test coverage for rel and target settings Fixed ).
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 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.
@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,

larowlan → closed merge request !798

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

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

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

larowlan → closed merge request !5333

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

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

smustgrave → credited arisen → .

smustgrave → credited kporras07 → .

Closed 🐛 Links with query params with integer keys reset the keys Closed: duplicate as a dup

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

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 🐛 Query string duplications Fixed

In the case of the Linkit contrib module, this code change does unset the rel and _target parameters, as described in 🐛 Add test coverage for rel and target settings Fixed . 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, even within Drupal core, 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 🐛 Link attributes not working after upgrading Drupal core from 10.2.2 to 10.2.3 Closed: works as designed that have spun off of this that suggested solutions 💬 Field content['#options'] missing after upgrade to core 10.2.3 Closed: works as designed have been provided, but because so many themes relied on the now-changed structure, it would have been nice if more communication could have happened before users deploy a change like this that could break things. 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...

@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