I figured out a bug in the theme_links() function for menu links. I found the bug http://drupal.org/node/173459 and solved this bug the same way.

Repro:
1. Add a menu link with path <front> to your primary links
2. Add some more menu link, but this is not required - this is only to verify the bug
3. Click on the <front> link to get it active
4. See source what is marked active and you will see the link is marked 'active', but the <li> *not* (BUG).

This makes styling of the active LI element absolutely impossible. Please get this trivial patch into for 6.2... it bugs me any my theme users.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Needs review » Needs work

That bug was fixed by #173459: If <front> is added to menu, the link is never marked 'active' in DRUPAL-5, but was never ported to DRUPAL-6. Let's do not make that mistake again: bugs have to be fixed in HEAD, than back-ported.

Could you re-roll the same patch for HEAD?

hass’s picture

Status: Needs work » Needs review
FileSize
716 bytes

It haven't been documented, but the bug in #173459 is fixed in D6. This bug is new and related to LI only not the link.

See "common.inc":

  // Append active class.
  if ($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page())) {

Attached patch is rolled for D7, above for D6.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Hum, ok. Thanks for the clarification.

The above patchs only do implement the solution of #173459 in theme_links(), so marking this as ready to go in. Is there other parts of the core where this could be needed?

hass’s picture

I cannot say for sure if there is more broken... i only know #173459 is fixed and struggled on trying to style an LI missing the 'active' class. THX for review.

Gábor Hojtsy’s picture

damz: http://drupal.org/node/173459 you linked to (which adds an extra check into l()) is also fixed in Drupal 6, l() also includes that code there.

Committed to 6.x. Still needs to go to 7.x.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.