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

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

should that be a span instead of a div?

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.

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

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.

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

reviewed and tested

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!

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?

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.

That actually was part of what dissuaded me from looking more into the problem I was having on my site (comment #30)

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.

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.

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

The patch works.

Updating the version.

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.

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

The last submitted patch, identify-active-paths-41595-65.patch, failed testing.

The last submitted patch, identify-active-paths-41595-68.patch, failed testing.

The last submitted patch, identify-active-paths-41595-70.patch, failed testing.

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

Unassigning and tagging.

Does anyone care to benchmark this?

Re-rolled, needs review.

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.

Wow; elegant.

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

#82 nice and working

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

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

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.

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.

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

D6 backport.

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

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

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

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.

Adjusted the D6 backport identically.

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.

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.