Make l() respect the URL query string before adding the 'active' CSS class

This patch takes care of the issue at hand, however the tests will not pass because siimpletest plays with the querystring in such a way that I cannot seem to get the existing testLActiveClass() and testLCustomClass() tests to pass.

Any advice is welcome...

- Considering the performance sensitivity of this function, I would suggest to only look at the query part if the path part is equal to the current path. That is: move the code introduced by this patch into an if.

- Can the front page have a query part? I guess yes, as even the default front page uses a pager (if I'm correct, I've never used the default front page so far). If so, the query part should also be taken into account for the front page case. Combined with my previous remark, I think that this results in moving the new code into the already existing if.

+  $parsed_path = parse_url(check_plain(url($path, $options)));
+  if (isset($parsed_path['query'])) {
+    parse_str(str_replace('&', '&', $parsed_path['query']), $path_query);
+  }
+  else {
+    $path_query = array();
+  }

- Why call url(), check_plain(), parse_url(), array check, array access, str_replace, and parse_str, to get what is directly available in $options['query'], though as an array? So it seems better to compare $options['query'] with parse_str($_SERVER['QUERY_STRING']). Please add a comment that we must use == (same key/values) and not === (same key/values, same order), and that we neither can use http_build_query($options['query']) === $_SERVER['QUERY_STRING'] for the same reason (being: different order in query is still the same resource).
Note: not passing in a query part (to the l() function) in the $options array but in the $path parameter is at your own risk and you're self responsible for encoding and such. So, IMO that is a case that we don't have to cater for here and thus this optimization is possible. Moreover, a (the only?) valid use case for passing in the query part in the $path is external URL's and they never equal the current page...

I think that in the resulting patch, any performance implications won't be on the critical performance path anymore, and thus this patch can get in more easily.

I think that links to fragments in the page should also be excluded from getting the active class. Example: the "Add new comment" link to the reaction form at the end of a page (where comments are enabled). If others agree, we should change the title to something like:
Make l() respect the URL query string and fragment part before adding the 'active' CSS class

Code wise it will become only just a bit more difficult. Just check if there is a fragment entry in the $options array and if so do not add active. I think that the function documentation should be extended with a more elaborate description of what the "active" class on a link conceptually indicates.

This is just a test, I disregarded the patch in #1 for now.

How about this?

This works, but needs a test case for when you're on a page with a query string, which I guess will need a custom module?

Okay, added a full test case fixed above failures.

Glad to see that I was kinda close on the right way to accomplish this. #11 is much cleaner and I just gave it a test drive (while watching the NYC Camp keynotes). Everything looks great.

Even better!

I'm removing the last assertion of UrlTest::testLCustomClass() because it duplicates UrlTest::testLActiveClass() and is also wrong.

#14: drupal-1410574-14-combined.patch queued for re-testing.

Apparently request()->query->all() doesn't work on the testbot?
WFM locally...

Okay, $_GET is still used in plenty of places in core, and Crell says that request() is a temporary function anyway. Might as well stick with what works.

I guess the 'API change' tag makes this not backportable to D7? :(

But now there are no added function calls, so the performance isn't a problem anymore.

Well we can have that conversation with David_Rothstein at the proper time. I know I need this fix for a D7 site with links to different Search API filters all showing as active.

Just rerolled patch from #21.

Looks good to me.

+++ b/core/includes/common.incundefined
@@ -2298,13 +2298,20 @@ function l($text, $path, array $options = array()) {
+    if ($_GET == $options['query']) {

I don't know if we should still be using $_GET here

I'm not sure that $_GET is reliable anymore now that we are sometimes firing subrequests. For now, the alternative is fortunately simple:

drupal_container()->get('request');

We can convert it to something purdy and injected later, but at least for now we should be accessing the request.

There are 3 conditions that have to be met to get the active class (path, language, query):
- Only the 3rd is extensively documented.
- The first 2 are joined in 1 if, the 3rd has its own (inner) if
So IMO it would be better to join all conditions in 1 if, with 1 extensive comment before that code that (shortly) explains the conditions to be met. OR: create 3 separate if statements with 1 comment line each.

Do we need to link the @todo's by also referring to this todo in the index.php as well? (or are there other means to take care of the @todo)

Great, awaiting the test results ("test request sent" at the moment of writing), but for me this is RTBC.

The test results are in, it has been RTBC before, so for me good enough to move to RTBC again.

Looks good. Committed/pushed to 8.x.

Tried to backport it to D7. This patch is a mix of:
- #39
- #29 (as $_GET has to be used in D7)
- Undoing the changes from #41595-16: All pager links have an 'active' CSS class (as #39 also already did) that solved the problem for theme_pager_link() only)
The tests from #41595: All pager links have an 'active' CSS class, which are part of the test collection, should show that this patch is also working fine. I am not sure if more tests are needed.

I agree with #41595-55: All pager links have an 'active' CSS class that states that this is not an API change.

- $_GET does contain 'q' in D7, so remove that in a copy of $_GET before comparing. This repaired most of the failing tests.
- The changes in testLActiveClass() from #39 are now also back-ported as in the current situation it tests without taking the query into account.

Works locally and does not seem to be related ...

#45: 1410574-45.patch queued for re-testing.

$_GET does contain 'q' in D7

That doesnt sound right...

re #49: That is how Drupal (until now) works: clean URLs are "rewritten" to index.php?q=..., see this code in bootstrap.inc:

function drupal_environment_initialize() {
  ...
  // When clean URLs are enabled, emulate ?q=foo/bar ...
  $_GET['q'] = request_path();
  ...
}

So clean URLs or not, the q parameter will always be there.

blarg ... read that as "doesnt"

Ignore me

Encountered this issue and it seems like one that definitely should be fixed. Patch #45 doesn't apply to 7.23 cleanly so I haven't had a chance to test yet.

Re-rolled against latest core dev. Applies cleanly to 7.24.

+1 for this patch.

+++ b/modules/simpletest/tests/common_test.module
+++ b/modules/simpletest/tests/common_test.module
@@ -52,6 +52,12 @@ function common_test_menu() {

@@ -52,6 +52,12 @@ function common_test_menu() {
     'access arguments' => array('access content'),
     'type' => MENU_CALLBACK,
   );
+  $items['common-test/l-active-class'] = array(
+    'title' => 'Test l() adding of active class',
+    'page callback' => 'common_test_l_active_class',
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
   return $items;
 }

hook_menu is not long for this world. MENU_CALLBACK type requests should have been using the new router exclusively for months now. Please do not add more hook_menu entries.

@Crell can you please link to some documentation that explains this? I'm still seeing MENU_CALLBACK documentation for both 7.x and 8.x (although I have heard that hook_menu is dying in 8.x). Since this is a D7 patch though I don't quite follow your comments?

@acbramley: Documentation is in the WSCCI Conversion Guide. The change notice was posted in October 2012 at https://drupal.org/node/1800686. I found a simple example at http://drupal.stackexchange.com/a/80714/226.

No Crell and Marc, this was committed.

acbramley they are talking about d8 but the issue is for d7, carry on.

Thanks Tim, I stand corrected. I was just trying to answer #55 but I didn't pay attention to the context. Excuse me acbramley for the confusion.

D'Oh! I missed the version tag, sorry. Ignore me!

I can confirm the patch on #53 correctly adds the active class to links with query parameters when the url matches

53: 1410574-53.patch queued for re-testing.

Just a note. The patch in #53 almost works for D6. Just some minor variable changes.

$is_active = ($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page()));
  $is_active = $is_active && (empty($options['language']) || $options['language']->language == $language_->language);
  if ($is_active) {
    $real_query_params = $_GET;
    unset($real_query_params['q']);
    $is_active = $real_query_params == $options['query'];
  }
  if ($is_active) {

Patch still applies nicely to the git repo.

Tested it with this on SimplyTest.me:

<?php 
echo l('external link', 'http://drupal.org') . "<br>"; 
echo l('active link', 'node/1');
?>

I don't know that it needs to be any more complicated than that. The active class appears as expected.

This is tricky because sometimes query strings meaningfully change the content of the page and sometimes they don't.

For example, if you have a View with exposed filters, this patch results in the menu item for that view losing the "active" class when the filters are selected (though "active-trail" is still there). Depending on how the theme uses those classes, this could really lead to undesired behavior.

I think we need to discuss the backport a bit more. Would it make sense to consider adding a new class for these situations but keeping the behavior of "active" the same as it is now?

+    // The query parameters of the current request are in $_GET, but the 'q'
+    // parameter in there should be ignored, as that is the Drupal path.
+    $real_query_params = $_GET;
+    unset($real_query_params['q']);
+    $is_active = $real_query_params == $options['query'];
+  }

This looks a little buggy; it cares about the order of the elements in $options['query'] and $real_query_params being the same (not just their contents), but the order shouldn't matter.

-  if (($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page())) &&
....
+  $is_active = ($path == current_path() || ($path == '<front>' && drupal_is_front_page()));

Is it possible $_GET['q'] was used over current_path() here to micro-optimize performance in this heavily called function? Doesn't look like the patch needed to change this if() statement at all, anyway.

I would love to have this in. This is a real bugger when using search to make listings. For example, one could have an overview with articles by pointing to search?type%3Aarticle and another overview with news items by pointing to search?type%3Anews. If both these overviews are linked to from the main menu, both will get the active class.

Shouldn't the status be "fixed"? Or am I missing something?

#70: not yet fixed in 7.x?