Front Page links are not marked as active in the l() function

'' is a user-facing construct that is more useful than a blank path since it is visible. It should be replaced with a blank path before it gets to l().

So this issue here is that <front> needs to be replaced before hitting that IF statement in l() so we don't have to add another case and slow down performance since l() is called so much. And, url() currently does the checking for <front>.

This is a decent issue that needs some insight before another patch. Thoughts?

Marked http://drupal.org/node/85204 as a dupe of this.

This issue http://drupal.org/node/65493 is somewhat related as well.

I encountered this too and for past year have been using a hack very similar to the first, but one which doesn't call drupal_is_front because a) unnecessary overhead? and b) that function wasn't always reliable as an indicator either.

Anyway, my fix was as follows:

common.inc, line 1119 (in 4.7.5), I changed the conditional
if ($path == $_GET['q']) {

to

  if ($path == $_GET['q'] || ($path == '<front>' && !isset($_REQUEST['q'])) ) {

This ensures that any links which have as their destination get the "active" attribute whenever somebody hits example.com. Hasn't failed me yet in about two dozen various installs. Note that I check _request, rather than _get, because some obscure and forgotten bit of documentation suggests that in some situations (possibly the whole drupal_get_destination stream) a request comes via post rather than get.

Testing welcome, patch attached.

Same patch removing two lines of cruft, sorry.

@chromeyellow: The original patch for this issue is the one we want as it calls drupal_is_front_page, your $_REQUEST['q'] call isn't the way to go unfortunately. Please review that one.

RobRoy:

Right you are! I tested the suggested fix and it works fine. Also came late to previous threads revolving around this issue, apologies.

(That said, the request-q method does avoid getting tangled in drupal_get_normal_path, with its attendant performance issues, and has never failed me...)

Anyway, +1 from me for this or any other working fix. Jakob Nielsen would not be impressed that Drupal currently fails to set the home link 'active' when user is on the home page. (Technically speaking it shouldn't even be a link at all, but that's another story...)

Works and fixes the problem.

As I said before, any code which passes '<front>' to l() is buggy. We should be fixing the root problems, not symptoms.

Well, function url() is also written to assume that <front> may be passed. From common.inc:1.611 line 1183:

  // The special path '<front>' links to the default front page.
  if (!empty($path) && $path != '<front>') {

In my case, I've added <front> to the primary links menu. I could submit a patch to menu_item_link() to replace <front> with '' before passing it to l(), but I'd suggest that would constitute fixing a symptom, not the root problem. We'd basically have to fix every invocation of l() because the API has specified for a while that you can pass <front> and it will work so we don't necessarily know everywhere it might occur.

What solution do you prefer?

This code works great to get the active item on the link itself. I know this is a seperate issue, but can anyone give pointers on how to get menu items to be activated as well? The active class would be assigned to the

<front> will always be a special case. I think it's hard to prevent it from reaching l(). There will always be code like

theme_my_link() {
  return l(t('My link'), variable_get('my_link', ''));
}

Small fuzz fixed.

Please review if you want to see this fixed in 6.x.

Why do you use $_REQUEST['q'] instead of $_GET['q']?

awaiting fix or reply to RobRoy's question.

#5

This ensures that any links which have as their destination get the "active" attribute whenever somebody hits example.com. Hasn't failed me yet in about two dozen various installs. Note that I check _request, rather than _get, because some obscure and forgotten bit of documentation suggests that in some situations (possibly the whole drupal_get_destination stream) a request comes via post rather than get.

If you insist, I can try to find that documentation tonight. I'm not sure if $path == $_GET['q'] should be changed while we're at it.

I proposed #5 with the request-q vs get-q thang.

I thought the question was made moot by robroy's indication that we were taking the drupal_is_front_page() route, as indicated in the FredCK's original patch?

Robroy, moshe - are you suggesting that we _not_ use drupal_is_front_page()?

If you'd rather use the simpler method suggested in #5... simply replace the $_request['q'] with $_get['q']. It was only an edge-case, back in 4.6, and I don't think that the edge-case exists now...

But the big question is... can we still push this to 6.x? Is anyone still against it?

I have re-rolled the patch for this issue. The new patch:

- documents url()'s existing behavior of accepting the special path '<front>',
- documents l()'s existing behavior of accepting the special path '<front>',
- fixes l() to conform to its documented specification of "allows themes to highlight links to the current page correctly."

drumm has objected that '<front>' should be handled elsewhere but that would constitute an API change (and IMHO an ill-advised one); url() and l() already accept '<front>', they just don't handle the 'active' class for it properly.

Others have complained about performance for this heavily used function but if the given path is not '<front>' the extra "if" condition only compares a single byte of $path (no other valid path starts with "<") and short-circuits the && so the call to drupal_is_front_page() is never made.

The previous patches on this thread that tests if $_GET['q'] is empty will not work because drupal_init_path() initializes $_GET['q'] = drupal_get_normal_path(variable_get('site_frontpage', 'node')) in that case.

This one-line trivial patch has been in the queue for over a year. RTBC.

I agree this solves an API implementation issue with the existing implementation and that <front> will end up with l() and url() especially now with the new menu system. Also, I agree that the check will only reach the $path comparision and will not call out if we are not on the front page. Committed.

Hi,
I think this issue is only partially fixed in Drupal 5.6

The active status is now set on the a element by l()

but it is still not set on the list item in theme_menu_links()

I added a phptemplate_menu_links() function to my theme which solves this

I just changed this

-if (stristr($index, 'active')) {
+if (stristr($index, 'active') || ($link['href'] == '<front>' && drupal_is_front_page())) { 

Sean Burlington
www.practicalweb.co.uk
London

This issue was fixed in 6.x and is probably never going to be fixed in 5.x.

OK

but the partial fix broke my design and I thought this might happen for others

not marking the frontpage tab on my primary menu as active was annoying but I could live with it

But I am using rounded corners on the tab so the a and li elements have background images - when only one of these had class=active I got tab background that didn't match!

Sean Burlington
www.practicalweb.co.uk
London

Hi Sean,

That worked great, thanks very much. I see no reason why this fix shouldn't be put in to version 5.x!