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.

CommentFileSizeAuthor
#3 common.inc_15.patch840 bytesm3avrck

Comments

Wesley Tanaka’s picture

Wesley Tanaka commented
Status: Active » Closed (duplicate)

http://drupal.org/node/41595

m3avrck’s picture

m3avrck commented
Status: Closed (duplicate) » Active

This will be the main thread now since the problem is more clearly defined above.

It seems to me the best patch would be to l() that compares: $path == $_GET['q'] // && all parameters in URL

One way to do this would be to compare as: $path == $_GET['q'] . $query but this doesn't work 100%. Will need to look into this more, don't have any more time at the moment.

m3avrck’s picture

m3avrck commented
Status: Active » Needs review
StatusFileSize
new common.inc_15.patch840 bytes

Here's a patch which fixes this problem.

Wesley Tanaka’s picture

Wesley Tanaka commented
Status: Needs review » Closed (duplicate)

fixing this bug and 41595 so that the earlier reported bug is the canonical one

landike’s picture

landike commented
Version: x.y.z » 9.x-dev
Issue summary: View changes

10 years ago was last comment. I worked with Drupal last time in 2010. And since that time nothing big changed.

This bug is reproduced in Drupal 6.13, 6.38 and I've seen code of common.inc in 7.50.

Original fix suggested in attached common.inc_15.patch did not help me - I still had active links, when the very first page is currently shown/selected/launched (but with no $query aka ?page=XYZ). Besides, I figured out, that my version of common.inc doesn't have $query passed, I used dedicated code for this.

And fortunately, I looked up to 9.x-dev codebase, and the structure of l() function gave me hint, how issue must be fixed.

So here is my piece of code, I applied to my old codebase. 

...

$query = drupal_query_string_encode($_GET, array('q'));

$isCurrent = ($path . (($query != '') ? '&' . $query : '')) == $_GET['q'];
$isFrontPage = ($path == '<front>' && drupal_is_front_page());
$noLang = empty($options['language']);
$langCheck = ($options['language']->language == $language->language);

if (($isCurrent || $isFrontPage) && ($noLang || $langCheck) && $query != '') {
   if (isset($options['attributes']['class']) && $options['attributes']['class'] != 'active') {
      $options['attributes']['class'] .= ' active';
   }
   else {
      $options['attributes']['class'] = 'active';
   } 
}
...

Not sure about that additional fix && $options['attributes']['class'] != 'active' to avoid double active classes to elements, which already have it. But for me fix works fine. I've seen in 9.x code, they have a better code out there.

That "over-conditional-like" logic is awful, I agree. But so far I need simple fix, to hide Drupal core bug on one of my production web sites.

I do really hope, that at least Drupal 8.x and 9.x for sure will not have such silly bugs.

landike’s picture

landike commented
Version: 9.x-dev » 6.38