All pager links have an 'active' CSS class

The "active" class is not set by the pager, but by by the function "l", which formats drupal links, when a link refers to the current page.

That's always the case with pager links (only the query part changes : "page=n".

This patch however proposes a "pager_current" class, that distinguishes the "currently viewed page".

It also removes the hard-coded "" tag, leaving the theme manage the appearence. (maybe is that too "violent ?)

sorry, beginner's mistake - the last line should read :
It also removes the hard-coded "strong" tag, leaving the theme manage the appearence. (maybe is that too "violent" ?)

Our handbook strongly encourages you to diff with:
cvs diff -u -F^f path-name.path

Notice that the difference is in the '-F^f' part... Just a little advise for your next patches... ;-)

Well it is indeed my first patch :-)

I'm on windows, using TortoiseCVS
Problem is i can't find where to specify correct parameters for the "create patch" function

I would even rather see all hardcoded HTML like div's, spans, whatever inside theme_* functions..

Steef

wtanaka : span instead of div ?
making it a div keeps it coherent with the way the other "page number links" are denoted :
div class="previous", div class="next" etc...
but maybe all of them should be spans after all ?

stefan nagtegaal : divs/spans/etc inside theme_* functions :
not sure i understand your point.
the code we're dealing with is inside theme_pager_list

I certainly lack technical and "historical" acquaintance with drupal core to have a definite opiniion about that...

what we call a 'drupal path' (say, 'node/54/edit') is in fact in the 'query' part of the actual matching url ('www.example.com/index.php?q=node/54/edit'), so it might be a little complicated...

and then again, maybe not ?

This is not a problem just with pager, but with all other links found in core that have parameters attached to their URL. I'm going to mark this one as a duplicate of the issue I just created as I have outlined what the problem is a bit clearer.

http://drupal.org/node/44502

A proposed fix would be a patch to l() because $_GET['q'] isn't compared to the parameters in the URL at all.

Allright.
I'll follow the other issue.

And i'll add a fresh one for my "strictly pager theming" patch - I still think the link for the current page should be accessible for css with an identificated div.

yched, I already have a patch that adds that to pager, along with cleaning up the rest of the markup that pager generates. http://drupal.org/node/44498

leave as duplicate for cross referencing

Great ! I didn't dare going through all this cleanup. Thanks !

@wtanaka, the reason I marked this issue as a duplicate of mine (and reversing what you did) was because the other issue *more* clearly defined the problem. By setting this one as the primary, it makes things more difficult since this thread does *not* clearly define the problem as the other one did. Additionally, the other thread was more concise and to the point. This helps *developers* review the problem much easier and faster. Please note this for future. As such, I don't have time to keep moving this issue back and forth and hence I'll leave it here and update accordingly.

As I was working on the pager in Drupal, I noticed that the l() adds class="active" to *all* of the pager links. This is because each of the links in pager.inc is either node?page=1 or node?page=2, etc...

In l() we have:

  if ($path == $_GET['q']) {
    if (isset($attributes['class'])) {
      $attributes['class'] .= ' active';
    }
    else {
      $attributes['class'] = 'active';
    }
  }

If you look above, $_GET['q'] will always be 'node' and hence, *all* pager links will be active. This is most certainly wrong.

We need to fix l() so that it also looks at the parameters in the URL, along with $_GET['q'] itself so it can apply class="active" appropriately. This problem is not just related to pager, but occurs elsewhere in core.

The attached patch fixes this but updating the comparision to make use of the URL parameters.

Just double checked, this query affects all sortable tables too. For example, go on the admin page and look at the source. You'll see that all pager links have class="active", all sortable table links have class="active" and in the admin menu, admin has class="active" too. Doh! This patch fixes that so all of these query links don't have class="active", only the admin menu item has class="active".

Better readability per chx's recommendation.

reviewed and tested

The patch currently doesn't abstract on query params order.
Two urls with the same query params but in a different order should be considered as the same url.

Or am I being too picky ?

I think too picky. I think in most cases in Drupal the arguments are always passed in the same order through the URL, I rarely see this changing and a patch to account for that would be overkill IMO. Simply comparing the path and the query fixes all of the problems I noticed in core so I think that should be sufficient.

wtanaka, that is a good catch, seems that problem only occurs when clean URLs are on, when they are off, that problem wasn't happening. I also discovered another problem, the converse of what you posted, in that if you were on a page with a query, such as /node?page=1 and had a link that was the same, it wasn't being marked as active either.

This new patch should fix both of these problems. Please confirm and set RTC, thanks!

+1 : This last patch works OK for me (I'm not using clean urls though)

A little advertisement here : I have proposed a patch on a related subject (node/49181).
It marks any link to a path that is in the current menu trail with the class ="active-trail", which I think is quite usefule for site-navigation awareness.

It only looks at the drupal path, and doesn't take the query part into account (just the way 'active' worked before your patch)
so in the exemples mentionned in the two previous posts, the links would still be marked 'active-trail' (which looks right to me : a page "with queries" can be seen as a child of the same page "without queries")

If I remember correctly, QUERY_STRING is a "Apache 2"-ism. It might not work on IIS, Apache 1, or any other webserver. If this is not the case, feel free to switch back the status.

This patch makes me nervous; l() is performance critical. This patch might significantly slow down sites that generate a lot of links.

Dries, I did searching and I don't think QUERY_STRING has any problems, the only problems associated with it are:

Note that $_SERVER['QUERY_STRING'] behaves differently under IIS/Apache.

In Apache (at least on Windows) it is ALWAYS set - if no query string was specified in the URL, $_SERVER['QUERY_STRING'] is initialised as an empty string.

In IIS, if no query string is included in the URL, $_SERVER['QUERY_STRING'] is NOT SET, so trying to access it without checking for its existence will generate notices.

And:

A note about the QUERY_STRING variable when using IIS:

I have found that IIS does not handle large query strings gracefully when passed from PHP. In addition to truncating them to around 1024 kb, I have seen IIS actually add data from other server variables to the end of the truncated data.

Both of which shouldn't affect anything at all.

Now, in terms of performance, I agree. However, right now the l() currently doesn't handle queries at all and wrongly applies "class='active'" which can be problematic to themers. However, code wise, it is minimal and substr() should be very fast with only knocking off the first 2 characters.

Perhaps somebody can benchmark this? I don't have great setup otherwise I would.

I can't reproduce this with the lastest HEAD. That patch works perfectly for me, with regular and clean URLs, even emptying cache on each of the page views.

Hmm, that shouldn't break it, should it? What if you disable that or test on another clean Drupal install?

Bump.
There hasn't been any news on the i18n issue ?
Maybe we can considered it fixed and commit that patch ?

m3avrck proposed me to work on this patch instead of http://drupal.org/node/53926.

I am this modifying a little bit the patch to take care of '' item also.

if (($path . (($query != '') ? '&' . $query : '')) == $_GET['q']) { my poor head hurts from reading this. Drupal has nice code. This is not nice. I'd daresay it's very ugly.

Steph, it seems that your patch "common.inc.patch.head" doesn't correct the problem I had there: http://drupal.org/node/53926 .
Am I right?
Are there in fact two different problems, or didn't I understand very well?

This bug are fixed in D5.x-dev..

This was never fixed and is still around in 7.x.

I found this issue trying to create a 6.x theme. All the pager links have class="active" set on them, so I can't theme them the way I want.

I'm setting it to version 8.x, as I know it won't go into 7.x, but I'll be working toward a 6.x version.

Attached is a patch that I believe addresses all concerns brought up in this thread related to the current subject (I did not address the original issue(s) or any of the linked to issues), although in three days of testing and retesting, it's possible I missed something.

The patch is against HEAD, which is still tagged as 7.0. I suspect I'll have to re-create it as soon as 7.0 becomes official and there's a real 8.x.

In order to completely address this, I needed to use both $_GET['q'] and $_SERVER['QUERY_STRING']. I couldn't find any other way to get access to all the correct parts w/o having to create a lot of additional code to remove the unneeded parts of the path.

Also, with the current status of core, I needed to introduce quite a bit more code ($query is now an array and part of $options). In order for that code to not be a burden on the overall function, I attempted to add tests which should fail out on the larger number of links before doing any real computations. I think it will reduce a lot (but probably not all) of unnecessary code execution. Hopefully, it is acceptable.

I'm resetting the version to see if the patch passes the test bot.

Trying to see if I can get the patch re-queued, since it seems to not be going anywhere.

Next step.

All right, never mind. I'll just wait for things to settle down. Sorry for the noise.

Adding a fix for the failed tests and a line I forgot to add in the previous patch. :^(

Testing...

The patch works.

The issue was stale for 4 years. This cannot be set to RTBC against 7.x within 3 days just on the basis that "it works".

- This could arguably be called an API change - the behavior of the l() function changes, and will impact existing sites out there
- l() is definitely in the critical path, so any change in there needs to be carefully scrutinized for performance impact. Benchmarks.

Updating the version.

#48: active_link_fix.patch queued for re-testing.

I do not believe that this is an API change but rather a correction of a long-standing problem. Ysched, could you perhaps elaborate on how you fear this behavior change will impact exiting sites negatively?

I do agree with you though that this needs benchmarks before such a frequently used function l() is changed.

One problem I note is that the patch as is does not account for the fact that the path that might use a language other than English. If $options['language']->language is set, it should be used as the second optional parameter to drupal_get_normal_path.

The function also looks like it can be sped up slightly through the storage of intermediate results in static variables. Hence, the $query and $current_page values need only be calculated once across many calls to l() on the same page.

The patch applies nicely to D8 & fixes the problem I identified in this duplicate bug #1170418: All Pagination Links Have Class class="active"

#48: active_link_fix.patch queued for re-testing.

Re-rolled with changes suggested in #55 by Lars Toomre.

Corrected.

Corrected.

Let's try removing one of the suggested changes.

Obviously, the original code identified some paths as active where the patched code does not. Let's combine both approaches.

Okay; let's try remove the caching, instead.

Looks like you might need to change a couple of the tests looking at #68's test results. Though #70 just looks like a bad bot. I'm going to try it again and see if that helps.

#70: identify-active-paths-41595-70.patch queued for re-testing.

Okay, this contains some unrelated changes, but it passes locally. Let's see what the testbot says.

Removing the unrelated changes.

Unassigning and tagging.

Does anyone care to benchmark this?

Thanks for your work on this patch. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch. Also, if anyone wants to help with benchmarking it, that would be cool too.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Re-rolled, needs review.

Thanks!

The original issue indeed was never fixed. The bug also exists in D6.

The suggested changes for l() in pretty much all patches of this issue are highly debatable and very likely have a serious negative impact on performance, since l() may be called very often in a single request.

We do not have many other areas in which links heavily rely on the 'active' class/styling and are using query strings like pager links anyway yet. This means we want to fix the precise bug in pager.inc first, and in a way that can be easily backported.

Let's resolve the generic issue in #1410574: Make l() respect the URL query string before adding the 'active' CSS class

Attached patch implements a test that proves that the bug exists. Fix forthcoming.

And the fix.

Amusingly, none of the pager links is supposed to be active at any time, so this could have been fixed very very easily ages ago, as in attached patch.

Wow; elegant.

Looks like a good solution to me.

Without patch in #82:

<ul class="pager">
  <li class="pager-current odd first">1</li>
  <li class="pager-item even">
    <a href="/node/1?page=1" title="Go to page 2" class="active">2</a>
  </li>
  <li class="pager-next odd">
    <a href="/node/1?page=1" title="Go to next page" class="active">next ›</a>
  </li>
  <li class="pager-last even last">
    <a href="/node/1?page=1" title="Go to last page" class="active">last »</a>
  </li>
</ul>

With patch in #82:

<ul class="pager">
  <li class="pager-current odd first">1</li>
  <li class="pager-item even">
    <a title="Go to page 2" href="/node/1?page=1">2</a>
  </li>
  <li class="pager-next odd">
    <a title="Go to next page" href="/node/1?page=1">next ›</a>
  </li>
  <li class="pager-last even last">
    <a title="Go to last page" href="/node/1?page=1">last »</a>
  </li>
</ul>

Also, patch looks good except for a new blank line at EOF.

The blank line at EOF is fine. Only the reverse (no newline at EOF diff warning) would be a reason to reject.

#82 nice and working

I wonder if for 8.x we should make l() smarter instead of trying to work around it?

@Dries: That's a separate issue: #1410574: Make l() respect the URL query string before adding the 'active' CSS class

Let's move this to 7.x then and focus on #1410574 for 8.x.

Well, it probably makes sense to keep the D7 and D8 code bases in sync while we work on a more optimal fix.

Yeah, I'd agree that #82 should go into D8 and D7 both now, and we can follow up with #1410574: Make l() respect the URL query string before adding the 'active' CSS class after.

My overly naive hope was to get this trivial fix into D8, D7, and also D6 for the new point releases happening tomorrow. :(

Even though it's utterly unlikely that this is going to happen anymore, here's the full stack of backports. All of them have been manually tested already.

Just to say I tested @sun's too patches in #93 against 7.10 & 6.22 and both applied nicely and provided the expected results.

Not sure if we'll get it in to tomorrow's release point, but..

NOTE: I should add that I didn't test the tests, just the results.

Yeah let's not leave 8.x with known regressions from Drupal 7, additionally we need the test coverage here, even if we're able to clean up the code in the follow-up (which already has a patch).

Committed/pushed to 8.x, moving back to 7.x.

Same as #93 for testbot.

- Tested on D7.12: works as supposed.
- Reviewed by comparing to the accepted D8 patch: no differences accept for occurrences of /core.

Committed and pushed to 7.x. Thanks! Marking down to 6.x.

D6 backport.

Re-uploading existing backport including tests from #93.

The test introduced by #96 (Pager functionality) fails on a tesbot instance, my MAMP localhost and also on btmash's machine.

8.x will have the same problem. Guess so far is that this might be related to Drupal being installed in a subdirectory.

+++ b/modules/simpletest/tests/pager.test
@@ -0,0 +1,159 @@
+  function testActiveClass() {
...
+    $this->drupalGet($GLOBALS['base_url'] . $elements[0]['href'], array('external' => TRUE));

So the href contains a potential subdirectory already, so this should have been $GLOBALS['base_root'] instead.

For now, I am submitting a D7 patch because apparently, there is not pager.test file for D8 :/

RTBC'ing D7 patch in #104, fixes the test on both localhost and testbot.

patch for D8 (includes pager.test from #96 and fix from #104).

I'm going to be marking 8.x as rtbc since this is basically a reroll of what was supposed to go in from #93, has the change from #104 , and the tests pass locally.

Great, thanks all! Committed/pushed #106 to D8, #104 to D7.

Back to 6.x for #100.

Adjusted the D6 backport identically.

#109 looks like a thorough backport (and the test coverage!).

Tagging for manual testing as well since I know Gábor prefers to see confirmation that the patch works well in production. Please test #109 on your D6 sites and report the results here in the issue.

I have applied the patch from #109 in a site running Drupal 6.20 and it works fine at home page, but not at any of admin pages. Later I saw the admin theme I was using (Rubik, based on Tao), has a custom theme_pager function which uses theme_links instead of theme_pager_link to render the pager links.

Also, the patch creates a new file modules/system/tests/pager.test. I don't know if test files like that are being used anywhere in D6.

Ya, sounds like the pager.test should be removed..

@danillonunes want to re-roll the patch for D6?

I don't see why we'd want to remove the test coverage. The test results for #109 nicely show that the test was executed and passes.

Good point.. Thanks @sun!

So this is fixed for all pagers, but what about any other links that have the same ?q, but different query parameters? They all get an active class. Is there an open issue for that?

#115 - l() has gone through a lot of changes between D6 and D8, so I'm reasonably sure that issue is fixed in the latest version at least.

I've attached a patch from simplytest.me showing that the active classes do not show up for links in pagers with this patch attached, so I'd say the manual testing has been done for this.

I was getting some whitespace errors when applying the patch locally, so I've marked it as "needs reroll".