If you visit a D8 site as an authenticated user and add a query string parameter which includes a quotation mark, then a JavaScript error will be thrown.

E.g. go to /search?keywords=it's

Error:
Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Document': '[data-drupal-link-system-path="search"]:not([hreflang])[data-drupal-link-query='{"keywords":"it's"}'],[data-drupal-link-system-path="search"][hreflang="en"][data-drupal-link-query='{"keywords":"it's"}']' is not a valid selector.

This does not effect anonymous users as the core/drupal.active-link library is only attached if the current user is authenticated.

CommentFileSizeAuthor
#40 2902769-40.patch664 bytesKeshav Patel
#39 active_link_js_throws-2902769-39.patch580 bytesDYdave
#35 2902769-nr-bot.txt107 bytesneeds-review-queue-bot
#34 active_link_js_throws-2902769-34.patch1.32 KBagarzola
#33 active_link_js_throws-2902769-33.patch668 bytesagarzola
#32 2902769-nr-bot.txt166 bytesneeds-review-queue-bot
#29 active_link_js_throws-2902769-29.patch1.33 KBagarzola
#27 interdiff_26-27.txt1.02 KBSuresh Prabhu Parkala
#27 2902769-27.patch1.45 KBSuresh Prabhu Parkala
#26 active_link_js_throws-2902769-26.patch1.45 KBdmitri.daranuta
#22 active_link_js_throws-2902769-22.patch1.36 KBamanire
#20 active_link_js_throws-2902769-20.patch1.36 KBamanire
#19 active_link_js_throws-2902769-19.patch1.39 KBamanire
#16 active_link_js_throws-2902769-16.patch1.45 KBivrh
#9 active_link_js_throws-2902769-5.patch1.42 KBkeeganstreet
#5 active_link_js_throws-2902769-5.patch1.42 KBkeeganstreet
#4 active_link_js_throws-2902769-4.patch683 byteskeeganstreet
#3 active_link_js_throws-2902769-3.patch1.49 KBkeeganstreet
#2 active_link_js_throws-2902769-2.patch701 byteskeeganstreet

Issue fork drupal-2902769

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

keeganstreet created an issue. See original summary.

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
FileSize
active_link_js_throws-2902769-2.patch701 bytes

This patch escapes single quote characters before passing the string on to be used as input for querySelectorAll.

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
FileSize
active_link_js_throws-2902769-3.patch1.49 KB

Patch for 8.4.x with ES6.

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
FileSize
active_link_js_throws-2902769-4.patch683 bytes

Previous patch throws error if drupalSettings.path.currentQuery is undefined. This is a new patch for 8.3.x.

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
FileSize
active_link_js_throws-2902769-5.patch1.42 KB

Previous 8.4.x patch throws error if drupalSettings.path.currentQuery is undefined. This is a new patch for 8.4.x.

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented
Version: 8.3.x-dev » 8.5.x-dev
Status: Active » Needs review

Hi: we do not expect any more 8.3.x commits except possibly of critical issues.

Status: Needs review » Needs work

The last submitted patch, 5: active_link_js_throws-2902769-5.patch, failed testing. View results

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
FileSize
active_link_js_throws-2902769-5.patch1.42 KB

That's fine @cilefen, active_link_js_throws-2902769-5.patch applies on 8.4.x and 8.5.x.

keeganstreet’s picture

keeganstreet CreditAttribution: keeganstreet at Deloitte Digital commented
cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented
Status: Needs work » Needs review
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs JavaScript testing

This is a quick fix to mute the error report but will not match the correct query string

The active link contains a bug. Any query string contains `'` or `\` [..more] doesn't match already..

Also,

node?a=1&b=2
node?b=2&a=1

the ordering affected the matching also. However, `node?a=1&b=2` and `node?b=2&a=1` is the same URL in W3C spec, right? (Drupal will give the same result I think)

Also, the [data-drupal-link-query] printed from PHP, we should keep the escape same way. It should not JSON IMO.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

This definitely needs JS test coverage.

And we need to keep it in sync with the PHP logic.

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.

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.

ivrh’s picture

ivrh CreditAttribution: ivrh commented
FileSize
active_link_js_throws-2902769-16.patch1.45 KB

The patch stopped applying for 8.6.2, so I am re-rolling it against 8.6.x branch

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.

amanire’s picture

amanire CreditAttribution: amanire as a volunteer and commented
Issue tags: -JavaScript +JavaScript
FileSize
active_link_js_throws-2902769-19.patch1.39 KB

Recognizing the concerns expressed in #12 and #13, but as an interim measure to workaround the errors, this is a reroll of #16 to apply against 8.8.2.

amanire’s picture

amanire CreditAttribution: amanire as a volunteer and commented
FileSize
active_link_js_throws-2902769-20.patch1.36 KB

That last reroll in #19 accidentally removed an extra line on active-link.js. Don't use it. Use this one instead.

amanire’s picture

amanire CreditAttribution: amanire as a volunteer and commented
amanire’s picture

amanire CreditAttribution: amanire at Environmental Defense Fund commented
FileSize
active_link_js_throws-2902769-22.patch1.36 KB

Ugh I think #20 has bad line endings. Again, recognizing the concerns expressed in #12 and #13, but as an interim measure to workaround the errors, this is a reroll of #16 to apply against 8.8.2.

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.

dmitri.daranuta’s picture

dmitri.daranuta CreditAttribution: dmitri.daranuta at FFW commented
FileSize
active_link_js_throws-2902769-26.patch1.45 KB
Suresh Prabhu Parkala’s picture

Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commented
Status: Needs work » Needs review
FileSize
2902769-27.patch1.45 KB
interdiff_26-27.txt1.02 KB

Tried to fix custom command failures. Please review.

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.

agarzola’s picture

agarzola CreditAttribution: agarzola at Chromatic commented
FileSize
active_link_js_throws-2902769-29.patch1.33 KB

I can confirm that the patch in #27 suppresses the error in 9.1.x. I’m attaching a re-rolled patch against 9.4.x with (I believe) the necessary changes to pass tests.

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.

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.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
2902769-nr-bot.txt166 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

agarzola’s picture

agarzola CreditAttribution: agarzola at Chromatic commented
Status: Needs work » Needs review
FileSize
active_link_js_throws-2902769-33.patch668 bytes

Re-rolled against the 10.1.x branch.

agarzola’s picture

agarzola CreditAttribution: agarzola at Chromatic commented
FileSize
active_link_js_throws-2902769-34.patch1.32 KB

Something caused the previous 9.x patch not to pass tests in 9.5. Tooling in 9.5 causes the non-ES6 version of the file to _not_ get blank lines around the conditional.

This new patch should apply cleanly to 9.5.x.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
FileSize
2902769-nr-bot.txt107 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs work » Needs review

Branch issue

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests, +Needs issue summary update

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

As a bug this could use a test case.

Also issue summary should be updated with proposed fix.

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.

DYdave’s picture

DYdave CreditAttribution: DYdave at Code Enigma commented
FileSize
active_link_js_throws-2902769-39.patch580 bytes

Rerolled #34 for 10.2.x.

File /core/misc/active-link.es6.js doesn't seem to exist anymore, so it was removed from the patch.

Please let us know if it moved somewhere and should still be included in the patch.

Thanks in advance !

Keshav Patel’s picture

Keshav Patel CreditAttribution: Keshav Patel at OpenSense Labs for DrupalFit commented
Assigned: keeganstreet » Unassigned
Status: Needs work » Needs review
FileSize
2902769-40.patch664 bytes

Changed 'const' to 'let', Please Review.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Keshav Patel opened merge request !7629