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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anrikun’s picture

Status: Active » Needs review
FileSize
1.33 KB

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

anrikun’s picture

Issue summary: View changes

Added full description

anrikun’s picture

Issue summary: View changes

Minor changes.

anrikun’s picture

Issue summary: View changes

Minor change.

anrikun’s picture

ramlev’s picture

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

ramlev’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

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()?

anrikun’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests +Regression
FileSize
2.11 KB
794 bytes

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.29 KB

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.

anrikun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.3 KB

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

anrikun’s picture

Status: Needs work » Reviewed & tested by the community

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that makes sense! :)

catch’s picture

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.

anrikun’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.68 KB
anrikun’s picture

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.

sun’s picture

Status: Needs review » Needs work

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

anrikun’s picture

Status: Needs work » Needs review
FileSize
2.77 KB

Ok sun. Here's the updated patch.

sun’s picture

+++ 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.

anrikun’s picture

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?

sun’s picture

Hm. You might be right. :)

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

anrikun’s picture

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Assigned: Unassigned » David_Rothstein

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

David_Rothstein’s picture

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).

David_Rothstein’s picture

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.

David_Rothstein’s picture

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?

anrikun’s picture

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).

Jkb84’s picture

I used Menu Trail By Path module to fix this issue

warmth’s picture

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');
warmth’s picture

Issue summary: View changes

Added a note about D7

MustangGB’s picture

Status: Needs work » Needs review
Anybody’s picture

Anybody’s picture

The problem still exists for Drupal 7 and there has not been any progress for a long period of time. Are there plans for this issue?

gippy’s picture

I may not understand the problem, but I don't see any difference after the patch is applied. No change in breadcrumb for user/1. Can someone explain exactly what is supposed to change?

divined’s picture

up

  • catch committed 820f4eb on 8.3.x
    Issue #1564388 by anrikun, sun: Fixed 'My account' link is never in the...

  • catch committed 820f4eb on 8.3.x
    Issue #1564388 by anrikun, sun: Fixed 'My account' link is never in the...

  • catch committed 820f4eb on 8.4.x
    Issue #1564388 by anrikun, sun: Fixed 'My account' link is never in the...

  • catch committed 820f4eb on 8.4.x
    Issue #1564388 by anrikun, sun: Fixed 'My account' link is never in the...
iancawthorne’s picture

Just a thought on this, as I found this issue while searching for solutions.

If you use the "menu_position" module and create a rule with "User page > Apply this rule on user account pages.", assigned to the "My account" menu item, this should give the desired result.

ydahi’s picture

Still no way to do this cleanly in D8?

I've tried the following modules and none of them seem to fit:

  • context
  • menu_position
  • user_current_paths
  • menu_trail_by_path

The problem is that when setting the "My Account" menu item as the active trail when the path is /user or /user/* means that this menu is active when the user visits another user account as well.

Any recommendations?

  • catch committed 820f4eb on 9.1.x
    Issue #1564388 by anrikun, sun: Fixed 'My account' link is never in the...