front page check not reliable

What did you set to be the home page in admin/settings?

What is the behavior that you desire in this situation? Should drupal not show the breadcrumb if you are on the home page regardless of the setting in admin/settings?

This seems like a "feature request" to me, so I'm going to move it there - also moving to cvs since it seems more likely to get handled in HEAD than in the 4.7 set.

Also, in case you didn't already know, it is possible to achieve this in your installation by using the is_front or $page == 0 methods inside of your theme

http://drupal.org/node/39891

We are suffering from a similar problem on Drupal.org. The main page shows a "Home" breadcrumb, even though we use the default front page of "node".

I had to add this to the theme to hide it:
print !$is_front ? $breadcrumb : '';

Approach 1.

Approach 2.

Thanks
-K

This issue is similar to http://drupal.org/node/58065. A patch is uploaded their.

@ Approach 1
It only works with url aliases if the default front page is different to "node".

I suggest a 3rd approach: instead of changing the breadcrumb at the menu-system level, we can do the front page checking at the theme level. This is how I've fixed up this problem on my own sites, using a custom mytheme_page() function to override phptemplate_page(). This means that if people DO want the 'home' breadcrumb on their front page (for some strange reason), they can still do this by overriding the behaviour back to the old way in their theme.

Well,

the code seems fine and the patch work. I also reviewed http://drupal.org/node/58065 which do could also be a solution, but i don't think i can decide which patch to take.

drupal_get_breadcrumb() should return the correct breadcrumbs (or no breadcrumbs when the front page is rendered). It is not the theme's task to fix up the breadcrumbs. If it is the theme's task to fix up the result returned by Drupal's APIs, something is wrong with those APIs.

All these patchess here patch the symptom, not the cause. The hack for drupal.org was just a temporary fix. AFAIK it was introduced by the change to let suggested menu items show up in the breadcrumb.

IMO this bug actually has nothing to do with the MENU_SUGGESTED_ITEM patch. That patch only brought this issue to light as the 'node' page uses the afore-mentioned menu item. Promoting any other page (with normal or other menu items for breadcrumbs) to the front page should exhibit the same behaviour.

And I believe that this should be fixed in the menu system rather than in the theme.

Can you please review the patch in #7? Besides the introduction of another global, it works well and also saves some cycles in phptemplate.engine and menu.inc (which are triggered on every page view).

Thanks
-K

I'd vote against the introduction and use of a global variable ($is_front). A function, drupal_is_front_page(), sounds like the better option.

Function added. I'd also missed another is_front block in phptemplate.engine in my last patch. Fixed that as well.

Thanks
-K

in phptemplate, $variables['is_front'] = drupal_is_front_page(); would like better and simpler, don't you think so? Nice work, otherwise.

The fact that drupal_is_front_page() takes a boolean parameter is awkward. You're using a function as a global variable; that only makes it worse IMO.

Can't the function body embed the proper logic?

chx: fixed - thanks :)

Dries: The logic as such is already exercised in drupal_init_path(), which checks if $_GET['q'] is empty. We can't do the same check in drupal_is_front_page() because drupal_init_path() updates/mangles $_GET['q] with the path to the front page. Therefore, the only way to check this would be as in the current implementation with drupal_get_path_alias ( #6 ), which IMO is wasteful.

The drupal_get_path_alias call also makes a call to drupal_lookup_path and custom_url_rewrite and any associated DB calls. Drupal checks if a page is the front page about 6 times on every page view.

Thanks
-K

Same patch but with globals.

-K

Just to clarify, I prefer the function approach. And as chx pointed out to me, base_path() works on a similar basis.

Thanks
-K

Tested on WAMP with PHP5, breadcrum was removed from the home page.

Code looks good.

+1, ready to commit.

In this code snippet from patched drupal_init_path()

  if (!empty($_GET['q'])) {
     $_GET['q'] = drupal_get_normal_path(trim($_GET['q'], '/'));
    $is_front = FALSE;
  }

if $_GET['q'] was set to the same path that is defined to be the frontpage, then you're in the frontpage, but $is_front would be FALSE.

I might be wrong, if so, please just set back to RTBC.

Markus: That IMO is by design (and is also existing and expected behaviour). The pages can possibly be vastly different in both presentation and structure.

Setting it back to RTBC.

Thanks,
-K

Tested, it works nicely. +1

(I can't believe so many people write '+ 1' just because the patch works.)

drupal_lookup_path() uses caching, right? It should be fast ... (not benchmarked)

Well... I can't claim to understand some of the more complicated stuff that's being debated, but I know that the patch solved a problem that was appearing on a number of my under-development sites without breaking anything else, as far as I can tell. Could be wrong, obviously, but it appears to work and do exactly what it's supposed to do. For ME, that's a +1, but if someone else sees problems that break other stuff, I'll gladly retract. :)

Attached patch introduces/retains a bug: pages promoted to the front page will lost their breadcrumbs as well. So, if I promote 'taxonomy/term/1+2' to the front page, I will not see a breadcrumb on both the frontpage and the 'taxonomy/term/1+2' page (as the check is made against $_GET['q'] which is updated/mangled in drupal_init_path (and other places))..

Thanks
-K

Attached patch solves all the issues (that I am aware of), but is a minor hack. In the unlikely event that a POST operation contains a variable named 'q', this will cause breakage.

Please review the previous patch as well. This is just another approach.

Thanks
-K

I would say the correct approach is #29. Reason is it does the same it was done in phptemplate to do the job (+1 for consistency), but through a common function, which is easier to maintain.

One last attempt at using a global (attached). Please compare the drupal_is_front_page function with http://api.drupal.org/api/HEAD/function/base_path . I believe this is (at present) the best way to do this.

@markus - Yep, but as mentioned, it's minorly flawed. Any patch that doesn't check for GET[q]='' or the REQUEST['q'] hack will suffer from this issue.

Choices: 32, 19, 30, 29.

Thanks
-K

I like #30 best but the PHPdoc code needs to be corrected. In my view, it isn't a hack. Though, I'd add some code comments describing why this works, and why other solutions might not work. It is a good idea to embed some of the knowledge for future reference. Once again, great work Zen! :)

Docs updated.

Thanks :)
-K

Awesome. Committed to HEAD. Thanks.

Drupal checks if a page is the front page about 6 times on every page view.

Wouldnt it be possible then to use a static variable in the function which is initialized on the first call (page loading) instead of checking $_REQUEST everytime? Just a question.

The problem that markus_petrux described in #23 now exists:

if $_GET['q'] was set to the same path that is defined to be the frontpage, then you're in the frontpage, but $is_front would be FALSE.

And I don't understand Zen's justification for this behaviour, in #24:

Markus: That IMO is by design (and is also existing and expected behaviour). The pages can possibly be vastly different in both presentation and structure.

Huh? How can the pages be 'vastly different'? The front page with 'q' set, and the front page without 'q' set, are (and should always be) exactly the same page! The drupal_is_front_page() function should give the right value, regardless of whether or not 'q' has been set in the URL.

Therefore, the technique of checking that $_REQUEST['q'] is not set, is one that does not work reliably. E.g: say the front page is set to the default ('node'), and the user accesses 'site.com/node', instead of just 'site.com'. This check fails, because $_REQUEST['q'] is set. The drupal_is_front_page() function has to actually compare the 'q' value with the value of the front page path, and right now it's not doing that.

This scenario is more common than you might think, since Drupal redirects users to the front page after login (by default), and since it redirects them with a path that includes the 'q' value. If the site is set to display the mission, then the mission currently doesn't get displayed in this situation (because the front page check fails).

Attached patch fixes this.

There has been a lot of discussion in this thread about minimising the use of the statement: drupal_get_normal_path(variable_get('site_frontpage', 'node')). In submitting this new patch, I am well aware that drupal_init_path() already makes this call. But drupal_is_front_page() needs to make the same call as well - there's just no way around it! And as already mentioned, drupal_get_normal_path() relies on drupal_lookup_path(), and drupal_lookup_path() uses static var caching to speed itself up.

+1 for Jaza's approach

Plus ; with current cvs code, $is_front is false with url
'http://example.com/?q='

(isset($_REQUEST['q']) == true even if it is an empty string)

The above url is the one generated on a drupal_goto('') (empty string), for instance. Quite common...
That makes no sense. Please don't leave core in this state :-)

I think it was this issue:
http://drupal.org/node/17291

that added the requested check for frontpage in phptemplate engine. It is now broken, which is what I meant to say before. :-)

Here's the diff of that patch.

A big oops on not checking for ?q= in this patch. I was getting ahead of myself.

However, I pretty much disagree with everything you are saying. IMO, pretty much every instance that you are quoting is a bug or poor form. I was hoping to address most of these post 4.7.. but here goes:

• When I am on the front page and log in, I should NOT be redirected to ?q=node. This is a bug.
• I should never be able to see /?q=. This should automagically be converted to /. This is a bug.
• IMO return ''; should be return '<front>'; Inherently more legible and understandable. '' is ambiguous and the first thought is that it will redirect to the same page. Alternatively, this can possibly be fixed in drupal_goto, by either defaulting to or checking for '' or sumfin'.
• And coming to the crux of the issue, the front page is exactly that - the front page. It is accessible only at / and not at /a/b/c/d/e/f/g/h/. When I visit /a/b/c/d/e/f/g/h/:
  • I should not get a mission and any other graffiti.
  • I should be able get my breadcrumb.
  • I should not get any front page only blocks. I should be able to see my node/page specific blocks.
  • If I have a node specific theme, I should be able to see that.

  If the user desperately wants to do so, he can use whatever checks are being made for the front page on his node/page and apply the same themes or whatever for it.

That is my take on this. I will submit a patch later tonight to fix the cause rather than the effect.

Lastly, shouldn't that be get_path_alias in your patch? IIRC this is what was being used previously.

Thanks! and apologies if my mark-up is screwed; no preview.
-K

The empty string thing can be solved by using empty() instead of isset().

OK, Zen, thanks for explaining the reasoning behind your approach in more detail. I can now see why you want the front page check to only work for requests that exclude the 'q' value. BUT I still don't agree with you! :-p

Say you have an 'about' page, which has a breadcrumb (formed by user-defined menu items) of 'home -> our company -> about us', and which has a path alias of 'ourcompany/about'. This 'about' page has also been set as the site's front page. In following your approach:

1) 'mysite.com' should display the 'about' page, with no breadcrumb, with the mission (if applicable), with front page blocks, etc.
2) 'mysite.com/ourcompany/about' (or the system path, e.g. 'mysite.com/node/4') should display the 'about' page, with its 'real breadcrumbs', without the mission, etc.

In following my approach, (2) gives the same result as (1).

I can see that some people might want it to work according to your approach. In fact, it will probably seem pretty cool that you can 'use' one page as both a front page, and as a page within your site's hierarchy. But I disagree with the principle of a page looking different, depending on the URL that you use to access it. I've always thought of the 'empty q' (i.e. the front page path) as a built-in alias to whatever the 'real' front page path is (e.g. 'node'). A page in Drupal always behaves exactly the same, whether you access it via an alias or via its system path. For consistency with this, the front page should behave the same whether you access it via its 'empty q' alias, or via its real path.

Users who choose to make a page the front page should expect certain consequences to result - regardless of the path used to access the page. To limit the behaviour to only the 'empty q' path is inconsistent and, IMO, may confuse many users.

Patch attached. The user module and blogapi patches are untested, but are straightforward. Both are instances of site_frontpage usage that were missed the last time around.

I've tried to test the URL fixes with as many different types of URL combinations I could think of and all the cases mentioned in my last post. All appeared to work OK. I've not bothered to address the case where a user types ?q= himself.

Please review with care :)

Thanks,
-K

If we can't form a consensus on this, then perhaps it's an indication that some users will want their front page to behave according to one approach, and other users will want it to behave according to another approach. If so, then looking ahead to 4.8/5.0, it may be worthwhile supporting both approaches, through a simple toggle option on the admin/settings page.

Hi Jaza :)

Thanks for your reply. Looking at it from the POV of drupal.org, please compare the front page and site_frontpage which is 'node'. On the frontpage, you have the large blurb on top with the "Security announcements" image, "What is Drupal" div, and the download block. This is the frontpage of Drupal.org. A unique page (or atleast it should be) for the site.

When you login or visit "node", what you expect to see is a list of all the nodes in Drupal, not the front page. The blurb on the top is not a node and has nothing to do with this page. Click on any of the pager links at the bottom and this blurb appears on every page. Serves absolutely no practical purpose and just wastes screen real estate on every page. Please visualise this on an 800x600 screen.

As per your example with the 'about' page, you would basically not have an 'about' menu item (as well as a 'home' item and vice versa) or alternatively, link the 'about' menu item to 'home'. If your front page is an "unadulterated" version of the 'about' page, then there will be no noticeable difference between the two pages besides the breadcrumb (which will thankfully allow you to navigate back to where ever) on the about page. Even if the front page is a customised version, there is nothing to stop you from checking for drupal_path_alias in your theme's code and replicating any customisations on both pages. A win-win situation?

I've spun off the blogapi and user module patch to http://drupal.org/node/60476 .
I've updated the common.inc patch (attached) to remove PHP notices.

Thanks!
-K

I think we should go with Zen's approach. It makes most sense to me. We can revisit this after 4.7.0 is out.

Could you roll a new patch? urls_2.patch.txt does no longer apply, and urls_3.patch.txt is only a partial patch. Thanks.

Hi,

In the interest of not holding up the blogapi and user.module fixes, I'd move them to http://drupal.org/node/60476 where they've been reviewed and committed. So, urls_3 should be the complete patch.

Setting to RTBC.

Thanks :)
-K

Committed. Thanks Zen!

When viewing a single node, I am seeing a breadcrumb of home => content. thats not good. I can't make heads or tails of this issue, but it looks related. Perhaps we fix the node view problem in another issue?

several people have confirmed this bug and agreed that it is annoying. reopening ...

Hi,

The fix for this is to enable and disable the content menu item. Please use http://drupal.org/node/58034 to follow this up.

Closing this issue.

Thanks,
-K

I'm reopening this. I'm with Jaza:

* if site_frontpage is set to frontpage, then / and /?q=frontpage and /frontpage are the frontpage.
* drupal_is_front_page, however, only returns TRUE on /.
* drupal_is_front_page is *really* drupal_had_no_path_requested, not drupal_is_front_page.

I don't *care* whether modules redirect to / or not, or whether it's bad form to redirect to site_frontpage, as opposed to /. What I do care about is that those three sample URLs are *all* the Drupal frontpage, and the code should represent this.

I'd like to point out that this bug also affects Drupal.org. Look at the following two urls:
http://www.drupal.org
http://www.drupal.org/node

This could confuse people (and search engines too.)

The fix for this is to enable and disable the content menu item.

Suggested fix is not working for me on 4.7.3. Tested on 2 different 4.7.3 sites on 2 different hosts.

Was this bug fixed by?:

Commit #39936 by drumm
http://drupal.org/cvs?commit=39936
Drupal: /includes/menu.inc 1.135
#78129 by Harry Slaughter. Fix up the breadcrumb home link.

Breadcrumb "home" link points to example.com/node in 4.7.3
http://drupal.org/node/78129
ericatkins - August 11, 2006 - 05:10
Project: Drupal
Version: cvs
Component: menu system
Category: bug
Priority: normal
Assigned: Harry Slaughter
Status: fixed

Eddie

I may be completely off base here but I'll give it a shot any way.

I've had the same problem with 4.7 (and under) for awhile using front_page module and without. What I was trying to accomplish was to display only the page as setup in the frontpage module but needed it themed for authenticated users. For everyone else just the spash page and content links I would allow.

Not knowing any better I took the approach of modifying the page.tpl.php file. I could not find any other way to do it with anything else.

What I ended up doing was a hack but it works. It uses a combination of 1 global and a boolean check ($user->uid) where ever you want to disable the output. Hope someone can come up with a solution where in the admin settings a flag could be set for user roles if you want to disable content for some and not other. In the meantime, I've included my page.tpl.php file for an example of how I did it.

I used the fancy theme based from bluemarine but believe this could be done with any theme.

Can this bug be resolved somehow
before the freeze on 1 JUL 2007?

Maybe the issue no longer exists in D6.

Thank you,
Eddie Maddox

Drupal 4 is no longer supported.