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:
- Add a link field to the Article content type.
- Create a new Article node. In the article's link field enter a value with an internal URL like
/?a[0]=test
. - 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
Verify the number of test permutations as mentioned in comment 13.5.- 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',
Comment | File | Size | Author |
---|
Issue fork drupal-2885351
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:
Comments
Comment #2
clemens.tolboomComment #6
Nikolay ShapovalovProvide 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.
Comment #7
Nikolay ShapovalovIf you don't want to patch core.
You can use https://www.drupal.org/project/link_formatter_query_fix to fix this issue.
Comment #9
svendecabooterI had a similar issue when linking to a View with exposed filters.
The patch in #6 successfully resolves this issue.
Comment #10
ifrikPatch #6 works for me.
RTBCing it to get this moving a bit more.
Comment #12
alexpottTestbot had a hiccup re-rtbcing
Comment #13
alexpottI'm not sure what you mean by "three link field values". We seem to have 1 field with count($test_urls) number of values.
Is this true?
Why by reference? I'm not sure that that is necessary.
We have 7 * 7 permutations as far as I can see. Is this correct?
We need to document return parameters.
inputByUser and title always seem to be the same. Is the title value necessary. There's docs in the test method that suggest not.
Comment #15
Siavash CreditAttribution: Siavash commentedShouldn'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.
Comment #21
PapaGrandePatch #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?
Comment #22
larowlanComment #24
paulocsComment #26
paulocsComment #27
paulocsI addressed everything pointed by alexpott except the correct number of permutations mentioned in 13.5 as I'm not sure.
Comment #28
psf_ CreditAttribution: psf_ at Front ID commentedCurrent MR !798 state works for me.
Comment #29
paulocsRemoving "Needs test" tag.
Comment #32
cpierce42Patch in comment #6 works for me. Not marking tested and reviewed as I have only tested in D 9.3.13
Comment #33
tintoI've tried to apply patch #6 against my local Drupal 9.4.x install, but it fails to apply with the following errors:
Can anyone else please confirm if this patch works for them?
Comment #34
clemens.tolboom@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...
Comment #36
smustgrave CreditAttribution: smustgrave commentedCan we get an updated issue summary please
What is the proposed solution and steps to reproduce?
Comment #37
mrshowermanResetting 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.
Comment #39
dcam CreditAttribution: dcam as a volunteer commentedWe'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.
Comment #40
dcam CreditAttribution: dcam as a volunteer commentedRE comment 13.5: 10 test cases is sort of correct. There are 10 display setting test cases that are tested for each URL:
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.
Comment #41
smustgrave CreditAttribution: smustgrave commentedRan the tests only
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
Comment #42
dcam CreditAttribution: dcam as a volunteer commented@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.Comment #43
smustgrave CreditAttribution: smustgrave commentedSorry I wasn't clear in my original post but
/?a%5B0%5D=test
is the href value I get with or without the patch.Comment #44
dcam CreditAttribution: dcam as a volunteer commentedI 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.Comment #45
dcam CreditAttribution: dcam as a volunteer commentedHere's a screen capture showing the bug: https://youtu.be/daWj49IyjHM. Maybe this will help.
Comment #46
smustgrave CreditAttribution: smustgrave commentedMaybe if someone else could test it? I can't replicate the issue.
Comment #47
Nikolay Shapovalov@smustgrave I will check this today.
Comment #48
Nikolay ShapovalovI 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.
Comment #49
aopes12 CreditAttribution: aopes12 as a volunteer commentedI 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.
Comment #50
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #51
Nikolay ShapovalovI accidentally added unwanted changes to MR.
Great that we have Needs Review Queue Bot.
MR updated and waiting for review.
Comment #52
smustgrave CreditAttribution: smustgrave commentedThat MR is for 9.3.x and D9 is EOL. Needs to be for 11.x
Comment #54
Nikolay ShapovalovThanks @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.
Comment #55
smustgrave CreditAttribution: smustgrave at Mobomo commentednope you did it right. Hiding the patch files for clarity.
Comment #56
borisson_Added a few questions to the MR.
Comment #57
Nikolay Shapovalov@borisson_ thanks for review, I made changes to MR.
MR is waiting for new review round.
Comment #58
borisson_This looks great,
Comment #62
larowlanLeft a couple of suggestions in the MR
Feel free to self-RTBC
Comment #63
Nikolay Shapovalov@larowlan, thanks for your suggestions, I applied one suggestion, but I still think
$options
is required. Please check comment #37.Comment #64
smustgrave CreditAttribution: smustgrave commentedSeems to have a failing tests.
Comment #65
Nikolay ShapovalovI 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.
Comment #66
Nikolay ShapovalovTests passed after rebase to recent HEAD.
Comment #67
smustgrave CreditAttribution: smustgrave commentedLeaning on @larowlan and @borission_ previous review. But appears feedback has been addressed and response to @larowlan for keeping $options.
Comment #68
larowlanPushed the simplification I was advocating for in the MR thread
Comment #69
smustgrave CreditAttribution: smustgrave commentedLeft some small comments.
Comment #70
Nikolay ShapovalovMR updated.
Comment #71
smustgrave CreditAttribution: smustgrave commentedFeedback appears to be addressed.
Comment #75
larowlanCommitted 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.
Comment #78
smustgrave CreditAttribution: smustgrave at Mobomo commentedClosed #3191456: Links with query params with integer keys reset the keys as a dup
Comment #80
mark_fullmerJust calling out that the final implementation of this change, with the line
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!