Updated at comment #112

Problem/Motivation

'User Account' menu link is displayed to anonymous users browsing a Drupal 8 site on many pages (any page that is not on the /user/% path). This is a regression from previous versions of 8.x and Drupal 7, which contained code to explicitly hide this particular menu item for anonymous users.

Steps to reproduce

  1. Install latest D8, with either the minimal or standard profile
  2. Log in as admin
  3. Create a new content type 'test'.
  4. Create a new content page, of type 'test'.
  5. Log out of the site
  6. Visit /node/1

Bad Display: The "User Account" menu link is visible to anonymous users.
Expected Display: Main-menu-links, 'Login' block, node/1 content, and normal header and footer displayed for anonymous users.

Other issue encountered

The (UserAccountLinksTest.php) failed to validate that the menu link is hidden for anonymous users on D8 test runs. Modifying the test exposed the bug.

Proposed resolution

  1. Re-implement hook_translate_menu_link_alter from D7 in D8 (which would mean less change for contrib). At this point that constitutes an API change. Apply 1912806_98_Regression_User_Account_menu_on_front_page.patch as a possible workaround.
  2. Add a second invocation of hook_menu_link_load at the end of _menu_link_translate - "The basic problem with invoking hook_menu_link_load from multiple places is that it will be passing different variable 'type's(array vs object) for the different invocations." #112
  3. Create a more meaningful/less confusing hook called hook_menu_link_postload(post_load?)in instead of hook_menu_link_load that is also intuitively connected to hook_menu_link_presave? #87

Remaining tasks

  1. Further investigation needed- Needs review by someone familiar with the hooks involved (hook_translated_menu_link_alter(), which should be replaced with hook_menu_link_load() in D8, but the latter is not called at the right time to effect the secondary menu [user menu]...)
  2. Reroll #99 to remove user_menu_link_load because it is not called the times it needs to be called.

User interface changes

TBC

API changes

TBC

#916388: Convert menu links into entities
#1842858: [PP-1] Convert menu links to the new Entity Field API

Original report by dcrocks

On versions of 8-x dev since 2/8/13 the 'Secondary links'(User account) menu is now visible on all pages to anonymous users. This occurs with all themes using the core page.tpl.php or using the technique therein to display secondary links. This is a regression from previous versions of 8.x and Drupal 7.

CommentFileSizeAuthor
#155 1912806-155.patch3.91 KBamateescu
#155 1912806-155-test-only.patch927 bytesamateescu
#155 interdiff.txt927 bytesamateescu
#152 1912806-152.patch3.72 KBamateescu
#147 1912806_147_Regression_User_Account_menu_on_front_page.patch5.16 KBdcrocks
#143 menu_links-1912806-143.patch2.08 KBmarthinal
#139 menu_links-1912806-139.patch2.25 KBmarthinal
#138 menu_links-1912806-138.patch2.13 KBmarthinal
#135 useracct.jpg64.17 KBdcrocks
#127 1912806_125_Test_secondary_links_on_front_page_fail.patch1.29 KBdcrocks
#127 1912806_125_Regression_User_Account_menu_on_front_page.patch3.54 KBdcrocks
#125 1912806_125_Test_secondary_links_on_front_page_fail.patch1.29 KBdcrocks
#123 1912806_123_Test_secondary_links_on_front_page_fail.patch747 bytesdcrocks
#122 1912806_122_Test_secondary_links_on_front_page_fail.patch875 bytesdcrocks
#120 1912806_120_Test_secondary_links_on_front_page_fail.patch917 bytesdcrocks
#119 1912806_119_Test_secondary_links_on_front_page_fail.patch864 bytesdcrocks
#117 1912806_117_Test_secondary_links_on_front_page_fail.patch864 bytesdcrocks
#117 1912806_117_Regression_User_Account_menu_on_front_page.patch3.1 KBdcrocks
#111 Screen shot 2013-07-30 at 9.05.11 PM.png47.7 KBekl1773
#99 1912806_98_Test_secondary_links_on_front_page_fail.patch864 bytesdcrocks
#99 1912806_98_Regression_User_Account_menu_on_front_page.patch3.28 KBdcrocks
#98 Screen shot 2013-07-24 at 11.10.56 AM.png51.94 KBekl1773
#98 Screen shot 2013-07-24 at 11.11.56 AM.png78.66 KBekl1773
#98 Screen shot 2013-07-24 at 11.12.50 AM.png66.71 KBekl1773
#98 Screen shot 2013-07-24 at 11.26.08 AM.png55.17 KBekl1773
#84 -front-page.jpg57.68 KBdcrocks
#84 404responsepage.jpg44.91 KBdcrocks
#80 1912806-80_test_secondary_user_link.patch2.18 KBmarthinal
#65 1912806_65_Test_secondary_links_on_front_page_fail.patch877 bytesdcrocks
#65 1912806_65_Regression_User_Account_menu_on_front_page.patch2.77 KBdcrocks
#63 1912806_63_Test_secondary_links_on_front_page_fail.patch953 bytesdcrocks
#62 1912806_62_Test_secondary_links_on_front_page_fail.patch938 bytesdcrocks
#59 1912806_57_Test_secondary_links_on_front_page_fail.patch905 bytesdcrocks
#56 1912806_55_Test_secondary_links_on_front_page_fail.patch820 bytesdcrocks
#56 1912806_55_Regression_User_Account_menu_on_front_page.patch2.69 KBdcrocks
#52 1912806-52_test_secondary_user_link-do-not-test.patch1.11 KBmarthinal
#45 1912806_45_Regression_User_Account_menu_on_front_page.patch2.84 KBdcrocks
#32 useracct.jpg39.1 KBdcrocks
#29 1912806_28_Regression_User_Account_menu_on_front_page.patch1.18 KBdcrocks
#27 1912806_27_Regression_'User_Account'_menu_on_front_page.patch1.18 KBdcrocks
#22 drupal8.account-links-anon.22.do-not-test.patch3.32 KBsun
#13 1912806-fix-broken-test.patch747 bytesjthorson
#13 1912806-fix-test-and-bug.patch1.54 KBjthorson
#11 1912806-hide-account-link-for-anonymous-11.patch825 bytesjthorson
#8 1912806-User-account-menu-on-front-page.patch580 bytesjulien
#2 useracct.png27.14 KBdcrocks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dcrocks’s picture

Here is an image of the latest 8.x front page.

dcrocks’s picture

FileSize
27.14 KB

Sorry, clicked too fast.

dcrocks’s picture

May be too hard for me to solve. I think this started with #916388: Convert menu links into entities and I think it is because "user_menu_link_load" does not treat its operands as objects, but I have not been able to fumble my way to a solution. Any suggestions would be helpful.

dcrocks’s picture

Component: Bartik theme » menu system

Since this happens with stark as well, it isn't a bartik theme problem. I will try menu system as that is where I think the error got introduced even though the fix may be in user.module.

catch’s picture

Priority: Normal » Critical

Bumping to critical since it's a regression.

dcrocks’s picture

Updated summary

julien’s picture

in user_menu_link_load the variable menu_links sometimes doesn't contain the "User account" link so it doesn't hide it. weird because it does when you go to user pages like /user /user/register

julien’s picture

Status: Needs review » Active
FileSize
580 bytes

it does not only affect the front page, it fail also in /contact. Seems like the right fix should be to figure out why the menu_links variable in user_menu_link_load is not right, because on this path, it doesn't contain the /user link element, so the if statement to hide it doesn't work. This one fix it when returning the links of the secondary menu.

julien’s picture

Status: Active » Needs review
sun’s picture

Status: Active » Needs review

This is what I can share:

The 'alter' flag previously had to be enabled in hook_menu_link_alter() (since hook_menu() did not have an effect), which caused the flag to be saved into the {menu_links} table. Upon rendering a menu link that has the flag enabled, hook_translated_menu_link_alter() was invoked to allow for runtime adjustments.

hook_translated_menu_link_alter() still seems to get invoked by _menu_link_translate() in HEAD. perhaps we just need to change the hook name?

as long as the original hook still exists, the hook_menu_link_load() is definitely wrong, because it is not executed at runtime

_load() affects the loading of a menu link (before it is processed/cached/whatnot). the _translated_menu_link_alter() hook is executed whenever a menu link is rendered

jthorson’s picture

Status: Needs work » Needs review
FileSize
825 bytes

Changes from using hook_menu_link_load() to using hook_translated_menu_link_alter().

Status: Needs review » Needs work

The last submitted patch, 1912806-hide-account-link-for-anonymous-11.patch, failed testing.

jthorson’s picture

Looks like there was a test that was supposed to catch this exact scenario already ... second contains the test fix, as well as patch from #11.

EDIT: Never mind. Original css id was accurate for Stark. Not sure why it wasn't failing yet, though, as manual testing shows the secondary links appearing in Stark as well.

dcrocks’s picture

Isn't this going backwards? Instead of replacing the new hook by the old one wouldn't it be better to replace the call to the old hook with a call to the new one?

jthorson’s picture

Status: Needs review » Needs work

The last submitted patch, 1912806-fix-test-and-bug.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review

As per #10, the hook_menu_link_load() is only called on cache clear. Since we're changing behaviour based on whether the current user is logged in or not, I believe we need to evaluate at run-time, versus as the cache is being built.

In any case, as long as this code comment holds true, it seems reasonable to roll back to the earlier behaviour, and then move forward with the hook_menu_link_load() option later (though I still can't figure out how that was originally intended to work!)

As for the test piece ... I'll admit I've taken a step or two backwards trying to sort out why it isn't failing. :)

EDIT: Fresh install, using Stark, the secondary links appear when logged out. When executed via the Testing UI, verbose results show that they do not. Only real differences in the source are one-sidebar on local versus no-sidebars on site-under-test, and no 'nav' element on the site under test (the secondary menus are contained inside the 'nav' element on local).

julien’s picture

it seems that on the front page, hook_menu_link_load() is only called on cache_clear or menu_rebuild, but it is called if you go to /contact or other path as anonymous. But the menu_links variable parameter doesn't contain the user account element to do the if statement.
It look like this variable only contain path based on the current page path, on cache clear it contain other path like node/add or comment/%/., but not user so the function doesn't hide it.

amateescu’s picture

Status: Needs review » Needs work

The patch from #11 is correct. We'll be able to drop hook_translated_menu_link_alter() when menu links will go through proper entity rendering.

CNW for figuring out why that test doesn't fail.. this would've been fixed in the original menu links issue if it failed in the first place :(

julien’s picture

i agree with #14 on rolling back to a D7 function. I don't think it will be useful in fixing the issue in the long run.

jthorson’s picture

I suspected that part of the reason the test isn't failing, is that the verbose for drupalGet('') after the logout attempt is actually displaying the login form (user/login) instead of the front page ... and this symptom does not show up on any page with 'user' in the path. However, even after changing this to drupalGet('node'), the secondary menus do not appear during testing (but do on my local install).

I'm stumped.

sun’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

For the life of me, I'm not able to reproduce the bug.

We definitely need to change that user hook, but I suspect that there is something else (configuration) going on.

Since the test cannot reproduce it, is it possible that this only happens with the Standard profile?

dcrocks’s picture

Taking back a meaningless remark.

xjm’s picture

jlmeredith’s picture

Issue tags: +SprintWeekend2013

I am unable to reproduce this bug as well. I have tried several different methods. I am working on issue summary's and am now wondering if I should write one for this issue or close the issue. It does seem that there is some needed action regarding the user hook, but I am guessing that the issue should be reopened with a better description of the issue that is more technically described.

Please advise.

dcrocks’s picture

I only use the standard profile and this has been consistent with all versions of 8.x since 2/8/13. The tests may fail to show the error but there is no doubt that the $secondary_menu is not empty when the front page is displayed to an anonymous user. Whether it was the hook broken in #916388: Convert menu links into entities or something else in that patch I don't see how this can be closed.

dcrocks’s picture

I added the hook definition to menu_link.api.php and the implementation to user.module. It is already invoked in menu.inc. This works on a test version of drupal8. It needs more work but I wanted to see what would happen for the tests.

Status: Needs review » Needs work

The last submitted patch, 1912806_27_Regression_'User_Account'_menu_on_front_page.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review
FileSize
1.18 KB

Try again

dcrocks’s picture

Status: Needs review » Needs work

This needs more work. This patch reverts some of the changes in #916388: Convert menu links into entities. Besides inline documentation: what happens to menu_link_load and should this be clearly identified as a temporary reversion? Should menu_link_load be removed? Should an issue to implement menu_link_load be opened?

xjm’s picture

I only use the standard profile and this has been consistent with all versions of 8.x since 2/8/13. The tests may fail to show the error but there is no doubt that the $secondary_menu is not empty when the front page is displayed to an anonymous user. Whether it was the hook broken in #916388: Convert menu links into entities or something else in that patch I don't see how this can be closed.

@dcrocks, can you provide specific, detailed steps to reproduce this bug, since others are unable to? A numbered list of exactly what to do, something like:

  1. Install Drupal 8 core with the standard profile.
  2. Go to node/add/page and create a page entitled "Foo".
  3. Go to node/add/page and create a page entitled "Bar".
  4. Go to admin/mypath.
  5. Etc.
  6. Etc.
  7. Go to somepath and confirm that something is not displayed (see screenshot).
  8. Log out.
  9. Go to somepath again and confirm that something is displayed, which is not the expected behavior

Something along those lines; that's just an example of what they would look like. It would also help to include a screenshot illustrating the problem.

dcrocks’s picture

Status: Needs work » Needs review
FileSize
39.1 KB

All you have to do is install drupal 8, either as a clone or with the 8.x-dev tar/zip. I think the summary is a pretty straight forward description of the problem. And I did provide a jpg in #2. The secondary menu is not supposed to be displayed to anonymous users, on the front page or any other page available to them. This was the behavior until #916388: Convert menu links into entities was committed. The very specific code in drupal 7 and 8 to implement that behavior was replaced with similar code, but never really implemented. If you are not seeing this then you are not using stark or bartik with the current standard install.
I did provide a fix in #29. As the attached jpg shows the secondary menu is no longer visible in the top right on the front page or any other page available to anonymous users. Logged in users still see the secondary menu top right(with bartik). But this fix just reverts part of #916388: Convert menu links into entities and needs some additional cleanup as I indicated in #30. The choices are:
1) Make the regression the new standard.
2) Ignore the regression until someone fixes menu_link_load.
3) Clean up #29 and leave it.
4) Clean up #29 and open a new issue to fix menu_link_load, implement it properly, and remove #29.

dcrocks’s picture

The 'other' bug that has manifested itself here is why UserAccountLinksTest.php, which tests specifically for this situation, doesn't fail. So 2 bugs have been identified. That should probably go in the summary.

xjm’s picture

I install Drupal 8.x HEAD with the standard profile. What do I do next, specifically?

alexpott’s picture

I'm not sure I get this issue... from what I can discern... the test you are running is this:

  • Fresh install of drupal using standard profile
  • Go to front page as an anonymous user
  • See Bartik themed front page
  • Look for secondary link in top right hand corner as per jpg image in #2

The thing is the test is explicitly checking that that this secondary link is not there... see // For a logged-out user, expect no secondary links.

 /**
   * Tests the secondary menu.
   */
  function testSecondaryMenu() {
    // Create a regular user.
    $user = $this->drupalCreateUser(array());

    // Log in and get the homepage.
    $this->drupalLogin($user);
    $this->drupalGet('<front>');

    // For a logged-in user, expect the secondary menu to have links for "My
    // account" and "Log out".
    $link = $this->xpath('//ul[@id=:menu_id]/li/a[contains(@href, :href) and text()=:text]', array(
      ':menu_id' => 'secondary-menu',
      ':href' => 'user',
      ':text' => 'My account',
    ));
    $this->assertEqual(count($link), 1, 'My account link is in secondary menu.');

    $link = $this->xpath('//ul[@id=:menu_id]/li/a[contains(@href, :href) and text()=:text]', array(
      ':menu_id' => 'secondary-menu',
      ':href' => 'user/logout',
      ':text' => 'Log out',
    ));
    $this->assertEqual(count($link), 1, 'Log out link is in secondary menu.');

    // Log out and get the homepage.
    $this->drupalLogout();
    $this->drupalGet('<front>');

    // For a logged-out user, expect no secondary links.
    $element = $this->xpath('//ul[@id=:menu_id]', array(':menu_id' => 'secondary-menu'));
    $this->assertEqual(count($element), 0, 'No secondary-menu for logged-out users.');
  }
alexpott’s picture

And just to add... when I do the above steps... I do not see the secondary link in the top right hand corner...

dcrocks’s picture

@34: If you look in the top right hand corner of your new home page(when logged out) you will see the secondary menu displayed, when using bartik as your theme.. If you are using stark, it will be displayed under the main menu. It should not be there.

@35: Yes, that is the bug because it is passing when it should be failing. No matter what the test is actually sampling, when you install a current drupal 8 the secondary menu is displayed to anonymous users. At least I have many times in the last month. I install the tar at http://drupal.org/node/3060/release?api_version[]=7234 when I want a test version. I clone it when I want to build a patch. Where are you getting your version of drupal 8 from?

And lastly, the patch in #29 makes the secondary menu go away for anonymous users. And yes, the test still passes.

Berdir’s picture

This seems to depend on the context in which the menu rebuild is happening.

When I'm logged in and then do a cache clear on the performance tab, the link is displayed all the time, My account for logged in users and User account for anonymous users.

However, when I do a drush cc all and then refresh the page as an anyonymous user, the user account link is gone. And so is "My account" when I then log in again after that.

dcrocks’s picture

I tried a minimal install, and the secondary menu does NOT show up for anonymous users. So the tests would certainly pass. But in the minimal install the menu module is not enabled and, I think, the secondary menu is hard coded and not 'built', as in the standard install. I have to look at that more.
Also, are the $main_menu and $secondary_menu variables even built in a minimal install? I noticed that the primary menu isn't displayed on a minimal install either.

dcrocks’s picture

So it appears there is no bug in the tests, but there is one in the standard install. On a current minimal install of drupal 8 an anonymous user sees neither main menu nor secondary menu. On a current standard install of drupal 8 they see both. In drupal 7 and drupal 8 prior to #916388: Convert menu links into entities they saw only main menu. This starts with tests for $main_menu and $secondary_menu in the page.tpl's. But the path from there appears very long and twisty.

dcrocks’s picture

Issue summary: View changes

Generaalized summary

jthorson’s picture

This is not limited to standard profile ... I have updated the issue summary with Steps to Reproduce, using the minimal profile.

jthorson’s picture

Issue summary: View changes

Updated issue summary with steps to reproduce

jthorson’s picture

Issue summary: View changes

Updated issue summary

jthorson’s picture

Issue summary: View changes

Updated issue summary

dcrocks’s picture

The patch in #29 fixes the issue in the standard install and I think it would fix it for the minimal install as well. But all it does is put back the implementation before #916388: Convert menu links into entities. It would be better to implement hook_menu_link_load properly. It is defined in menu_link.api.php and implemented in user.module. But it needs to be invoked somewhere. The old hook was invoked in menu_link_translate. I'm not sure if the same place is appropriate here, or what form the invocation should actually take.

xjm’s picture

Aha! Thanks @jthorson. I see it now.

xjm’s picture

Issue tags: +Needs tests

Tagging for a test coverage fix.

dcrocks’s picture

I have spent some time looking at code and am not sure how to implement hook_menu_link_load. I am not even sure that it should be perceived as a replacement for hook_translated_menu_link_alter. So I have redone the fix in #29 and added some additional documentation. This fixes the regression in the standard install, and I think it will in the minimal install as well, but I haven't tested that yet. Please review the comments and test.

I think the test in UserAccountLinksTest.php need to be worked on but I'm not sure it needs to be in this issue. It is really working correctly, it is just doing its test against the wrong page.

dcrocks’s picture

I did a little research. This has happened before: #937916: Anonymous users see a "User account" link on the front page of a new site. It ended up being fixed in #925778: User edit title is broken (so is beta1-beta2 upgrade path). If you start out about comment #100 there is an interesting discussion about how hook_translated_menu_link_alter came about. That is also the fix that created the current tests. Related is #950276: Improve documentation of hook_translated_menu_link_alter().
I think the reasoning then applies now but, to me at least, calling hook_menu_link_load in _translate_menu_link would be confusing. But the existing name seems to be confusing too(see #950278: Change hook_translated_menu_link_alter() into hook_menu_link_translated_alter().

xjm’s picture

Thanks @dcrocks. That definitely underscores the need to fix the tests.

I think the test in UserAccountLinksTest.php need to be worked on but I'm not sure it needs to be in this issue. It is really working correctly, it is just doing its test against the wrong page.

Well, it does need to be in this issue, because of our testing gate. We need an automated test that fails against HEAD, but passes when combined with your fix.

Patch review:

  1. +++ b/core/modules/menu_link/menu_link.api.phpundefined
    @@ -11,12 +11,14 @@
    + * This hook is invoked from _menu_link_translate() after a menu link has been
    + * translated; i.e., after dynamic path argument placeholders (%) have been
    + * replaced with actual values, the user access to the link's target page has
    + * been checked, and the link has been localized. It is only invoked if
    + * $item['options']['alter'] has been set to a non-empty value (e.g., TRUE).
    + * This flag should be set using hook_menu_link_presave().
    

    Excellent documentation!

  2. +++ b/core/modules/menu_link/menu_link.api.phpundefined
    @@ -25,11 +27,25 @@
    + * @param $item
    + *   Associative array defining a menu link after _menu_link_translate()
    + * @param $map
    + *   Associative array containing the menu $map (path parts and/or objects).
      * @see hook_menu_link_presave()
      */
    + function hook_translated_menu_link_alter(&$item, $map) {
    

    So, reading, I'm not sure what a menu $map is. Can we either explain the structure, or reference the function where it is defined, like we did for $item here?

    We also need to hint the datatype here: http://drupal.org/node/1354#types
    (And might as well do so in the documented signature as well.)

    Also, there should be a blank line between the parameters and the @see.

  3. +++ b/core/modules/menu_link/menu_link.api.phpundefined
    @@ -25,11 +27,25 @@
    +/**
    + * This hook is defined here and has an implementation in usermodule,
    +  *but is not invoked anywhere.
    + * It was meant to be a replacement for the above hook_translated_menu_link
    +  * but it remains to be seen if it actually will have a similar purpose.
    + *    @todo implement hook_menu_link_load ¶
    +*/
    

    Just to clarify, was this added in 8.x? Also, we need a proper docblock for this still: http://drupal.org/node/1354#functions
    (Plus the whitespace is a bit wonky, and we usually want followup issues filed whenever we add an @todo. Also, function names should always have parens when used in docs, e.g. hook_menu_link_load()).

  4. +++ b/core/modules/user/user.moduleundefined
    @@ -1127,6 +1127,16 @@ function user_menu_breadcrumb_alter(&$active_trail, $item) {
    + * Implements hook_translated_menu_link_alter().
    + */
    +function user_translated_menu_link_alter(&$link) {
    +  // Hide the "User account" link for anonymous users.
    +  if ($link['link_path'] == 'user' && $link['module'] == 'system' && !$GLOBALS['user']->uid) {
    +    $link['hidden'] = 1;
    +  }
    +}
    

    It's odd to fix a bug by adding a new hook implementation. I think it would be better to do what @jthorson's patch does in #17.

Also, let's get the issue summary updated with a proposed resolution (listing the different resolutions proposed) and a clear explanation of this hook chaos. Thanks!

dcrocks’s picture

Thanx for the input. I'll try to address every comment but I think I will need help with the test system. I tested a minimal install and the fix worked but I don't know how to get to a 'front' page there there that isn't based on the user menu.

xjm’s picture

@dcrocks, you can set the front page to the test page, like so:


/**
 * Enable whatever and the test page.
 *
 * @var array
 */
public static $modules = array('whatever', 'test_page_test');

function setUp() {
  // ...

  config('system.site')->set('page.front', 'test-page')->save();
}
dcrocks’s picture

Could I do something like point to page 'node/add'? I know that returns an 'access denied' msg, but it also currently prints the 'user account' menu as well. If I apply the fix, it doesn't. Then I don't have to setup a temporary page.

xjm’s picture

@dcrocks, in automated tests, it's best to use test implementations whenever possible, for decoupling. Otherwise, we run into situations exactly like this one, where some existing element in core is used in a test and regressions go unnoticed because it turns out it has a special behavior. There have been tons of bugs in the field and entity systems that weren't caught because the tests relied on nodes, which, it turns out, were special in a lot of ways.

Also, relying on existing implementations in core means that the tests break when those things are changed (tests are code that needs to be maintained too). I had to spend hours decoupling tests from the node listing on the front page when we filed #1806334: Replace the node listing at /node with a view, for example.

So, in general, it's preferable to always rely on a path, page, field, form, whatever that isn't part of another module. :) Hope that helps.

marthinal’s picture

When verifying that "My account" is enabled we can obtain the fail. When disabling "My account" again I have no fails...

For the moment I don't understand why when verifying, the link appears...

xjm’s picture

@marthinal, that test uses the testing profile, which does not include the node listing /node following #1806334: Replace the node listing at /node with a view, so $this->drupalGet('node') may return a 404. So, let's go ahead and add the two lines I suggested from #49 and then we can go from there. :)

dcrocks’s picture

marthinal’s picture

@xjm, sure :) I was debuging and I wanted to show this result.

@dcrocks, yes I know it's working on the same test but if you run this patch you will see the fail.

Also manually if you disable "My account" from Menu links the link will dissapear .... so I was just testing this behavior from the test and found this.

dcrocks’s picture

Ok, I'll give it a try. Made some changes to docs and removed unnecessary hook sample.

Status: Needs review » Needs work

The last submitted patch, 1912806_55_Regression_User_Account_menu_on_front_page.patch, failed testing.

amateescu’s picture

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.php
@@ -58,6 +58,19 @@ function testSecondaryMenu() {
+/**
+* Enable whatever and the test page.
+*
+* @var array
+*/
+public static $modules = array('whatever', 'test_page_test');

LOL :)

There is no 'whatever' module..

dcrocks’s picture

Status: Needs work » Needs review
FileSize
905 bytes

Just the test

Status: Needs review » Needs work

The last submitted patch, 1912806_57_Test_secondary_links_on_front_page_fail.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review

Clearly don't know what I'm doing. Pause.

dcrocks’s picture

dcrocks’s picture

Status: Needs review » Needs work

The last submitted patch, 1912806_63_Test_secondary_links_on_front_page_fail.patch, failed testing.

dcrocks’s picture

OK, I hope this is it except for any cleanup in comments, etc.

marthinal’s picture

The issue with the test for the current bug was that the test didn't detect "User account" link (for anonymous users) and manually this link appears with minimal and standard installation.

Ok.

As I said in #52, without @dcrocks patch, if we verify that "My account" menu link is enable (from the test) we can obtain the fail. You could take a look at the previous verbose and also, there you can verify that the link appears.

At this point I don't know if we could verify what's going on with the current test and the current code... or directly work on @dcrocks patch ...

Hope this info helps :)

dcrocks’s picture

The visibility of the secondary menu for core themes is dependent on whether $secondary_menu is empty or not. When not logged on, $secondary_menu will be empty if the 'hidden' flag is TRUE, or if the current path includes 'user'. For confirmation, on a clean standard install of 8.x, while not logged in, if you go to any defined page, the secondary menu is visible, except pages with 'user' in the path(e.g. 'user' or 'user/register'). On those pages the secondary menu is not visible.

In the minimal install front-page is defaults to 'user'. So the reason the tests were not failing was that, for the secondary menu visibility test, it was testing a page that included 'user' in its path. So the change to the test in #65 is a valid use case.

As a note, it is clear that the 'user' menu paths have to be visible to anonymous users, but with the 'Login' block available on the home page additional links would be confusing. Hence the 'hidden' flag.

marthinal’s picture

@dcrock, Yes I understand what you mean but please read #21.

If you change in the test the '' to 'node' the link doesn't appear ... did you try this?

Maybe it's something with my installation so please guys confirm that ... :)

If you have this situation as described in #21 (and as I got in my installation ) please try in the test to verify if "My account" link is enabled going to 'node' (#52). There you will see the fail. Then go to 'node' without verify "My account" link and you will have no fails....

This situation is what I'm trying to describe.

dcrocks’s picture

If you go to $site/node you get a 'Page not found' error page and the secondary menu is not visible. This is true for any such errors on standard or minimal install. Interestingly, if you go to a page that gets a 'Access denied' error the secondary menu is visible. Don't know why these special cases have different behaviors. But in any case the test should be done against a valid page, not a non-existant page. That is why 'test-page' is a better use case.

marthinal’s picture

Check the verbose message before "No secondary-menu for logged-out users."

For 'node' we have not content generated ... not a Page not found

dcrocks’s picture

There is no page called 'node'. Try a minimal install of 8.x and see what displays with $site/node or $site/admin. Error pages are returned. And node is still a bad choice because the secondary menu won't be displayed on any 'page not found' error so the test still fails to fail properly. The test has to be done against a valid page.

marthinal’s picture

The problem is that the link appears in the situation explained before ... I understand that the final test need to be with another path. So maybe this is a normal behavior for the test... but I don't understand why

dcrocks’s picture

Here is what I understand:

Without the fix the secondary menu(displayed as 'User account', not 'My account') is displayed on all pages for an anonymous user(ie., not logged in) except, (1) pages with 'user' in their path(eg., user/register), and (2) certain error pages(eg., Page not found).

With the fix, an anonymous user never sees the menu. I haven't found an exception yet.

Remember the whole point is that the anonymous user should be able to see the user menus. But they shouldn't see 2 copies of the menu in different places on the same page.

dcrocks’s picture

To your other comment, $site/node on the standard install just returns the front page. On a minimal install it returns a 'Page not found'. Can't explain it but more than likely because the menu module is loaded on the standard install and not on minimal. I don't think this is relevant to the test.

The test was wrong and needed to be fixed. But a fix for the regression that was based on the new hook_menu_link_load would be better.

Berdir’s picture

/node is now a view and the minimal install profile does not enable views.module.

podarok’s picture

#75 minimal install profile do not creates any content types

xjm’s picture

Issue tags: -Needs tests

Thanks @dcrocks -- Good to see that the simple change to the test will expose the bug. I think that's all we need to do for test coverage.

We've strayed a bit off topic -- I believe what we need to decide now is which solution we'll use, the one provided in #65 (which works, but is really more of a workaround), or if there's a more complete solution.

dcrocks’s picture

I agree. It would be better to use the new hook. The question is where and when and how. In this case you have 2 hooks operating in tandem, one just before a menu item is created or updated, the other before it is printed to the page, thus providing a more dynamic nature to link handling. User.module takes advantage of this to say this particular menu item has special(non systemic) visibility rules and then later applying those rules to just that menu item.
But the placement of the invocation of hook_menu_link_load probably has to meet more than that one use case. It has to be someplace that every menu link goes through but not too early or too late. That just may be where hook_translated_menu_link_alter is called now.

dcrocks’s picture

The code to invoke hook_translated_menu_link_alter is still in function _menu_translate_link() in menu.inc. That function is called 7 times over 5 different modules. It doesn't appear effective on all seven of those calls to invoke the hook. For example, it is called when building the 'menu edit' form. I looked at function menu_link_load. It is called 7 times across 6 modules. There doesn't seem to be any real correlation between it and the way hook_menu_link_load is being used in d8. Maybe it should be called hook_menu_link_loaded? Anyone have any ideas about how hook_menu_link_load was expected to be used?

marthinal’s picture

Hi guys,

I think... I found where is the bug. To reproduce you need to use drush cc all. When you clear the cache as anonymous the link disappear. It's a problem adding the data to the cache. The menu link name is 'account' and is the same for logged in and anonymous users so this is why when we ask on the test for this menu, the link appears...

Attached a patch with the test modification. If you try the test alone (without the patch) you will see the Fail. adding the patch the bug is fixed.

Into the patch I have added the uid to detect a difference when caching but I think we need to talk about the best option .... I really don't know.

Hope it helps and if this option is good we don't need a regression maybe...

Berdir’s picture

That doesn't work.

a) It means we have to maintain a cache for every uid, not an option.
b) It only fixes this case, there could be other examples with different conditions.

The point is that it should be possible to somehow alter the menu links when they come from the cache, you could altered the cache for this specific use case.

marthinal’s picture

Absolutely using uid isn't the way but ... what is the difference between 'alter before' and 'alter after'... maybe we can detect if the user is logged in or not and make a difference with this concept before set the cache.

dcrocks’s picture

Your test is still not valid because it is looking at an invalid page. On minimal, page 'node' returns a '404 Not Found' header response. The test has to fail or succeed against a valid, legitimate, page. Neither minimal nor standard install shows the regression on '404' error pages. It must have to do with the way drupal handles the 404 response. Other error pages show the regression. In addition, there is already a test for 'My account' link is enabled. That is the default. I don't see how doing it twice helps.
And I don't see how this can be a cache issue. If the flag is set, the menu is not show, otherwise it is. No changes to link processing, just link visibility. This is all happening well after the link is loaded. Lastly, it is not the 'My account' link that is being shown. It is the 'User account' link, which is the top of the menu tree at 'Link_path'=>'user'(mlid=3, plid 0) while 'My account' is at 'Link_path'=>'user%'(mlid=19, plid=3).
It comes down to 2 things; (1) The user menus have to always be visible, and (2) the Secondary Menu is a special menu, coded outside the the standard menu flow, and is not always supposed to be visible if it points to the user menu. If some other menu was assigned to the Secondary Menu other than the user menu, it would always be visible and we would probably not be having this discussion.

dcrocks’s picture

FileSize
44.91 KB
57.68 KB

Another note. If you log in successfully and then point to a non-existing page, neither the primary, secondary, nor any other menu is visible on the 'Page not found' 404 response page. This occurs on both a minimal or a standard install. Perhaps that is a bug.

dcrocks’s picture

I tried to trace how the $secondary_menu variable gets set. The processing is the same for $main_menu. Function menu_navigation_links() is called for both with the name of the menu they are looking for. This is the only place in all of core that calls menu_navigation_links(). After a very long path through function menu_tree_page_data() it gets to function _menu_tree_tree_check_access() which is where function _menu_link_translate() is called, and where the hook is invoked that used to test for anonymous user and set the 'hidden' flag. At the end of menu_navigation_links, the 'hidden' flag is tested and the corresponding link is dropped from the output.
There is a lot of code and several cache and/or database calls along the way. The initial functions called are menu_main_menu() and menu_secondary_menu(). I don't know if it is possible to insert an early test for anonymous users because the possible interaction between main menu and secondary menu. Main_menu is always visible to anonymous users.

dcrocks’s picture

The only contrib module that I know implements 'translated_menu_link_alter' is DEVEL, which is currently still happy because the hook is still invoked. There certainly may be more. If the hook is scratched they need something in its place, probably with the expectation it is being called from the same place. When translated_menu_link_alter does get replaced, more contrib modules may have problems.

(Actually, DEVEL currently has a problem because they used hook_menu_link_alter which has been replaced by hook_menu_link_presave. They haven't provided their own fix yet.)

dcrocks’s picture

How about creating a hook called hook_menu_link_postload(post_load?) as more meaningful and less confusing than than hook_menu_link_load. It is also intuitively connected to hook_menu_link_presave.

dcrocks’s picture

Issue summary: View changes

Improve steps to reproduce...

no_angel’s picture

Attempted to update issue summary.

no_angel’s picture

Issue summary: View changes

no_angel - Issue Summary updated.

jibran’s picture

Issue summary: View changes

Updated issue summary.

dcrocks’s picture

I am still looking at this. The description for hook_menu_link_load() is:

"Alter menu links when loaded and before they are rendered."

It expects to receive an array of links. The previous hook, hook_translated_menu_link_alter(), which is still invoked from _menu_link_translate(), received a single link. That would indicate the new hook was expected to be called from some place else. _menu_link_translate() is called from _menu_tree_check_access() which is called from menu_tree_check_access(). Given the purpose, I would expect the requirement would still to be to modify a single link. I also think that would be the expectation of current users of the previous hook. The hook could be called with an array of links where _menu_tree_checks_access returns, or in _menu_link_translate with a single link as it is now. Does anyone have any thoughts about where this should occur or if it is desirable to adopt the new calling parameters?

dcrocks’s picture

It is important to note that the secondary_menu is not a real menu. It is derived from whatever "Source for the Secondary links" in structure/menus/settings is set to. It just so happens the default is the 'User account menu'. But if someone changed that setting to another menu they may want it visible to the anonymous user. Or not.

Right now the secondary-menu doesn't exist until some static code placed in the page.tpl.php in bartik and in the system module is executed. The code starts with a test to see if $secondary_menu is set. It is just as easy and as effective to test for ($secondary_menu && $logged_in). Same result as by using the hook. And the test would still work.

Soon the 'primary' and 'secondary' menus will be provided as blocks #1869476: Convert global menus (primary links, secondary links) into blocks. It would provide a better UI there if the menu's visibility to anonymous users could be toggled in the configuration for those blocks.

It would be easy to clear this regression and then an issue to implement hook_menu_link_load could be opened without having to worry about all this stuff.

ps. I hope someone looks at this once in a while.

dcrocks’s picture

Status: Needs review » Postponed

#1869476: Convert global menus (primary links, secondary links) into blocks makes a lot of changes. Even though it currently still depends on the 'hidden' flag it might offer other ways of handling this regression. It still bothers me that drupal goes all the way to build the menu tree and then says oops, I didn't really want this. So I think it best to postpone this issue on the other and then take another look.

mradcliffe’s picture

Status: Postponed » Active
Issue tags: +Needs manual testing

Let's keep this on the active radar per advice/comments on IRC.

It's old so it probably needs another Manual Test and update.

dcrocks’s picture

It is definitely still there.

xjm’s picture

Status: Active » Needs review
Issue tags: -Needs issue summary update, -Needs manual testing, -SprintWeekend2013

"Active" means that there's no patch in the issue.

#80: 1912806-80_test_secondary_user_link.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1912806-80_test_secondary_user_link.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review
dcrocks’s picture

ekl1773’s picture

Reproduced the issue on a fresh install of D8 according to the issue summary steps. The "User Account" link appears everywhere that isn't in the /user/ path, as expected. Screenshots include a variety of pages for emphasis, but I checked all pages on each browser just to be thorough.

Using Chrome and Bartik:
Screen shot 2013-07-24 at 11.10.56 AM.png

Firefox (in a /user/ page):
Screen shot 2013-07-24 at 11.11.56 AM.png

Opera:
Screen shot 2013-07-24 at 11.12.50 AM.png

IE8:
Screen shot 2013-07-24 at 11.26.08 AM.png

Testing patch #65: 1912806_65_Regression_User_Account_menu_on_front_page.patch.

Patch applies but shows a white screen of death after the installation is complete. Tested on my all-MAMP local setup, Gaelan tested on a MAMP/MariaDB install. Simpletest.me and the MAMP PHP error log show the same error:

Fatal error: Cannot access protected property Drupal\Core\Session\UserSession::$uid in /home/sf5c923510cbb121/www/core/modules/user/user.module on line 1034

Line 1034 is the patch implementation of hook_translated_menu_link_alter(), which was resurrected from D7. If hook_menu_link_load is now in D8, this patch should be revisited and rewritten using it (from what I've gathered in the comments).

Line 1034 is the if statement here:

function user_translated_menu_link_alter(&$link) {
  // Hide the "User account" link for anonymous users.
  if ($link['link_path'] == 'user' && $link['module'] == 'system' && !$GLOBALS['user']->uid) {
    $link['hidden'] = 1;
  }
}
dcrocks’s picture

Rerolled the patch in #65 against current head. Updated to correct usage. Unless something else is planned, this patch should go in. Please review.

dcrocks’s picture

Did some looking and still can't see the hook_menu_link_load called any where. However, both user.module and shortcut.module have implementations. Should we just rename hook_translated_menu_link_alter to hook_menu_link_load and still call it from the same place?

tstoeckler’s picture

@dcrocks: hook_menu_link_load() is called by the entity system, which automatically calls hook_ENTITY_TYPE_load() for any entity, in this case the 'menu_link' entity. I don't know about hook_translated_menu_link_alter().

dcrocks’s picture

User.module implements user_menu_link_load, but it apparently isn't working, as it doesn't perform its function, ie, prevent the user account menu being displayed to anonymous users by setting the 'hidden' flag.

dcrocks’s picture

Doing some experimenting and it appears the hook is not being called for the 'secondary-menu'. Added a var_dump in user_menu_link_load and it is triggered on all menus accept secondary(and probably primary) menu.

dcrocks’s picture

Did some more testing. hook_menu_link_load is not being invoked for the secondary menu, whatever menu is chosen as the source in the settings.

dcrocks’s picture

Referring back to comments in #10, #17, and #19, the patch in #99 is a workaround until other things happen. Hook_menu_link_load is being invoked, but not at the right time for this use case. If some one could review this patch this issue could be closed with a follow up.

dcrocks’s picture

An alternate solution would be to change the call at the end of _menu_link_translate() to call hook_menu_link_load instead of hook_translated_menu_link_alter. Not sure of the implications of calling the same hook from 2 different places.

dcrocks’s picture

I tried this call

\Drupal::moduleHandler()->invokeAll('menu_link_load', $item);

at the end of _menu_link_translate() to replace the existing hook call. But it fails because 'invokeAll' expects the second argument to be an array whereas an object is being passed:

Warning: call_user_func_array() expects parameter 2 to be array, object given in Drupal\Core\Extension\ModuleHandler->invokeAll() (line 312 of core/lib/Drupal/Core/Extension/ModuleHandler.php).

Still think a different hook should be used here(hook_translated_menu_link_alter or something else) to prevent any confusion on what is actually being passed. Which means a new hook is being added and I don't know if that will pass at this stage of D8.

dcrocks’s picture

When the secondary menu variable is passed to the hook at the end of _menu_link_translate it has type object but the 'foreach' statement in the hook implementation treats it as if it were NULL and ignores it. All other calls to the hook pass a variable of type array. Hook_translated_menu_link_alter treats it as a single dimensioned array and has no problem. I don't understand the problem here as objects are valid data types for 'foreach'.


ps. I verified the type by doing a var_dump(gettype($menu_links)) at the top of the hook implementation.

dcrocks’s picture

Someone needs to decide how to handle this. This issue is about a regression from D7 to D8. In D8, if the secondary menu's source is the User account menu, it is visible to anonymous users. In D7 it is not. In D7 hook_translate_menu_link_alter, called at the end of _menu_link_translate, was used to prevent showing the User account menu to anonymous users. That hook was dropped in D8. It was assumed that hook_menu_link_load would work in its place. But hook_menu_link_load is not called at the right time to achieve this, therefore D8 has a regression.

(1) Re-implement hook_translate_menu_link_alter in D8. At this point that constitutes an api change.
(2) Add a second invocation of hook_menu_link_load at the end of _menu_link_translate.

Solution (1) would be considered temporary, as discussed in #10, #17,and #19. And (2) has a problem in that if the menu link is the secondary menu, hook_menu_link_load passes an object to the implementations whereas in all other cases it passes an array. If the implementations of hook_menu_link_load is changed to accept just one or the other it will succeed in one case and fail in the other. However, the only implementation, at this point, that has a problem is user_menu_link_load. It could be changed to support both variable types.

So someone needs to decide which way to go. Both would require opening a follow up. In my mind (1) is the simpler. It would also mean that any contrib modules from D7 that use hook_translated_menu_link_alter, such as DEVEL, would not notice a change. The patch in #99 works for (1). I've tested a patch for (2). It would help if someone tested/reviewed #99. I'm leaving this until someone decides.

dcrocks’s picture

ekl1773’s picture

I can't weigh in on how to solve this, but I can test the patch! It doesn't sound like either option is really ideal. And there's no way to move hook_menu_link_load() to a place where it would include this secondary menu? (not sure how that works...)

Testing again in Chrome dev channel, Bartik theme, and the patch in #99 does indeed work. The User Account menu is no longer present on any page when logged out, whether that page is under /user/ or not. The patch does work in Firefox, Opera and IE8 as well, though I'm not including screenshots here.

Screen shot 2013-07-30 at 9.05.11 PM.png

The next step, from what @dcrocks has said, is to choose a fix and possibly close this issue with the temporary patch and a followup?

dcrocks’s picture

This has gotten to be a very long issue, mostly my fault. I looked at the issue summary to see if any more would help, but I think it is concise and accurate. But a few highlights:

1)This only affects the 'User account' menu link, not any other user menu items.
2)It so happens that the secondary menu, by default, points to the 'User account' menu after install.
3)Anonymous users should not see this menu because, well, technically, they don't have a user account.
4)The basic problem with invoking hook_menu_link_load from multiple places is that it will be passing different variable 'type's(array vs object) for the different invocations.
5)Using hook_translated_menu_link_alter at least gives a hint as to where it is invoked, plus contrib is used to it.

I would actually like to reroll #99 to remove user_menu_link_load because it is not called the times it needs to be called and is just a waste of cycles.

dcrocks’s picture

Issue summary: View changes

Some additional clarification.

ekl1773’s picture

Updated the issue summary with just a few more details for anyone just glancing at it.

dcrocks’s picture

In the old hook_translated_menu_link() 2 items were passed; $item and $map. Is there any reason to continue passing $map. I didn't find any place that actually used the $map parameter, but perhaps contrib expects it? I want to reroll what will be, hopefully, the final patch.

dcrocks’s picture

I don't think there can be a beta of D8 without fixing this. I've tried to analyze a little more. ConfigStorageController.php, NodeStorageController.php, and DatabaseStorageController.php all invoke hook_(entity_type)_load(in function attachLoad()). But it appears that only DatabaseStorageController.php invokes it on entity_type = menu_link.
Modules Book, Shortcut, and User currently implement hook_menu_link_load. It appears that hook_menu_link_load is not being invoked on the front page, and the arguments passed on other anonymous pages is useless to user_menu_link_load unless it is a user/% page.
Since there have been no issues open in either the book or shortcut module about their use of hook_menu_link_load I think this issue is still only about user_menu_link_load and the only conclusion is that it is ineffective as currently invoked.
If we invoke it from somewhere else, such as _menu_link_translate, then developers will have to deal with possibly 2 different parameter types being passed and have to test the passed arguments to see if they want to use them. I think the best course is to remove user_menu_link_load and re-instate user_translated_menu_link_alter.

I would appreciate some comment but will re-roll the patch in a day or 2 in any case.

dcrocks’s picture

I'm still hung up about why hook_menu_link_load is being invoked on some pages but not others. The only thing I can see is that on those pages where it is invoked(eg. ../user/register) the menu path matches the current path for the page. Thus menu_link_load works some of the time for the secondary menu and translated_menu_link_alter works all of the time. So the latter seems a better solution but that means there will be two post menu_link load hooks being executed in the same code chain. Thoughts?

dcrocks’s picture

This patch replaces user_menu_link_load() with user_translated_menu_link_alter. Though hook_menu_link_load is being used by other modules in core, hook_translated_menu_link_alter best serves the needs of user.module, and may serve others in the future. Someone please test and review. Sorry I filled this issue up with so many comments, but I think this should go in before beta.

dcrocks’s picture

Well that was weird. The 'fail' patch was to show that UserAccountLinksTest was giving a false positive before the patch. Now it is positive after 5 previous fails? The only thing I can think of is that since user_menu_link_load is being called now on some pages the test just shows that user_menu_link_load does work on certain pages. I need to look at this some more.

dcrocks’s picture

Trying the fail patch by itself.

dcrocks’s picture

Status: Needs review » Needs work

The last submitted patch, 1912806_120_Test_secondary_links_on_front_page_fail.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review
FileSize
875 bytes

Another try

dcrocks’s picture

I have to try this.

dcrocks’s picture

The last few posts have been a waste but I still couldn't understand why the test was succeeding now where it had consistently failed before. So I did a minimal install and saw the same result, some pages didn't show the 'User account' menu to anonymous users, and some did. Then I looked at the actual link generated and there appears to be a difference in output after the switch to twig. The rendered output now looks like

<nav role="navigation">
      
      <h2 class="visually-hidden">Secondary menu</h2><ul class="links"><li class="menu-2 odd first last"><a href="/~rocks/dru80909/index.php/user">User account</a></li></ul>
    </nav>

Here is the code that tests for the existence of the link:

    // Log out and get the homepage.
    $this->drupalLogout();
    $this->drupalGet('<front>');

    // For a logged-out user, expect no secondary links.
    $element = $this->xpath('//ul[@id=:menu_id]', array(':menu_id' => 'secondary-menu'));
    $this->assertEqual(count($element), 0, 'No secondary-menu for logged-out users.');
  }

It depends on the 'ul' element having an 'id', but it is no longer produced. I don't know whether to modify the test or change the link that is generated.

dcrocks’s picture

UserAccountLinksTest.php has already been changed for the differences in output after twig, so I generated a patch on these changes as well. There may be other places in that code that may also need to be changed.

Status: Needs review » Needs work

The last submitted patch, 1912806_125_Test_secondary_links_on_front_page_fail.patch, failed testing.

dcrocks’s picture

So here it is again.

Status: Needs review » Needs work
Issue tags: -SprintWeekend2013

The last submitted patch, 1912806_125_Regression_User_Account_menu_on_front_page.patch, failed testing.

dcrocks’s picture

Status: Needs work » Needs review
dcrocks’s picture

dawehner’s picture

+++ b/core/modules/user/user.module
@@ -921,14 +921,33 @@ function user_menu_breadcrumb_alter(&$active_trail, $item) {
+* Alter a menu link after it has been translated and before it is rendered.
+*
+* This hook is invoked from _menu_link_translate() after a menu link has been
+* translated; i.e., after dynamic path argument placeholders (%) have been
+* replaced with actual values, the user access to the link's target page has
+* been checked, and the link has been localized.
+* @see core/includes/menu.inc/function _menu_link_translate()
+*
+* It is only invoked if $item['options']['alter'] has been set to
+* a non-empty value (e.g., TRUE).
+* This flag should be set using hook_menu_link_presave().
+* @see /core/modules/menu_link/menu_link.api.php.
+*
+* Implementations of this hook are able to alter any property of the menu link.
+* For example, this hook may be used to add a page-specific query string to all
+* menu links, or hide a certain link by setting:
+* @code
+*   'hidden' => 1,
+* @endcode
+*
+* @param $link - A menu link.
+*/

I don't think we need all this documentation coming from system.api.php

dcrocks’s picture

That documentation no longer exists in system.api.php. I can shorten it, or re-introduce the documentation in system.api.php. I got the feeling that this hook may be removed once again in the future(d9?).

Delphine Lepers’s picture

I don't think hiding the link from anonymous user is the best approach.
This could be much easily solved by returning t('Log in'); by default instead of t('User account');

function user_menu_title() {
  if ($GLOBALS['user']->isAnonymous()) {
    switch (current_path()) {
      case 'user' :
      case 'user/login' :
        return t('Log in');
      case 'user/register' :
        return t('Create new account');
      case 'user/password' :
        return t('Request new password');
      default :
        return t('User account');
    }
  }
  else {
    return t('My account');
  }
}

If the user menu is not wanted, it can be disabled from the blocks section.

dcrocks’s picture

The point is not to show the menu at all. The 'secondary menu' does not take the same path as the 'User account' menu block and the only way to turn it off is to turn it off on all pages. And wouldn't it be confusing to have both the 'User account' block and a link to the user login on the same page?

dcrocks’s picture

FileSize
64.17 KB

Here is a Bartik home page with your change.

home page

Delphine Lepers’s picture

Delphine Lepers’s picture

Status: Needs review » Reviewed & tested by the community

You are right of course. I have tested and it's working.

marthinal’s picture

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

This is not RTBC at all.

I was talking about this bug with amateescu at DrupalCon.

Please read #19.

I think we don't need the comments so only write a TODO.

And maybe this is a good moment to fix this bug for the moment.

Patch attached.

So again please read #19. For the moment this is the solution.

marthinal’s picture

Adding more detailed todo.

dcrocks’s picture

This is #127 without the comments and the verificatin of the bug in UserAccountLinksTests.php. It should come back green. Is there someone ready to commit this? It has been a long time.

dcrocks’s picture

Status: Needs review » Reviewed & tested by the community

#139 applied cleanly and does the job.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.php
    @@ -28,7 +28,14 @@ public static function getInfo() {
    -
    +  ¶
    +  ¶
    ...
    +  ¶
    
    @@ -61,7 +68,9 @@ function testSecondaryMenu() {
    +        ));    ¶
    
    +++ b/core/modules/user/user.module
    @@ -895,14 +895,15 @@ function user_menu_breadcrumb_alter(&$active_trail, $item) {
    + * ¶
    ...
    +  }  ¶
    

    Trailing whitespace

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.php
    @@ -28,7 +28,14 @@ public static function getInfo() {
    +  function setUp() {
    

    public function setUp()

  3. +++ b/core/modules/user/user.module
    @@ -895,14 +895,15 @@ function user_menu_breadcrumb_alter(&$active_trail, $item) {
      * Implements hook_menu_link_load().
    ...
    +function user_translated_menu_link_alter(&$link) {
    

    This is a completely undocumented hook... Also the docblock is off.

marthinal’s picture

Status: Needs work » Needs review
FileSize
2.08 KB

@tim.plunkett This is a D7 hook... what do we need to do in this case?

catch’s picture

I think we need to add back the documentation for that hook to 8.x, and then ensure there's an issue to track it somewhere since we're supposed to not need it any more. I'm OK with using the hook here if it's actually needed..

tim.plunkett’s picture

Status: Needs review » Needs work

Here is the old doc, this belongs in modules/system/system.api.php

The only problem: It is still behind a if (!empty($item['options']['alter'])) { check, how is that even working in this patch?

/**
 * Alter a menu link after it has been translated and before it is rendered.
 *
 * This hook is invoked from _menu_link_translate() after a menu link has been
 * translated; i.e., after dynamic path argument placeholders (%) have been
 * replaced with actual values, the user access to the link's target page has
 * been checked, and the link has been localized. It is only invoked if
 * $item['options']['alter'] has been set to a non-empty value (e.g., TRUE).
 *
 * Implementations of this hook are able to alter any property of the menu link.
 * For example, this hook may be used to add a page-specific query string to all
 * menu links, or hide a certain link by setting:
 * @code
 *   'hidden' => 1,
 * @endcode
 *
 * @param array $item
 *   Associative array defining a menu link after _menu_link_translate()
 * @param array $map
 *   Associative array containing the menu $map (path parts and/or objects).
 */
function hook_translated_menu_link_alter(array &$item, array $map) {
  if ($item['href'] == 'devel/cache/clear') {
    $item['localized_options']['query'] = drupal_get_destination();
  }
}
dcrocks’s picture

The 'alter' flag is set in user_menu_link_presave():

  // The path 'user' must be accessible for anonymous users, but only visible
  // for authenticated users. Authenticated users should see "My account", but
  // anonymous users should not see it at all. Therefore, invoke
  // user_menu_link_load() to conditionally hide the link.
  if ($menu_link->link_path == 'user' && $menu_link->module == 'system') {
    $menu_link->options['alter'] = TRUE;
  }

I admit I find those comments a little confusing. I had actually included the hook documentation way back in #45 but had removed it after it was recommended I do so, though I can't remember where or why that was now. I placed the hook documentation in menu.link.api.php rather than the D7 location of system.api.php because it seemed best to keep the menu link docs together.

User_menu_link_load is being called currently, but not at the right time to prevent the regression. It seemed better to fully replace the implementation of user_menu_link_load, for now, with user_translated_menu_link_alter() just to prevent the unnecessary execution of the code.

dcrocks’s picture

Status: Needs work » Needs review
FileSize
5.16 KB

Here is a patch with documentation. Please review.

dcrocks’s picture

Status: Needs review » Postponed

I think this should be postponed on #1842858: [PP-1] Convert menu links to the new Entity Field API. Things could change significantly when that is committed and this is considered to be a temporary fix in any case. UserAccountLinksTests.php still has a bug, but if this patch becomes unnecessary that can be a new issue.

Berdir’s picture

Status: Postponed » Needs review

That issue doesn't change anything about how menu links work. It just changes their structure, it should not have any effect as to which hooks are called.

dcrocks’s picture

It looked like it might change when they are called, which is the core of this issue. But I admit not understanding enough to argue against the status change.

dcrocks’s picture

Issue summary: View changes

Updated as best I can per @xjm in #47 https://drupal.org/node/1912806#comment-7187018 using comments.
There are a lot of comments on this one, so while concise is good, it could use a small blurb on which hook is which and why each does or doesn't work.
-@ekl1773

penyaskito’s picture

Issue summary: View changes
Status: Needs review » Needs work
+++ b/core/modules/user/user.module
@@ -894,14 +894,14 @@ function user_menu_breadcrumb_alter(&$active_trail, $item) {
+  if ($link['link_path'] == 'user' && $link['module'] == 'system' && !$GLOBALS['user']->id()) {

This shouldn't use $GLOBALS['user'] but \Drupal::currentUser() instead.

See https://drupal.org/node/2032447

amateescu’s picture

Title: Regression? 'User Account' menu on front page » Regression: 'User Account' displayed on front page for anonymous users
Component: menu system » menu_link.module
Status: Needs work » Reviewed & tested by the community
FileSize
3.72 KB

Cleaned up the patch a little, this is just about bringing back hook_translated_menu_link_alter() (because it shouldn't have been removed in #916388: Convert menu links into entities) and adding test coverage, so I think it's good to go.

alexpott’s picture

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.php
@@ -61,7 +67,7 @@ function testSecondaryMenu() {
-    $element = $this->xpath('//ul[@id=:menu_id]', array(':menu_id' => 'secondary-menu'));
+    $element = $this->xpath('//ul/li/a[text()=:text]', array(':text' => 'User account'));
     $this->assertEqual(count($element), 0, 'No secondary-menu for logged-out users.');

Asserting for something not being there makes this very very likely to break again in the future - wouldn't it be better to get the full menu and assert the link exists but the hidden property is set to true?

catch’s picture

Status: Reviewed & tested by the community » Needs work

Agreed.

amateescu’s picture

Status: Needs work » Needs review
FileSize
927 bytes
927 bytes
3.91 KB

Agreed as well :)

Status: Needs review » Needs work

The last submitted patch, 155: 1912806-155-test-only.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

Patches failed/passed as expected.

dcrocks’s picture

Did both a minimal and standard install with a D8 clone plus patch. Everything looks fine.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Code looks fine for me, @alexpott & @catch concerns were attended and @dcrocks tested it, so back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

dcrocks’s picture

Does this need a follow-up? Is it expected to be a long term fix?

amateescu’s picture

This should be the place where we write a ViewBuilder for menu links: #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.