This issue in itself is a minor bug but I'm marking it as major because it is a blocker to fix critical bug #550254: Menu links are sometimes not properly re-parented that affects D7 too.

Problem/Motivation

We don't want "My account" link to show up in the breadcrumb when we are on a page that is descendant-or-self of system path user/%. Currently, this is achieved using the following hack:

<?php
  $items
['user/%user'] = array(
   
'title' => 'My account',
   
'title callback' => 'user_page_title',
   
'title arguments' => array(1),
   
'page callback' => 'user_view_page',
   
'page arguments' => array(1),
   
'access callback' => 'user_view_access',
   
'access arguments' => array(1),
   
// By assigning a different menu name, this item (and all registered child
    // paths) are no longer considered as children of 'user'. When accessing the
    // user account pages, the preferred menu link that is used to build the
    // active trail (breadcrumb) will be found in this menu (unless there is
    // more specific link), so the link to 'user' will not be in the breadcrumb.
   
'menu_name' => 'navigation',
  );
?>

As it is a hack, it is not a clean way of doing this, plus, moving user/% to a different menu has the side effect of preventing "My account" from being shown as "in active trail" when it should, e.g. when a user edits its own account.

Proposed resolution

API should be used here: by simply implementing hook_menu_breadcrumb_alter(), we can remove the "My account" link from the breadcrumb without hacking menu_name, nor breaking the active trail.

Files: 
CommentFileSizeAuthor
#16 user_menu_breadcrumb-1564388-16-D7.patch2.77 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 40,388 pass(es).
[ View ]
#14 user_menu_breadcrumb-1564388-14-D7-tests_only.patch790 bytesanrikun
FAILED: [[SimpleTest]]: [MySQL] 39,358 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#14 user_menu_breadcrumb-1564388-14-D7.patch2.7 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 39,357 pass(es).
[ View ]
#13 user_menu_breadcrumb-1564388-13-D7.patch2.68 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 39,345 pass(es).
[ View ]
#9 drupal8.user-menu-breadcrumb.9.patch2.3 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 39,748 pass(es).
[ View ]
#7 drupal8.user-menu-breadcrumb.7.patch2.29 KBsun
FAILED: [[SimpleTest]]: [MySQL] 39,939 pass(es), 9 fail(s), and 426 exception(s).
[ View ]
#6 user_active_trail-1564388-6-test_only.patch794 bytesanrikun
FAILED: [[SimpleTest]]: [MySQL] 36,618 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#6 user_active_trail-1564388-6.patch2.11 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 36,617 pass(es).
[ View ]
#1 user_active_trail-1564388-1.patch1.33 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 36,488 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.33 KB
PASSED: [[SimpleTest]]: [MySQL] 36,488 pass(es).
[ View ]

I was not able to test this locally. Let's test it here.

Issue summary:View changes

Added full description

Issue summary:View changes

Minor changes.

Issue summary:View changes

Minor change.

This patch fixes the issue. Tested and everything is ok.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

There was some discussion of this in #550254: Menu links are sometimes not properly re-parented from #162 down, it looks like sun already tried various menu flags to get the same behaviour, but on the other hand the breadcrumb alter doesn't feel right to me.

The patch on the other issue showed there's no test coverage for this, let's add some here. Also if the only intention of the array_splice() is to remove one array element, what's wrong with unset()?

Status:Needs work» Needs review
Issue tags:-Needs tests+regression
StatusFileSize
new2.11 KB
PASSED: [[SimpleTest]]: [MySQL] 36,617 pass(es).
[ View ]
new794 bytes
FAILED: [[SimpleTest]]: [MySQL] 36,618 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Added the missing test.

@catch: I'm using array_splice() instead of unset() for numeric keys to be reindexed. We're not supposed to know how $active_trail will be used after alter, so I guess it's better to return it in a clean state.

+ Added tag "regression" as this used to work in D6.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new2.29 KB
FAILED: [[SimpleTest]]: [MySQL] 39,939 pass(es), 9 fail(s), and 426 exception(s).
[ View ]

Meanwhile, I think that this might be the only proper solution for the problem.

Alas, not the first alter hook being implemented by User module. The very same link happens to be hacked out in a hook_menu_translated_link_alter() implementation for anonymous users -- one might consider that an even larger hack than this.

+++ b/core/modules/user/user.module
@@ -3760,3 +3754,14 @@ function user_file_download_access($field, $entity_type, $entity) {
+  if (strpos($item['path'], 'user/%') === 0 && $active_trail[1]['link_path'] == 'user' && $active_trail[1]['module'] == 'system') {

Can we move the last condition first? It's the fastest one, so improving performance, since this hook is called on every page.

Even better, swap the first and last.

Re-rolled against HEAD and applied this change. No other changes, so this looks RTBC.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.user-menu-breadcrumb.7.patch, failed testing.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,748 pass(es).
[ View ]

Let's try this one then.
Replaced isset($active_trail[1]) by isset($active_trail[1]['module'])

Status:Needs work» Reviewed & tested by the community

Seems OK now. Back to RTBC as at #7.

Status:Needs review» Reviewed & tested by the community

Thanks, that makes sense! :)

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks. Committed/pushed to 8.x, moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 39,345 pass(es).
[ View ]

StatusFileSize
new2.7 KB
PASSED: [[SimpleTest]]: [MySQL] 39,357 pass(es).
[ View ]
new790 bytes
FAILED: [[SimpleTest]]: [MySQL] 39,358 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

It seems to be OK, but for safety, let's add an extra module = 'system' condition in update query.
Let's run tests-only patch too.

Status:Needs review» Needs work

The delete query in the update needs to use db_delete() with a proper db_like() condition.

Status:Needs work» Needs review
StatusFileSize
new2.77 KB
PASSED: [[SimpleTest]]: [MySQL] 40,388 pass(es).
[ View ]

Ok sun. Here's the updated patch.

+++ b/modules/user/user.install
@@ -909,5 +909,17 @@ function user_update_7018() {
/**
+ * Move menu link "My account" and its children from Navigation to User menu, by
+ * deleting them if non-customized.
+ */
+function user_update_7019() {
+  db_delete('menu_links')
+    ->condition('link_path', db_like('user/%') . '%', 'LIKE')
+    ->condition('module', 'system')
+    ->condition('customized', 0)
+    ->execute();
+}
+++ b/modules/user/user.module
@@ -1736,12 +1736,6 @@ function user_menu() {
-    'menu_name' => 'navigation',

Hm. After re-reviewing the changes once more, I actually think we cannot backport this to D7. :-/

An existing site or theme might rely on the fact that the My account + child links appear in the Navigation menu, and the links might not be customized.

Thus, by committing this patch, the links would suddenly appear in a different menu after running update.php, potentially breaking a site's layout and/or theme.

Hi sun, I think you forgot something:
Remember that there have never been any 'user/%' links displayed in the Navigation menu.
Using the Navigation menu here was just a hack for breadcrumb and no links where supposed to actually show up in the Navigation menu.

So what we move here are only hidden links. So no layout and/or theme to break :-)

I guess my comment in code does not reflect that and might be changed then?

Hm. You might be right. :)

Btw, did you intend to escape the '%' within 'user/%'? That essentially leads to LIKE 'user/\%%', I think.

Yes I did, in order to catch any link_path starting by 'user/%'.

Status:Needs review» Reviewed & tested by the community

Alright, let's see what D7 maintainers think about this. :)

Assigned:Unassigned» David_Rothstein

This smells risky, which means I kick it to our beloved D7 release manager. :) Sorry, David.

It's not too comforting that no one seems to be 100% sure this won't break something :) And after thinking about it for a while, I'm in the same boat. It's just really hard to understand all the possible consequences of these types of menu system changes.

Kind of feel like I need to think about this a bit more, sorry. As a stalling tactic, I'll point out a minor issue in the D7 patch :)

/**
+ * Move menu link "My account" and its children from Navigation to User menu, by
+ * deleting them if non-customized.
+ */

The summary should ideally be one line (with a max of 80 characters). Especially since this gets shown in the user interface on update.php, I'd suggest doing that by just removing the "by deleting them if non-customized" part and moving that to a regular code comment inside the body of the function (since it is more of a technical description).

Assigned:David_Rothstein» Unassigned
Status:Reviewed & tested by the community» Needs work

Hold on, I just played with this a little more and found a problem:

  1. Start with a fresh installation of Drupal.
  2. Apply the patch.
  3. Run drush updatedb.
  4. Visit user/1/edit. The page is all messed up. The page title is "Home" (rather than the username), and the breadcrumb is just "Home" (rather than "Home » [username]").

Strangely enough, I couldn't reproduce this if I ran the updates with update.php, only with Drush (I have Drush 5.1 locally). But something weird is going on there. It looks like a lot of items that are supposed to make it into the {menu_links} table aren't getting inserted there.

Also, independent of the above comment, if you go to user/1/edit and print the results of menu_get_active_trail(), then before the patch you get:

Home > My Account > Edit

But after the patch you get:

Home > User Account > My Account > Edit

I realize that might be partially intended, but do we really want to change the output of that function in Drupal 7? Are there modules or custom code that might be calling menu_get_active_trail() directly and working with its result in a way that would be broken by this?

Hi David,

About #24: I don't know much about Drush, but isn't it possible that Drush does not trigger a menu rebuild after update?
The patch deletes menu links, expecting menu to be rebuilt, so that deleted menu links are recreated at the right place. Menu rebuild is automatic when we run update.php. But maybe Drush does not do it?

About #25: actually, trail is correct this way as 'user/%user/edit' is the path. This is intended because trail in Drupal is currently wrong (this is the subject of this issue).
Besides, as of Drupal 7 API, trail and breadcrumb are not necessarily the same.

Do you think we still should let current trail as is? I'll try to see if we can get the expected result without changing the trail (but this would be another hack).

I used Menu Trail By Path module to fix this issue

Sorry that doesn't worked for me but this did (thanks to Sun's idea on #1292590: "My Account" not active in menu tree of User Menu):

<?php
 
global $user;
  if(!empty(
$vars['user'])) {
        if (
$_SERVER['REQUEST_URI'] == '/site/user') {
           
menu_set_active_item('user');
        }
        if (
drupal_match_path(current_path(),"user") ||
           
drupal_match_path(current_path(),"user/".$user->uid)){
           
menu_set_active_item('user');
        }
    }
?>

plus
jQuery('#block-system-user-menu ul li a.active').addClass('active-trail');
jQuery('#block-system-user-menu ul li a.active').parent().addClass('active-trail');

Issue summary:View changes

Added a note about D7

Status:Needs work» Needs review