active-link.js throws JS error if query string parameter contains a single quote

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

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.

This definitely needs JS test coverage.

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

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

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.

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

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.

Tried to fix custom command failures. Please review.

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.

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.

Re-rolled against the 10.1.x branch.

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.

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.

Branch issue

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.

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 !

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

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

#12 and #13 both point out that the approach of simply removing the apostrophe is not sufficient. It eliminates the JS error, but it doesn't result in the active link JS working properly as the query it builds will not match the correct link.

I debugged this for a while and here are some points that i observed:-
1)$generated_link = $link_generator->generate($element['#title'], $element['#url']->setOptions($options)); in
Link.php is called twice for the link.For the first time it get the wrong element without the data drupal selectors but it does not attach that to the markup(which is on the next line).For the second time when this is called again $element['url']->setOption('set_active_class', TRUE); sets the class to true which inturn sets the correct data attributes to the element and then attaches it to the mark-up in $element['#markup'] = $generated_link; in Link.php.
2)With the correct element set at the PHP side It still does not correctly render the element in the markup.

Just wanted to confirm if I am on the right track of figuring out what could be the issue?

Re #45
The way PHP renders the links and their attributes is fine. This was correctly reported as a JavaScript issue and that's where the solution will be.

The specific issue is JavaScript is not adequately finding those links when they use special characters. If the JS query string escapes special characters in the same way PHP does, then it will get a match and work properly.

Use the steps in #44 to make a link that will reproduce the problem as reported, and the solution is modifying the JS. In fact The solution will likely be modifying the same line as the prior patches, but instead of just removing special characters, it converts it to the encoding that allows it to match the expected link and add the "is active" class.

Conflicts with committed patch for #3464340: JS errors from Drupal.behaviors.activeLinks (... is not a valid selector)

The fix for the ticket mentioned in #47 fixes the issue mentioned in this ticket as well. I applied the changes of that ticket in Drupal 11.0.x and it fixed the js error mentioned here. I didn't verify that initially that’s why I added a commit into the MR here. Maybe someone else can also verify it once so moving it to Needs Review.

Attaching screenshots fas evidence.

Before:

After:

Please read the tags and pipeline status before putting into review.

This issue is not reproducible in Drupal 11.x. Verified it with fresh Drupal 11.x setup. Attaching SS for reference.

Please check my previous comment where I have mentioned the same #49. I think this ticket can be closed since it is already fixed along with https://www.drupal.org/project/drupal/issues/3464340. I moved it to Needs Review so that someone else can also verify it once.

Reroll the patch to version 10.4.2

The solution of #3508677: active-link.js doesn't support query arguments with an ampersand (and other characters)) also seems to work for this one

Trying to remove the usage of currentQuery altogether here #3620504: Stop relying on drupalSettings.path.currentQuery, read from window.location instead which should solve this issue here as well. I added test coverage into that one that covers special characters