User edit title is broken (so is beta1-beta2 upgrade path)

This is actually extensively covered by unit tests, so I'm not sure what went wrong for you. Definitely not critical.

"root" can technically only appear if there is a menu link in {menu_links} with the link_title "root". Can you figure out whether there is one and from where it comes?

I think I see the misunderstanding. Drunken monkey's user 1 is called "root".

My user 1 is "admin", and these are the title and breadcrumbs at user/2/shortcuts:

I assume this is probably actually an issue with user.module somewhere.

This probably isn't a release blocker, but it's pretty nasty...

And we clearly either need new tests or to fix the old ones.

Let's try that as a title.

I'm pretty sure this is due to the some of the changes in menu.inc from #907690: Breadcrumbs don't work for dynamic paths & local tasks #2.

...subscribing

seems like we had to fix this once for D6 - %user_category maybe?

Mmm. Reflecting on it more, unfortunately, this is actually critical.

I wrote a quick test that created two users, one with admin rights, logged in with the admin user, viewed the other's view and edit page. It passed the correct breadcrumb and title assertions.

It looks like this might only be a problem with user 1 as those two users were uid 2 and 3 respectively.

I can reproduce this with user 2 and user 3 like so:

- Create 2 non-admin users
- Give View user profiles permission to Authenticated User
- Log in as User 2
- Go to user/3

Result is that the title is ok but the breadcrumb still shows user 2 (the currently logged in user).

Nevermind. CVS didn't clean up properly, and I didn't have a clean checkout. Sorry.

Here's a patch for my test I wrote. It now fails like it should ;-)

So, ummm.... Drupal 7 is useless until this is fixed?

...not useless. I'd say rather crippled. That's why this is critical ;)

This bug is that user n's breadcrumb is shown as Home >> [n-1 username] instead of Home >> [n username] the way I see it. Right? Does this happen for all users or did you people only test it with 3 users? If so, could this simply be a matter of incrementing some variable starting from 1 instead of 0 or something like that? I actually wouldn't know. I am just guessing here.

This bug is that user n's breadcrumb is shown as Home >> [n-1 username] instead of Home >> [n username] the way I see it.

...I meant to also ask: does the [n-1 username] part of the breadcrumb link to the right user though or is the link messed too?

The breadcrumb is showing the currently logged in user for me, no matter which user I go to. I created a few accounts to test this with.

The link is pointing to the currently logged in user also.

The issue is that the $uid passed to user_page_title is wrong

So it seems that it's being overridden by the user_uid_only_optional() function, which is used in the argument for the parent menu item, which sets the argument to that of the currently logged in user. You can see this by changing $items['user/%user_uid_only_optional'] in user_menu to $items['user/%'] and the title and breadcrumb on the edit / shortcuts / etc pages works as expected. I'm not currently sure why this is behaving this way...

I've done some digging and narrowed it down to the changes in menu_set_active_trail() in #907690: Breadcrumbs don't work for dynamic paths & local tasks #2. Part of the problem seems to be that menu_link_get_preferred() isn't working as expected in this situation. E.g. on the edit screen it is returning user/%/edit as the "preferred" link, when it should be returning user/2/edit.

Commenting-out the line following step 3 in menu.inc affects this bug but doesn't fix it completely (it fixes the title but makes the breadcrumb weirder):

    // Look for the correct menu link by building a list of candidate paths,
    // which are ordered by priority (translated hrefs are preferred over
    // untranslated paths). Afterwards, the most relevant path is picked from
    // the menus, ordered by menu preference.
    $item = menu_get_item($path);
    $path_candidates = array();
    // 1. The current item href.
    $path_candidates[$item['href']] = $item['href'];
    // 2. The tab root href of the current item (if any).
    if ($item['tab_parent'] && ($tab_root = menu_get_item($item['tab_root_href']))) {
      $path_candidates[$tab_root['href']] = $tab_root['href'];
    }
    // 3. The current item path (with wildcards).
    $path_candidates[$item['path']] = $item['path'];
    // 4. The tab root path of the current item (if any).
    if (!empty($tab_root)) {
      $path_candidates[$tab_root['path']] = $tab_root['path'];
    }

Should it? The mapping returned by menu_link_get_preferred() seems correct. Also, if you look at the same situation for nodes then the link path and router paths returned are node/%/edit not node/12/edit, but the breadcrumbs are still right.

(not my area (yet ;) ), so apologies if I'm talking rubbish!)

+++ modules/user/user.test	1 Oct 2010 02:52:46 -0000
@@ -1461,6 +1461,48 @@ class UserEditTestCase extends DrupalWeb
+  function testUserTitle() {

Please add those assertions to the existing MenuBreadcrumbsTestCase in modules/simpletest/tests/menu.test instead.

- Insert them as the first assertions after the initial variable setup.

- Use the same assertion method like all the rest of that test case.

- Make sure that the assertions come before the existing assertions.

- You may copy the required assertion lines from further down in the existing test case, but do not alter any of the existing test code.

Powered by Dreditor.

I think this is part of a larger issue. There are breadcrumb and title tests in other module's tests. I followed node.test.

Maybe we need an issue to move all of those into menu.test?

Or should we split the title and breadcrumb assertions into different tests?

ok this is how I understand the issue to be at the moment (sorry for my limited knowledge in this area):

- The incorrect breadcrumb and title are rendered using the parent item's title
- The parent item's title is generated from:

$items['user/%user_uid_only_optional'] = 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),
    'weight' => -10,
    'menu_name' => 'user-menu',
  );

Which generates that title with the following:

return empty($arg) || $arg == '%' ? $GLOBALS['user']->uid : $arg;

- So therefore a value of '%' is being passed to the parent menu item instead of the desired uid

So, ummm.... Drupal 7 is useless until this is fixed?

That’s Barry's subtle way of disagreeing with the Critical priority here. And I agree. I would call this a Normal bug. Not in the mood to change it over Webchick though.

Let's split the difference at "major". But I don't think we can ship with this broken, personally.

I agree with not shipping this with it broken. It would really undermine Drupal credibility to have a bug this obvious in release. Tagging "Release blocker" (it has some 7.x tasks and 1 8.x task?).

@justafish's analysis looks correct to me. The problem is %user_uid_only_optional, which happens to load the wrong user account for the case $arg == '%'.

Next steps:

1) Re-implement the tests in the way I stated above.

2) Remove that $arg == '%' from the %user_uid_only_optional argument loader and see what else breaks. The breadcrumb tests at least should pass with that.

oops. Stale browser tab.

Aha! That was irony. Sorry I didn't realize that ;)

...stale tab too. Sorry.

Okay, here are the user breadcrumb tests using assertBreadcrumb in MenuBreadcrumbTestCase. Several changes:

1. Added a web_user and changed the web_user defined at the end to prived_user, which more accurately reflects what that user is.
2. The most important test, where we're not messing with link titles, admin_user will look at its own user page, user edit page, and then look at web_user's user page and user edit page. With assertions for each.
3. All user breadcrumb assertions besides the first two are now setup with the admin_user logged in and testing the web_user's user page and user edit page.

#3 might be too wild and crazy of an idea. I like cutting down the redundancy a bit though.

Edit: once again, this should fail in 6 places currently.

menu_get_item and menu_link_get_preferred are returning values with the URL arguments in the data-set. These functions look correct.

When the menu system processes menu_tree_page_data, the URL arguments aren't passed as parameters to menu_build_tree.

Probably, either menu_build_tree or menu_tree_page_data will need to change, but I'm not sure exactly what's required right now. I'll come back to this tomorrow - please post advice/solutions/patches in the meantime :-)

I can't reproduce the title bug while the breadcrumb is equally broken on Drupal 6. If you can reproduce the title issue please screenshot. Edit: nk is uid 1, chx is uid 2. Drupal 6 says "my account" , Drupal 7 says "nk" in the breadcrumb. It's the same link however.

And unless someone can reproduce the title bug, this is not a release blocker: anything that did not block Drupal 6 (see "my account" in the breadcrumb? you can try on drupal.org too!) should not block Drupal 7.

Um, sorry, it's only user/2/edit that has a broken title? That's the issue. We do not fix the breadcrumb, that's a different issue.

Fall back to Drupal 6. This is hopeless.

Well, user/2/shortcuts too (see @webchick's screenshot). I've looked at user/2/track and user/2/contact and they are fine however.

This patch gets us out of Dodge.

Sure! we can do shortcuts too. Note to anyone trying to fix the breadcrumb: the my account (d6) / username (d7) link is the same as the one that gets into the breadcrumb. The very scaffolding to separate them is missing. A breadcrumb_alter might be a hackish fix in another issue if you want. Proper fix is unavailable AFAIK. As the title comes from the same place, proper fix is also unavailable.

Test is already included.

Great, #47 fixes the title on the edit and shortcuts screens. A stray backslash snuck in here:

@@ -1,4 +1,4 @@
-<?php
+\<?php

Hm, I thought I ran my tests and they passed. It's very curious -- the command line runner passes UserEditTestCase, the user interface runner ends with an error before the batch but the error screen just shows passes. The bot have found a number of seemingly unrelated issues failing. Needs a followup issue! The things you find with a typo.

It failed because MenuBreadcrumbTestCase is still expecting the breadcrumb to be the newer way.

Was this new way to display user page title (and breadcrumb is a separate issue now?) a part of UX design changes? If not, then I guess this rollback to D6 behavior is fine.

This now passes.

@mradcliffe: Can you re-roll #37 and just do what I stated in #26 and #32? This patch should not touch any of the other breadcrumb assertions.

I'm not sure what chx is trying to achieve by derailing this issue first by not reading the issue and afterwards with those patches.

It's always the same mess. Attached patch implements the test procedure and is therefore expected to fail. Reproduce first, then fix. Anything else does not make sense.

I'm sorry, but there is no possible way to make this remotely work, because:

We have gazillions of different expectations for the current mess of user path and user/% path links:

• user itself should not show up in any menu, only in the breadcrumb for user register, password, and login pages.
• user/% should show up on all pages in the user menu, containing the title "My account" or the current user's name.

  Note that the link in the user menu is the untranslated router item; i.e. user/% itself. We expect % to be translated into the current user's uid, but only for this link.

• user/% should appear in the breadcrumb when visiting user/123/edit, whereas % should be naturally and logically translated with 123, not the current user's uid. This entirely contradicts the last bullet.
• user/logout should appear in the user menu, next to user/%, but none of the other sibling paths should appear (such as user/register or user/password, and whatnot).
• Some totally insane performance optimizations have been implemented through menu to_arg callbacks and custom menu argument loaders. Title callbacks, page callbacks, and access callbacks therefore try to handle both an integer function argument, but also a user account object as argument. Some of them are only able to handle an integer, and don't support a user object at all.
• This mess was not introduced by the breadcrumb patch, but merely revealed through it. The breadcrumb tests already contain assertions for the case when there is a menu link for a user account, and those do work as expected. We also have no problem with page titles or whatnot. The actual problem is that we have multiple, entirely different expectations for the % path argument placeholder replacement value, and an entire mess of performance optimizations that lead to invalid path argument replacement values.
• Ultimately, the expectation of the breadcrumb tests is the only valid expectation. No other untranslated link path like user/% appears in any menu, and therefore, this entire problem space is limited to our other weird expectations of how this untranslated link should appear (on [any] pages that are not even related to the user account page).

  We have to

  1. Replace that untranslated link in the user menu with an actual, valid menu link. Likely means that something along the lines of user/me is required.
  2. Remove all of the invalid performance optimizations. If required, they have to be re-implemented differently.

Would it be possible to avoid if (is_object($uid))?

We have to
[...]
2. Remove all of the invalid performance optimizations. If required, they have to be re-implemented differently.

Not sure what has been left unclear. It would really help if you would carefully read issue follow-ups.

And it would tremendously help if you would stop a) calling me out which I have despite your claims otherwise I never did. I have talked to you extremely patiently for extreme long time b) throwing around generic claims. How am I supposed to know that this is such a thing you vaguely hinted at? I can't even count how many times I asked you to speak in specifics, concrete examples.

Something along the lines of attached patch. I'll explain the remaining problem in pictures, as I seem to speak Spanish.

What I have "tried to achive" is fixing the edit / shortcut page title . I have my doubts as I have expressed above about our abilities to prepare a proper patch for D7. As far as I understand the breadcrumb contains a copy of the menu link pointing to my account therefore it will always be broken unless you change where the breadcrumb comes from. So I have thought it is impossible to fix the breadcrumb in Drupal 7 and have rolled back the page title to Drupal 6. Could you please explain me where I am wrong? Does the breadcrumb not contain a copy of the "my account" link? If it does (and your screenshots indicate that), then what does ripping out existing performance optimizations buy us versus the posted , passing , ugly rollback?

The patch in #62 has a few issues:
- php notice: Notice: Undefined index: localized_options in menu_navigation_links() (line 1741 of /srv/www/vhosts/d7/HEAD/includes/menu.inc).
- 2 instances of "My account" appear in the top-right menu.
- the second of the "My account" links in the top-right link to "user/%" which does not work.

The patch in #53 also has issues:
- the <h1> title (on user/xxx) is correct, but the breadcrumb gives the url/text of the current logged-in user, not the user whose account is being viewed.

Neither of the patches in #62 and #53 resolve the issue's cause, which is that the API functions (e.g. menu_get_active_trail) return incorrect data, because some of the APIs are not being passed the correct data.

This patch attempts to resolve the issue by applying the current path-context to the APIs, so the correct data is returned, however the root cause is that user/% can have 2 different contexts on the same page: in some places (e.g. breadcrumbs, menu-active-trail) it will be the context of the current URL, in others (e.g. the top-right user menu) it should be without context (and so provide a link to the current logged-in user's account).
This patch can't address that problem.

Issues with this patch:
- on user/xxx the menu text/url in the top-right (secondary-menu) will point to user/xxx instead of the currently-logged-in user.
- patch uses arg(). There may be a better-performing alternative.

Solved it. Breadcrumb tests pass. Didn't look at the other test failures.

Although I believe it is wrong, restored %user_uid_optional argument loader and to_arg function for Blog, Tracker, and other modules.

Worth waiting for the testbot results, but manual testing of sun's patches in #67 and #68 both apply and appear to work.

My manual tests are:
> in an environment with 2 users (uid1 and a new test user uid 14 with username "test_user_foo")
> browse to user/1: check: top-right "My account" link has correct text and href, and that breadcrumb shows "Home"
> browse to user/1/edit: check: top-right "My account" link has correct text and href, and that breadcrumb shows "Home >> admin"
> browse to user/14: check: top-right "My account" link has correct text and href, and that breadcrumb shows "Home"
> browse to user/14/edit: check: top-right "My account" has correct text and href, and that breadcrumb shows "Home >> test_user_foo"
> browse to node/1: check: top-right "My account" has correct text and href, and that breadcrumb shows "Home"
> browse to node/1/edit: check: top-right "My account" has correct text and href, and that breadcrumb shows "Home >> test node" (where "test node" is the node title)

I've no preference between #67 and #68: they both appear to work (from the manual tests: lets see what testbot says tho!)

In Drupal 6, the breeadcrumb says "Home › My account" when I am an admin or a normal user viewing another user's account - thus, fixing that aspect is NOT part of the critical bug. It has a breadcrumb in Drupal 6 of "Home › My account › XXX" When I am user 1, editing the account of user XXX, so even that would not be a regression.

Tried #68 and it fixes the title and breadcrumbs to what I would expect as a new Drupal user (i.e. not someone used to the d6 way of doing it). So it fixes the regression and improves Drupal's usability as well.

Since when does this work?

+    // Ignore router path inheritance to make the Logout link appear on the
+    // top-level of the user menu by default.
+    'plid' => 0,

If it does work it's only by accident and should not be used in core.

I've tried to figure out what is going wrong in those OpenID tests, but was not able to identify the cause. They also fail for me locally. For some unknown reason, the hashed password in the user password reset URL is different to the expected hash that is re-generated and tested against in the user/reset page callback. However, that must be something entirely different, unrelated to the user menu changes, because the page callback gets the $hashed_pass argument -- it's just not the correct value, and only in those OpenID tests (for any reason).

Apparently, all other user registration tests are passing, so I'm really not sure what's going on there. I recognized that the OpenID tests are rebuilding the password reset URL, instead of parsing and reading out the URL from the sent out e-mail.

re: #71: If it was wrong in D6 already, then you are basically right in that we don't have to fix the duplicate breadcrumb links to user and user/# in the breadcrumb. However, as I tried to explain in #57, we have to remove the weird logic that tries to output the untranslated user/% link anyway, which is what the latest patch already achieves by restoring D6's "My account" link pointing to "user" instead of "user/%". Only putting "user" in the user menu, forcing "user/%" to stay in the default navigation menu (and which therefore also eliminates the need for any performance optimization for the "user/%" path, which was only done, because the user menu containing the user/% link was output on all pages).

re: #73: We are facing the problem that the user menu should contain a link to "user" (My account) and "user/logout" (Logout). I agree that manually setting 'plid' on user/logout is a crappy hack, but as of now, I don't see any other way to disable the menu system's automated menu link parenting logic. This problem is new to D7, because we changed the "logout" path to "user/logout". In D6, the paths "user" and "logout" were able to happily live next to each other in the menu, because "logout" was not registered below "user". We only have this issue in D7, since both links should appear in the same menu, but still next to each other. After all, forcing 'plid' to 0 only affects and disables the automated router item parenting logic in menu_link_save(), when the corresponding menu link has not been customized.

In the end, I don't see a more correct solution than the current patch. What remains is to get the tests passing.

Resolved/reverted another change in expectations: Anonymous users should see "User account" as link title, not "My account", in case the link to "user" is output.

Also clarified comments.

Removed a needlessly duplicated comment in the added test assertions.

Let's move /logout back to the top level. Why does it need to be under /user?

Fixed those OpenID tests. This one should pass.

This should not be committed with the plid hack.

We need to either change the path back to just /logout, or user module needs to implement hook_menu_link_alter() to provide the desired fix-up.

Trying again. Forgot that testbot runs without clean URLs.

Hm! Closer inspection of the remaining test failure makes me believe we're facing the last remaining bit of #550254: Menu links are sometimes not properly re-parented again.

So let's simply try whether merging that patch makes the upgrade tests pass.

Alright. Let's try hook_menu_link_alter() then.

I don't really get what's happening in that upgrade path. One last try.

So is everyone happy now?

+++ modules/system/system.install	5 Oct 2010 13:17:12 -0000
+++ modules/system/system.install	5 Oct 2010 13:17:12 -0000
@@ -1981,11 +1981,23 @@ function system_update_7015() {

I've read and tried to understand the update queries that are performed in system_update_7015(), but I have no clue why they have been done in this way, and why the "Log out" link worked (?) previously... I didn't verify, but I suspect that the (non-customized) link was not moved to the user menu upon upgrade previously, so tests merely passed, because the link still appeared in the Navigation menu. At least, that's my bet.

Powered by Dreditor.

Still works from a front-end clicking-around point of view, good work!

#88: drupal.user-breadcrumb.88.patch queued for re-testing.

I'd actually prefer the 'plid' definition in hook_menu(), as it keeps the special definition close to the router item definition (the user_menu_link_alter() implementation is many code lines away), and effectively leads to the same result. But anyway, I'm happy either way.

wow what a trick! congratulations! I never though it even possible to fix this issue but moving the link aroundinto another menu... brilliant. just sheer genius.

Both pwolanin and I seem not to like that 'plid' trick, the current approach is so much better.

+ * Parses the last sent e-mail and returns the one-time login link URL.
Core uses email.

+  // default (i.e., unless it has been customized).

No comma after 'i.e.'.

-  // Use %user_uid_only_optional here to avoid loading the full user for
-  // basic access checks.
-  $items['user/%user_uid_only_optional'] = array(
+  $items['user/%user'] = array(

So does this revert back to loading the full user everywhere? Did anyone check?

Core uses email.

No, not that I know of. I used e-mail everywhere, and I used it a lot.

No comma after 'i.e.'.

No wonder I still see so many instances, even after correcting many. Some of the largest grammar style guides precisely state that there should be a comma after "i.e." and "e.g.". Since most of them are printed books, I can only reference to insightful online style guides like http://www.grammarbook.com/punctuation/commas.asp

So does this revert back to loading the full user everywhere?

As in D6, the user/% path arguments are only loaded when actually requested. The link to "My account" that appears on all pages no longer links to user/%, but the static /user path, which doesn't load anything.

i.e., vs. i.e. is American English, :(

e-mail, sorry bad grep.

user/% I'm going to verify either way a bit later.

Double checked user_load() and it's OK. One day I will post the British English translation for Drupal core.

Ok, it seems the latest patch meets the menu maintainers' requirements, and has been tested to work successfully. Comes with tests to prove it, too.

Committed to HEAD. Thanks a lot, all!

As a note, the patch did remove the function "user_uid_only_optional_to_arg".

I accidently used that instead of %user_uid_optional in userpoints D7 port (Patch to change that already exists and not because of this issue...) and this leads to a FATAL error with the current HEAD of userpoints.module.

Sounds like an API change to me, maybe someone else was using it already? :)

1. This patch seems to have caused #937916: Anonymous users see a "User account" link on the front page of a new site.

2. It also seems to have introduced a design change that wasn't really discussed explicitly above. See the attached screenshots. Whereas the user menu on the top right used to have a link with the current logged-in user's name ("David" in the case of the screenshot), it now goes back to the D6 way of saying "My account".

Regardless of whether #2 was intentional or not, it seems to be a large change to have made without any design discussion. Should we reopen this for that?

Adding a tag.

Yes we have to reopen this...

If this needs work, it also needs a new test.

Hm.

1) Perfectly possible that I'm completely wrong, but I was under the assumption that anonymous users would see a "User account" link; i.e., as fall back when the user login block is not enabled? (At least, didn't this exist in D6?) If I'm wrong here, then it's merely a matter of replacing the 'access callback' with 'user_is_logged_in'.

2) Actually, the original design documents contained "My account" as link title, not the user's name. The user's name was only supposed to appear in the Toolbar, but not in the user menu. Since the previous implementation was built off the existing user/% router path, there was no easy way to _not_ display the user's name, as that router path information is used for other purposes. So I recalled that various people were not happy with the user name appearing there, and because this change allowed for "My account", the menu title callback shows that. Like with 1), this can be easily changed by adjusting the title callback to not return "My account", but format_username($GLOBALS['user']).

No performance penalty or technical problems involved for both changes, so I'm leaving decisions to the UX team.

That said, "User account" is the expected page title for the user/login, user/register, and user/password pages, and the title callback is responsible for that. If we decide to hide the link for anonymous users, then we likely need hook_menu_link_translated_alter(), so we always allow access to the router item (like now), which normally always shows the menu link, but if user_is_anonymous(), then we dynamically set 'access' to FALSE.

If it's worthy of discussion, it's worthy of a test, so that regression bugs are easily recognised.

This patch checks that:
- the secondary-links menu contains a "My account" link pointing to "/user".
- the secondary-links menu contains a "Log out" link pointing to "/user/logout".

In D6 you definitely did not see a My account link for anonymous. This ingenious fix might need to be rolled back :( and manarth, thanks for the test .

The return of "My account" (for logged-in users) is a good thing. It's an important link that should be easy to find when you're looking for it.

- If the user forgets their username then they are going to have a difficult time getting to their user page where they can see it. :-O
- Having cross-site standards such as "My account" improves the usability of all of them.
- A lengthy username could make the toolbar look odd, which is fine, but I as a designer wouldn't want it messing up the page layout too. "My account" OTOH is a nice predictable bit of text.

Attached patch hides the "User account" link for anonymous users.

While we're at it, let's please put the user_uid_only_optional_to_arg() function back. I clearly wasn't paying attention when I reviewed this patch, but that's an unnecessary API change. And just because User module doesn't need it doesn't necessarily mean nothing in contrib needs it.

And it causes "Fatal error: undefined function user_uid_only_optional_to_arg()." when you update the core files to the latest version. Sad panda.

This patch:

- Adds the tests in #106 (with minor tweaks to support non-clean-urls)
- Adds Sun's patch in #109
- Adds a new assertion for the patch in #109
- Restores the user_uid_only_optional_to_arg function.

+++ modules/user/user.module	11 Oct 2010 18:34:41 -0000
@@ -1807,6 +1821,15 @@ function user_admin_paths() {
+ * ¶
+ * @todo rethink the naming of this in Drupal 8.

1) Trailing white-space.

2) The @todo needs to be updated. Actually, the phpDoc description (not summary) should state:

"
Deprecated. Use %user_uid_optional instead.

@todo D8: Remove.
"

+++ modules/user/user.test	11 Oct 2010 18:34:42 -0000
@@ -1207,6 +1207,58 @@ class UserAutocompleteTestCase extends D
+    // Log in and get the homepage
...
+    ¶
...
+      ':text' => 'My account',
+      ));

1) Comments should form proper sentences; i.e., end with a period.

2) Trailing white-space (also elsewhere in the test).

3) Wrong indentation (also elsewhere in the test).

+++ modules/user/user.test	11 Oct 2010 18:34:42 -0000
@@ -1207,6 +1207,58 @@ class UserAutocompleteTestCase extends D
+    // For a logged-in user, expect the secondary menu to have links for:
+    // "My account"
+    // "Logout"

We can remove the colon and just continue the sentence with:

'for "My account" and "Logout".'

Of course, wrapped at 80 chars.

Powered by Dreditor.

Updated patch to address the issues in #113.

Fixing broken patch from #114.

Thanks for re-rolling, manarth. I think this patch is ready to fly.

Huh. Interesting. Is that the only way to fix this? I'm confused about the layer of indirection here. We're faking that we've just translated a menu link when we haven't. Why don't we just put these lines:

+  // Hide the "User account" link for anonymous users.
+  if ($link['link_path'] == 'user' && $link['module'] == 'system' && user_is_anonymous()) {
+    $link['access'] = FALSE;
+  }

into user_menu_link_alter() directly?

At the very least this needs a butt-load of comments to explain why we're doing something so odd. But I'm not really pro- this approach unless there's an extremely good reason, because this is a complete WTF.

webchick:
"Why don't we just put these lines:"

<?php
+  // Hide the "User account" link for anonymous users.
+  if ($link['link_path'] == 'user' && $link['module'] == 'system' && user_is_anonymous()) {
+    $link['access'] = FALSE;
+  }
?>

I gave that a try, and the tests in the patch fail (the secondary menu isn't removed: anon users see a "User account" link). I presume sun gave that a try, and switched to faking the translation because regular hook_menu_link_alter can't do this.

Definitely open to other ideas. FWIW, I've attached my (failing) patch so others can see if I've missed anything.

I did not look up whether there is a new/better way to pass the 'alter' option along to the menu link for router items. The implementation in #116 is the one that is known to work since Drupal 6.

http://api.drupal.org/api/function/hook_menu_link_alter Alter the data being saved to the {menu_links} table being saved so i dunno what do you want from that poor hook? But, care to remind me why we can't put that check into the access callback of 'user' ...?

We do not want to limit access to the router item. We only want to hide the link that points to it, in case the current user is anonymous. Therefore, the router item's access callback is no help.

The proposed solution simply leverages hook_translated_menu_link_alter() to conditionally hide the link for anonymous users. Authenticated users are expected to see the link.

AFAIK, putting the information directly into the router item is not officially supported/wanted:

  $items['user'] = array(
    // ...
    'options' => array('alter' => TRUE),
  );

Therefore, the indirection from hook_menu() to hook_menu_link_alter() to hook_translated_menu_link_alter() is required to invoke user_translated_menu_link_alter() for that menu link.

The difficulty is that $item['access'] must be true for all users, but $item['hidden'] must be conditionally set for page-requests from anonymous users.

The menu system provides a callback-hook for access, but only a boolean for hidden, so this must be checked on each page request (it cannot be set in hook_menu or hook_menu_alter because the output from these is built then cached).

I've taken the patch in 116, added comments to try to explain the code, and switched $link['access'] = FALSE; to $link['hidden'] = TRUE; to make it clearer what we're trying to achieve.

(NB: the word 'translated' in hook_translated_menu_link_alter is misleading: nothing to do with language translation. Simply means altered-per-request.)

Sadly, I have to add this to the beta blockers list. When you try to update beta1 to HEAD you get:

Fatal error: Call to undefined function user_uid_only_optional_to_arg() in /Users/webchick/Sites/core/includes/menu.inc on line 776

Shortened + rephrased comments. Also changed 'hidden' from TRUE to 1, as that is the menu system's internally used value.

.

Breaks beta-beta upgrad path apparently.

Ok, since #126 is essentially the same as #124 with slightly re-worded comments and a TRUE > 1 fix, I'm going to count that as an RTBC. sun's argument for scaling back the comments is that we shouldn't get into that level of API detail within a function body. I agree. However, manarth's description is far better than what's currently at http://api.drupal.org/api/function/hook_translated_menu_link_alter/6 so I'd like to see a follow-up issue to vastly improve the documentation of that hook. Most people will have absolutely no idea it could be used for this purpose.

Committed #126 to HEAD.

bunch of crossposts

hahaahaha cross posts while fixing crosspost

Follow-ups:
#950276: Improve documentation of hook_translated_menu_link_alter()
#950278: Change hook_translated_menu_link_alter() into hook_menu_link_translated_alter()