When non-ASCII characters are used in the query string of a Url object, the link generator service fails to add an is-active class to the anchor tag.
This faulty behavior is demonstrated by the test-only changes in the MR. The MR modifies the core LinkGeneratorTest::testGenerateActive() test to demonstrate that this test passes when an ASCII query string is used but fails with a Cyrillic query string.
Post #8 proposed a change to the JavaScript that adds this class.
Fix the link generator service so that the tests pass. #8 needs to be put into the MR and reviewed, or another change needs to be proposed.
None.
None.
The constructor for \Drupal\Core\Url allows the user to pass in an array of key-value options as the third parameter.
These options are supposedly documented in the documentation for Url::fromUri() but in practice core sets and uses other options that are not documented there.
Specifically, the link generator service recognizes and uses the set_active_class option. This option is documented in core/includes/theme.inc in the documentation for template_preprocess_links() (deprecated, moved to an OO hook \Drupal\Core\Theme\ThemePreprocess::preprocessLinks).
- set_active_class: (optional) Whether each link should compare the
route_name + route_parameters or URL (path), language, and query options
to the current URL, to determine whether the link is "active". If so,
attributes will be added to the HTML elements for both the link and the
list item that contains it, which will result in an "is-active" class
being added to both. The class is added via JavaScript for authenticated
users (in the active-link library), and via PHP for anonymous users (in
the \Drupal\Core\EventSubscriber\ActiveLinkResponseFilter class).
There are also core test cases in \Drupal\Tests\Core\Utility\LinkGnereatorTest to ensure this option does the right thing.
Code:
\Drupal::service('link_generator')->generate($title, Url::fromRoute('yarcom_sphinx.search_type', array('search_type' => $t), array(
'set_active_class' => TRUE,
'query' => array(
'q' => $i['query'],
),
)));
If $i['query'] is russian characters, css class is-active does not appear.
Example 1:
$i['query'] = 'комацу' (russian characters).
Browser address bar: http://dev.******.ru/search/yarcom_news?q=%D0%BA%D0%BE%D0%BC%D0%B0%D1%86...
Html code:
<a href="/search/yarcom_news?q=%D0%BA%D0%BE%D0%BC%D0%B0%D1%86%D1%83" data-drupal-link-query="{"q":"\u043a\u043e\u043c\u0430\u0446\u0443"}" data-drupal-link-system-path="search/yarcom_news">News entity</a>
Example 2:
$i['query'] = 'komatsu' (english characters).
Browser address bar: http://dev.*****.ru/search/yarcom_news?q=komatsu
Html code:
<a <strong>class="is-active"</strong> href="/search/yarcom_news?q=komatsu" data-drupal-link-query="{"q":"komatsu"}" data-drupal-link-system-path="search/yarcom_news">News entity</a>
In the first example q=%D0%BA%D0%BE%D0%BC%D0%B0%D1%86%D1%83" and data-drupal-link-query="{"q":"\u043a\u043e\u043c\u0430\u0446\u0443"}" are not identical.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes
Comments
Comment #1
cilefen commentedComment #2
dawehner.
Comment #3
tr commentedкомацу must be urlencoded to put it in a URL.
%D0%BA%D0%BE%D0%BC%D0%B0%D1%86%D1%83 is the correct urlencoding.
Browsers will display the Cyrillic characters instead of the urlencoded characters, but if you look at your server logs you will see that when you type http://dev.******.ru/search/yarcom_news?q=комацу into your brower
the server actually receives http://dev.******.ru/search/yarcom_news?q=%D0%BA%D0%BE%D0%BC%D0%B0%D1%86...
The string \u043a\u043e\u043c\u0430\u0446\u0443 is also equal to комацу, but it is written with Unicode escape characters instead of urlencoding. So I don't see anything wrong with Example 1.
The only bug I see here is "If $i['query'] is russian characters, css class is-active does not appear.". I have not tried to reproduce that yet.
Comment #4
Sky Cat commentedThe problem is just that the class is not set "is-active" if the query contains the Russian characters.
Comment #5
tr commentedYes, that's what I said:
Comment #6
dawehnerThis problem is not a routing problem ....
Comment #7
pwolanin commentedpossibly related to #2809789: UrlGenerator fails to urlencode aliases, incorrectly encodes "system" path but if this is about the search query string this is specific to search module not system module?
Does this bug still occur? Please set back to active if it does (or post a test patch)
Comment #8
Sky Cat commentedThe problem in function system_js_settings_alter(), file - system.module.
When the $current_query->q duplicate $current_path, in the file active-link.js variable takes the value path.currentQuery.
The array activeLinks becomes empty.
Current query:
'q' => string(17) "weather/yaroslavl"Current path:
string(17) "weather/yaroslavl"misc/active-link.js:
var activeLinks = context.querySelectorAll(selectors.join(','));is empty.
Problem in string
var querySelector = path.currentQuery ? "[data-drupal-link-query='" + queryString + "']" : ':not([data-drupal-link-query])';If the code is changed to
class is-active will be added
Comment #9
pwolanin commentedPlease confirm that the bug exists in 8.3.x
I'd need to look at the JS but the proposed fix looks simple - can you make that as a patch?
Comment #17
pameeela commented@Sky Cat thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.
As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)".
Are you able to advise whether this issue is still occurring? If so, are you able to post a patch with the change you proposed in #8?
Comment #18
tr commentedThis is still an issue, and it's easy to confirm - maybe this 5-year old bug and all the others like it can actually get help and contribution from the community, instead of being shut down for lack of activity?
Here are two trivial patches to demonstrate the problem. They both change the core LinkGeneratorTest.
The first patch changes a test case so that the query parameter is 'комацу' - this test case does exactly what is shown in the original post, and this test case will now fail using Cyrillic characters in the query string.
The second patch changes that same test case in the same manner, this time using the query parameter 'komatsu' - this test case also does exactly what is shown in the original post, but this time it passes because ASCII characters were used.
I present two patches so you can see that the test failure with the first patch was not due to me modifying the test the wrong way - the only difference between the two patches is that one uses 'комацу' and the other uses 'komatsu'.
Oh, and before someone complains about the patch looking bad in the browser, that's because Drupal.org STILL serves up patch files using a text encoding of 'windows-1252', which doesn't properly display Russian characters like this. But if you download the patch and look at it you will see it is correct. This was reported three years ago in #2922638: No charset on response for patch & text files
Comment #19
pameeela commented@TR thanks for confirming. I’m sure you know that the best way to get this resolved is to post a patch with a fix.
If you or someone else posts a patch, I will try to expedite review via the bug smash initiative.
I am not sure exactly what you mean but I assure you that no one is trying to shut down issues. I am glad that by following up on this ticket that we are now more likely to progress it thanks to the info and test patches you have posted.
Comment #20
larowlanComment #21
pameeela commentedComment #28
mstrelan commentedThis needs an issue summary update, let's start with the standard issue template. I think it's also not clear from the title what is the bug that needs fixing, i.e. the
is-activeclass is not added. Let's get 2525830-18-комацу.patch converted to an MR so we can see the test fail, then we can work on a fix for this.Comment #30
tr commentedTest-only patch from #18 still applies. The test results in #18 demonstrated the bug for many years, until patch testing got turned off.
Here is the patch again in a merge request.
Comment #31
mstrelan commentedI'm not disputing that, just trying to help make it easier for anyone interested in picking this up. Without this it's hard to know at a glance if anything has changed in core that might have fixed this over the last 5 years.
Comment #32
tr commentedUpdated issue summary.
Comment #33
mstrelan commentedThanks for updating this. I'm still confused about the steps to reproduce. If I understand correctly from #3 and #8, the HTML output is correct and the JS needs to be updated. The proposed resolution is to make the Unit test pass, but that's contradictory to the above. We probably need a FunctionalJavascript test instead. I note that we now have
\Drupal\Tests\system\FunctionalJavascript\ActiveLinkTestwhich seems like a good place to test this. I tried to follow the suggestion in #8 but active-link.js has changed quite a bit so it wasn't straight forward.