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.
Comment | File | Size | Author |
---|---|---|---|
#16 | user_menu_breadcrumb-1564388-16-D7.patch | 2.77 KB | anrikun |
#14 | user_menu_breadcrumb-1564388-14-D7-tests_only.patch | 790 bytes | anrikun |
#14 | user_menu_breadcrumb-1564388-14-D7.patch | 2.7 KB | anrikun |
#13 | user_menu_breadcrumb-1564388-13-D7.patch | 2.68 KB | anrikun |
#9 | drupal8.user-menu-breadcrumb.9.patch | 2.3 KB | anrikun |
Comments
Comment #1
anrikun CreditAttribution: anrikun commentedI was not able to test this locally. Let's test it here.
Comment #1.0
anrikun CreditAttribution: anrikun commentedAdded full description
Comment #1.1
anrikun CreditAttribution: anrikun commentedMinor changes.
Comment #1.2
anrikun CreditAttribution: anrikun commentedMinor change.
Comment #2
anrikun CreditAttribution: anrikun commentedComment #3
ramlev CreditAttribution: ramlev commentedThis patch fixes the issue. Tested and everything is ok.
Comment #4
ramlev CreditAttribution: ramlev commentedComment #5
catchThere 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()?
Comment #6
anrikun CreditAttribution: anrikun commentedAdded the missing test.
@catch: I'm using
array_splice()
instead ofunset()
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.
Comment #7
sunMeanwhile, 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.
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.
Comment #9
anrikun CreditAttribution: anrikun commentedLet's try this one then.
Replaced
isset($active_trail[1])
byisset($active_trail[1]['module'])
Comment #10
anrikun CreditAttribution: anrikun commentedSeems OK now. Back to RTBC as at #7.
Comment #11
sunThanks, that makes sense! :)
Comment #12
catchThanks. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #13
anrikun CreditAttribution: anrikun commentedComment #14
anrikun CreditAttribution: anrikun commentedIt 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.
Comment #15
sunThe delete query in the update needs to use db_delete() with a proper db_like() condition.
Comment #16
anrikun CreditAttribution: anrikun commentedOk sun. Here's the updated patch.
Comment #17
sunHm. 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.
Comment #18
anrikun CreditAttribution: anrikun commentedHi 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?
Comment #19
sunHm. You might be right. :)
Btw, did you intend to escape the '%' within 'user/%'? That essentially leads to
LIKE 'user/\%%'
, I think.Comment #20
anrikun CreditAttribution: anrikun commentedYes I did, in order to catch any link_path starting by 'user/%'.
Comment #21
sunAlright, let's see what D7 maintainers think about this. :)
Comment #22
webchickThis smells risky, which means I kick it to our beloved D7 release manager. :) Sorry, David.
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedIt'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 :)
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).
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedHold on, I just played with this a little more and found a problem:
drush updatedb
.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.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, 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?
Comment #26
anrikun CreditAttribution: anrikun commentedHi 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).
Comment #27
Jkb84 CreditAttribution: Jkb84 commentedI used Menu Trail By Path module to fix this issue
Comment #28
warmth CreditAttribution: warmth commentedSorry 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):
plus
Comment #28.0
warmth CreditAttribution: warmth commentedAdded a note about D7
Comment #29
MustangGB CreditAttribution: MustangGB commented16: user_menu_breadcrumb-1564388-16-D7.patch queued for re-testing.
Comment #30
Anybody16: user_menu_breadcrumb-1564388-16-D7.patch queued for re-testing.
Comment #31
AnybodyThe 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?
Comment #32
gippy CreditAttribution: gippy commentedI 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?
Comment #33
divined CreditAttribution: divined commentedup
Comment #38
iancawthorne CreditAttribution: iancawthorne commentedJust 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.
Comment #39
ydahiStill no way to do this cleanly in D8?
I've tried the following modules and none of them seem to fit:
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?