Items defined with type MENU_CALLBACK show "Home" as a title

subscribing see also #994112: Page titles are not set on /cart/checkout/review and /cart/checkout

Analysis seems to make sense. However, we should

1) improve and clarify the inline comment (moving some of the essentials in the analysis here into code).

2) Extend the existing breadcrumb test to also test a page of type MENU_CALLBACK.

Here it is again taking into account sun's comments

Test still hasn't been extended.

Also, small typo: it should read (since it's needed for the page, not its.

Here it is again with typo corrected and a new test testTitleOrphanMenuCallback which fetches path menu_callback_orphan defined in menu_test.module that is a MENU_CALLBACK item with no parents - and tests for the appropriate page title

hmm, test is no good, reviewing now

access denied on my callback, doh
here it is again

+++ includes/menu.inc	14 Dec 2010 00:24:30 -0000
@@ -2303,11 +2303,25 @@ function menu_set_active_trail($new_trai
-    // Make sure the current page is in the trail (needed for the page title),
-    // if the link's type allows it to be shown in the breadcrumb. Also exclude
-    // it if we are on the front page.
+    // Make sure the current page is in the trail (since its needed for the page
+    // title), don't add it if we are on the front page (since its already in
+    // the trail).
+    //

1) Let's revert the addition(s) of "since it's".

2) Avoid abbreviations like "don't" and always spell them out instead.

3) The reasoning for not including the current page on the front page does not sound 100% correct to me. Since that is irrelevant for this issue in the first place, let's remove that reasoning.

4) Let's also remove the "blank comment line" (or rather, that entire separation of comments).

+++ includes/menu.inc	14 Dec 2010 00:24:30 -0000
@@ -2303,11 +2303,25 @@ function menu_set_active_trail($new_trai
+    // See #965272: The title for a page is determined by taking the last item in the
+    // active path which is not a local task. The active path is seeded with a
+    // link to home and each item in the path with type
+    // MENU_VISIBLE_IN_BREADCRUMB is added to the list.
+    // At the end of this process the preferred link for the current item is
+    // added to the end of the breadcrumb if it is flagged as
+    // MENU_VISIBLE_IN_BREADCRUMB or MENU_CALLBACK. If we don't do this,
+    // items of type MENU_CALLBACK without parents will result in the page title
+    // being Home- as this would be the only item in the active path. Adding the
+    // preferred link to the breadcrumb trail even if it is only a MENU_CALLBACK
+    // does not have an adverse affect on the breadcrumbs as the current page
+    // is always dropped from the breadcrumb trail anyway
     $last = end($trail);
-    if ($last['href'] != $preferred_link['href'] && ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) == MENU_VISIBLE_IN_BREADCRUMB && !drupal_is_front_page()) {
+    if ($last['href'] != $preferred_link['href'] && !drupal_is_front_page() &&
+        in_array($last['type'], array(MENU_CALLBACK, MENU_VISIBLE_IN_BREADCRUMB), TRUE)) {
       $trail[] = $preferred_link;

This type check is not correct.

1) MENU_VISIBLE_IN_BREADCRUMB is an internal menu system constant that is normally not specified directly. The menu system constants are bitmasks.

2) In D7, router items of type MENU_CALLBACK do not have a corresponding menu link.

In terms of code and logic, I think we can go back to the original patch and remove the additional condition altogether. The effect is that a MENU_CALLBACK (which can only originate from menu_get_item(), not menu_link_get_preferred()) is appended to the active trail, if it's not contained (last) yet, so we can use it for the page title elsewhere.

A side-effect of this change will be that menu links that are explicitly NOT visible in the breadcrumb will also be appended by this code. But again, the last item in the active trail does not actually appear in the breadcrumb, so that should be fine.

In turn, we can heavily shorten the entire code comment to clarify the situation.

(Note that we normally don't put issue numbers/URLs into code comments, comments should wrap at 80 chars, and always form proper sentences [trailing period, etc])

+++ modules/simpletest/tests/menu.test	14 Dec 2010 00:24:37 -0000
@@ -33,6 +33,16 @@ class MenuRouterTestCase extends DrupalW
+  ¶

Trailing white-space.

+++ modules/simpletest/tests/menu.test	14 Dec 2010 00:24:37 -0000
@@ -33,6 +33,16 @@ class MenuRouterTestCase extends DrupalW
+   * Test title is set correctly on orphan (no parents)
+   * items of type MENU_CALLBACK and not 'Home'

1) Wraps too early.

2) Can be shortened into:

"Tests page title of MENU_CALLBACKs."

+++ modules/simpletest/tests/menu.test	14 Dec 2010 00:24:37 -0000
@@ -33,6 +33,16 @@ class MenuRouterTestCase extends DrupalW
+   * @see http://drupal.org/node/965272

+++ modules/simpletest/tests/menu_test.module	14 Dec 2010 00:24:37 -0000
@@ -16,6 +16,13 @@ function menu_test_menu() {
+  //is set correctly - @see http://drupal.org/node/965272

Please remove those issue URLs.

+++ modules/simpletest/tests/menu.test	14 Dec 2010 00:24:37 -0000
@@ -33,6 +33,16 @@ class MenuRouterTestCase extends DrupalW
+    $this->assertText('Menu Callback Orphan', t('Raw text found on the page'));

+++ modules/simpletest/tests/menu_test.module	14 Dec 2010 00:24:37 -0000
@@ -16,6 +16,13 @@ function menu_test_menu() {
+    'title' => 'Menu Callback Orphan',

Let's change this title into:

"Menu callback title"

and afterwards, remove the custom assertion message (which isn't very informative); the default is "%text found."

Note that the menu router item title is passed through t() upon output, so t() must be used for assertText().

+++ modules/simpletest/tests/menu_test.module	14 Dec 2010 00:24:37 -0000
@@ -16,6 +16,13 @@ function menu_test_menu() {
+  //This item is of type MENU_CALLBACK with no parents to test title
+  //is set correctly - @see http://drupal.org/node/965272

Missing leading space and trailing period (full-stop). See http://drupal.org/node/1354 for comment coding style details.

+++ modules/simpletest/tests/menu_test.module	14 Dec 2010 00:24:37 -0000
@@ -16,6 +16,13 @@ function menu_test_menu() {
+    'page callback' => 'node_page_default',

If node_page_default() will change in the future, this test will break, even though it's technically completely unrelated. I think there should be an existing "null" page callback somewhere in menu_test.module already that we should use here instead. Otherwise, please create a new page callback function that simply returns an empty string.

Powered by Dreditor.

Here is another patch with these issues remedied.
Now we don't test the menu type at all?
Should we be testing that it is not of type MENU_LOCAL_TASK or MENU_DEFAULT_LOCAL_TASK as these items should not be used for the title?

How I can test this patch in my drupal instalation?

please see http://drupal.org/patch/apply
but basically
*download the file
*on the command line

cd /path/to/drupal/install
patch -p0 < /path/to/patchfile

Thanks larowlan.

I tested the patch (issue-96527-2.patch) and works fine on my drupal installation.

Do I have to change the status to "reviewed & tested by the community"? (It's my first time that I test a patch, sorry)

Possible dup: #1005830: Breadcrumb trail is missing and page title is misset to 'Home' for many module pages

The patch from #14, issue-96527-2.patch, solves the problem for me.

This could really use one more look from sun.

+++ includes/menu.inc	28 Dec 2010 06:04:05 -0000
@@ -2303,11 +2303,18 @@ function menu_set_active_trail($new_trai
-    // Make sure the current page is in the trail (needed for the page title),
-    // if the link's type allows it to be shown in the breadcrumb. Also exclude
-    // it if we are on the front page.
+    // Make sure the current page is in the trail (needed for the page title)
+    // The title for a page is determined by taking the last item in the active
+    // path which is not a local task. The active path is seeded with a link to
+    // home and each item in the path with type MENU_VISIBLE_IN_BREADCRUMB is
+    // added to the list. At the end of this process the preferred link for the
+    // current item is added to the end of the breadcrumb if it is not of type
+    // MENU_LOCAL_TASK. Adding the preferred link to the breadcrumb trail even
+    // if it is only a MENU_CALLBACK does not have an adverse affect on the
+    // breadcrumbs as the current page is always dropped from the breadcrumb
+    // trail anyway.

We can heavily shorten this, since most of the comment explains the behavior of other functions. Something along the lines of:

Make sure the current page is in the trail to build the page title, by appending either the preferred link or the menu router item for the current page. Exclude it if we are on the front page.

(Yes, I realize that the front page situation could be explained a bit more, but that's an entirely different, weird handling, off-topic for this issue, so let's just retain the current sentence about it.)

+++ modules/simpletest/tests/menu.test	28 Dec 2010 06:04:07 -0000
@@ -33,6 +33,14 @@ class MenuRouterTestCase extends DrupalW
+  ¶

Trailing white-space.

+++ modules/simpletest/tests/menu.test	28 Dec 2010 06:04:07 -0000
@@ -33,6 +33,14 @@ class MenuRouterTestCase extends DrupalW
+    $this->drupalGet('menu_callback_title');
+    $this->assertText(t('Menu Callback Title'));

Did we test that the menu item's title does not already appear in the navigation menu or somewhere else on the page, leading to a false positive?

Instead of testing this manually, prepend the following to those test lines:

// Verify that the menu router item title is not visible.
$this->drupalGet('');
$this->assertNoText(t('Menu Callback Title'));
// Verify that the menu router item title is output as page title.
...

Powered by Dreditor.

I have two colorbox forms. They should not show as links.

The urls are cb/form/form1 and cb/form/form2.

What is the best way todo this in the menusystem:

1. Should I use a MENU_CALLBACK and then in the formfunction call drupal_set_title('My title')
2. Should I use MENU_VISIBLE_IN_BREADCRUMB
3. Should I use MENU_NORMAL_ITEM and deactivated it in the menu system.

And now that single test failure is... guess what? ;)

"Menu Callback Title" not found	     MenuRouterTestCase->testTitleMenuCallback()

Apparently, 'type' => MENU_CALLBACK, is missing in the router item definition.

Added type => MENU_CALLBACK however this wasn't in the earlier tests either

Reroll

Ignore my comments from before, I understand what sun is saying about why nominating MENU_CALLBACK is important, without it the default of MENU_NORMAL_ITEM would be used and hence the text would show on all pages as a menu link in the navigation.

+++ includes/menu.inc	4 Jan 2011 23:02:52 -0000
@@ -2303,11 +2303,11 @@ function menu_set_active_trail($new_trai
+    // appending either the preferred link or the menu router item for the ¶

Trailing white-space.

Powered by Dreditor.

Sun points out that this existing function also prevents displaying the title of a local task:

http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_get_ac...

which seems to have been a main point of that check. Are we sure that we won't get a local action as a page title?

@pwolanin: Thanks for having a look.

Local actions are also covered by the exclusion condition in menu_get_active_title(), since MENU_LOCAL_ACTION is also MENU_IS_LOCAL_TASK. Weird, but true.

Whitespace removed, thanks sun

@sun, thanks for double checking.

Subscribing, as this affects Ubercart, and bumping in the hope it will get committed after two months at RTBC.

#994112: Page titles are not set on /cart/checkout/review and /cart/checkout
#1084432: Wrong page title on checkout pages

I can confirm that the patch in #37 still applies cleanly to 7.x HEAD and fixes the problem. I would change status to RTBC, but it already is ...

This patch did not fix

/user/login

still shows "Home". Using Bartik theme.

@schaub123: Two things:

First, that is not relevant to this issue. user/login is not a MENU_CALLBACK, it is a MENU_DEFAULT_LOCAL_TASK. The reported problem and proposed fix have to do with improper titles on MENU_CALLBACK functions.

Second, I cannot reproduce what you report. When logged out, if I visit user/login (with Bartik) I see a page title of "User account". I suggest you open a new issue if you continue to have a problem, as the behavior report is not related to the subject of this thread.

subscribe

D8 first. Let's get these fixed on D8 and into D7.

Patch in #37 still applies in Drupal 8.x, but I re-rolled it just for fun with the latest and greatest git format.

I tested this with Ubercart running under D8 (yes, you read that correctly); The original problem still exists in D8 and the patch still fixes the problem in D8.

I should also mention I ran the menu SimpleTests with the patch applied (patch adds tests for this issue), and they all ran green.

I'm going to take it upon myself to upgrade the priority. Here's why:

We can't make a non-beta release of Ubercart for Drupal 7 when a bunch of the page titles incorrectly read "Home", so we're stuck in beta until this (and other things, of course) gets fixed.

Drupal 7.1 is just around the corner, and if this fix doesn't get into 7.1 it's going to be many more months before it can get fixed in D7 core.

I'd like to point out that this patch is to restore functionality that was broken by #907690: Breadcrumbs don't work for dynamic paths & local tasks #2, way before D7 was released. Pushing the bar higher by insisting this gets fixed in D8 first is just delaying the resolution of a long-known bug inadvertently introduced into D7.

I understand why issues are being moved to D8, but I don't really agree that it's the correct move for *this* issue because this issue was RBTC before D7.0 was published. By moving the issue to D8 it's going to take that much longer to get it committed to D8 first then into D7. Nothing significant runs under D8 yet so a problem that is obvious with contribs in D7 can't necessarily be reproduced in D8 yet. This results in further delays to fixing a known bug in D7.

Tagging...

@TR: see #1050616: Figure out backport workflow from Drupal 8 to Drupal 7

Still looking good to me. Should apply cleanly to D8 and D7.

subscribe

subscribe

Reviewed this patch and it looks good. Committed to 7.x and 8.x.

I'm glad to hear that. Good news definitely.

Now I have a problem with MENU_LOCAL_TASK.
You can see it on the user site when your are logged out.

It shows always the same title ('User account') by all three paths:

-user/register
-user/login
-user/password

I have another multilanguage site and there it shows 'Home' as title.

It shows always the same title ('User account') by all three paths:

-user/register
-user/login
-user/password

That's the expected behavior. IIRC, even covered by unit tests.

I don't think so.

When you look in the user module you can see in the hook_menu different titles.

Therefore I can't understand why it is always the same title.

These titles are for the tabs (ie local tasks) not the page title

Not really relevant to this thread though - it's a different issue.

Thanks for this fix - I was tearing my hair out trying to understand how to create menu items using the examples in the Drupal 7 Module books. The titles were not behaving as expected and the books make no mention of the bug.

Is this fix applied to D7? If so, which version of D7?

Also, for MENU_CALLBACK, I understand we cannot have that particular page show up in the Menu system or navigation, because essentially it is a call back from a REFERRER page.

Therefore, is it possible, that in any instance a web site user lands on a MENU_CALLBACK page, that we show the navigation... of the REFERRER page if that page that referred the MENU_CALLBACK page is a navigation menu item? This is how breadcrumbs work on checkout pages.