Investigate using the core "User account menu" in favor of custom Navigation Block for same

Navigation project goals include: ✨ Add the user image to the user menu when available Active

How User menu is made: User menu is provided by a Menu Plugin (YAML FORMAT)
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...

user login/logout item:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...
items itself dynamically changes title & path. cache content 'user.roles:authenticated'

----
Perhaps it's simply that using the same namespace 'user' should be renamed to keep more separation for when a site may want to place the traditional list of Account Items?

#[NavigationBlock(
  id: 'user',
  admin_label: new TranslatableMarkup('User'),
)]

If this needs to be different to the user account menu that would normally be in the front end theme (brought up by @ckrina on the MR), then it might also be possible to define an additional menu like 'Navigation user links' so that they're separate - but that would still allow people to add an extra link or remove something if they want to.

Agreed. I missed the part about customizing the links. Tagging this as Stable Blocker.

Having the "User account menu" and the user account links that appear in the toolbar being different does feel like a weird quirk, from a UX perspective I think using the "User account menu" and "modernizing" it makes a lot of sense. As noted, it also allows the user menu links in the toolbar to be customized, which is a win in my opinion. I think it would be weird if every other menu in the Navigation could be customized apart from the user links.

There are sites that customize the user account menu links with links that they might not want in the navigation toolbar, however, if it's just a menu block in the layout, those sites could swap it out with a different menu block easily enough. So using the same block by default might be plenty?

however, if it's just a menu block in the layout, those sites could swap it out with a different menu block easily enough. So using the same block by default might be plenty?

Agreed. We should think in terms of what's the most sensible default for the 89% use-case. As you say, they can always use a different menu on the front-end, or swap this one out for a different one in the navigation.

Adding 📌 Agree on a lazy load strategy Active as reference to deal with caching at user level issues.

My concern or question about how to integrate a menu here is because most of the links I think that could be added to this menu are user specific, like /user/XX/foo and I am not sure how we could handle routes like that from a menu.

Mostly thinking about menu links created from the UI by site admins, where it is not possible to indicate URL parameters.

Core's user routes provides shortcuts like /user or /user/edit that redirects to the user specific page, but that's something not all the contrib modules that provide user specific routes or views that use the user ID as parameter would support.

We could encourage the usage of modules like https://www.drupal.org/project/user_current_paths → or https://www.drupal.org/project/me_redirect → for this menu, though.

Now that 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review is closer and we are pushing again towards Navigation stability, I would like to make a proposal for this one:

• Navigation would provide a Menu Navigation User Links, or a better name to be proposed
• That Menu would include by default the same links we currently have hardcoded.
• NavigationUserBlock class will give the ability to choose a menu to render as user links. By default we would use the Menu provided above
• NavigationUserBlock would render by default the links from the menu stored in config. If empty, or the Menu does not exists, it would fallback to the hardcoded links we have now. In this way we will ensure that removing the menu will not lead to unexpected behaviors

Would this proposal cover all the previously mentioned needs?

#14 looks good, with the slight caveat that I'm not sure about the fallback - could that be handled with validation maybe?

Validation could be a good approach, and necessary.

My concern about validation-only approach is that selected menu could be removed from the system by accident, or deliberately, breaking the navigation. In that case, we might need to provide something.

If the concern is about cache, we could make it in a way where the block would not need to be cached per user, given that links in the fallback would be /user, /user/edit & /user/logout, which are not user-specific.

Once 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review has been merged, started to work on this one.

Here is a brief summary of the approach taken to be validated.

• Navigation provides a new menu navigation-user-links
• This menu is locked, so it should not be deleted unless unlikely things happen
• Navigation provides default links for the menu mentioned above (Same links we have now hardcoded)
• NavigationUserBlock renders the menu, so new links could be added
• To customize the Navigation, rendering the Username, JS is used to avoid caching issues. Small flickering that might need to be addressed.
• If the Menu is removed for any reason, the navigation item fallbacks to a button that links to the user profile page

Please take a look and validate the approach before continue polishing and updating tests.

Couple of small comments on the MR. More general thoughts here:

- I think an extra menu is a good idea, allows it to be customized separately from any visitor-facing user menu.

- the js looks fine to me (which doesn't mean much because I am bad at js), although I wonder if there's a way to do it using core's existing lazy builder/big pipe workflow without additional js, but... I've never tried to do that with a menu link. And also we could look at that in a follow-up since it wouldn't be a data model or API change at all, whereas the new menu itself feels very stable blocking.

The very minimal fallback added here seems fine.

Tests are now green. We might need an extra FunctionalJavascript or Nightwatch test to validate the user name is replaced by JS successfully, but I would prefer to have the approval of the JS approach first.

One thing to check here - does the current implementation result in the render cache for this block caching per-route. If so, it would be good if we're able to avoid that. The main reasons for per-route caching are the active trail for expanded menu links and maybe the 'active' class.

js not works
https://gyazo.com/f93eb21f5e7ddba4797e2cc9292d056f

i fixed js but seems we need to care about reload again
https://gyazo.com/e1a88c9d32cbd215f97a11b6198d92b6

also keyboard search broken because button still has data-index-text="u". not sure it is critical. maybe we also need to move that functional out of twig to js

Thank you for your review @catch.

Made a couple of small changes from your comments and answered to one of you MR questions.

One thing to check here - does the current implementation result in the render cache for this block caching per-route.

Checked at DB level and we are not introducing new items in the cache_render table related to this block.

This is looking great. Just noticed two really tiny things in the menu wording. I don't see any other issues left to deal with.

The JS looks OK to me.

Noticing that we're still set to Needs work here. Is there anything else left to do here @plopsec that I'm not seeing?

Thank you for your review @m4olivei

It was still in NW because @finnsky noticed some possible issues with the markup and JS. Also, when the navigation is expanded a small glitch is perceived when the username replaces the "admin" label. I would like to know if there is any possibility to improve that behavior.

Adding the "Drupal CMS release target" to see if we could hopefully get to it.

This is ready for a re-revew. Summary of changes:

• Merged latest 11.x
• Fixed the issues noted by @finnsky around keyboard search. We now update data-index-text to match the dynamic usename
• Changed the default text of "User" to "My Account" to better fit other places this kind of thing is needed. Updated test to match
• Updated \Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest. We're doing less isValid checks? I think this is a good thing.

I also considered @catch's comment from earlier. For the reasons he mentions, but also as a way to address the small flicker @plopesc noted.

the js looks fine to me (which doesn't mean much because I am bad at js), although I wonder if there's a way to do it using core's existing lazy builder/big pipe workflow without additional js, but... I've never tried to do that with a menu link. And also we could look at that in a follow-up since it wouldn't be a data model or API change at all, whereas the new menu itself feels very stable blocking.

The trouble is that, as far as I can tell, the title of a menu link needs to be a plain string. I tried to create a new Menu plugin (\Drupal\navigation\Plugin\Menu\MyAccountMenuLink) for use as the top level default menu link. My idea was to override \Drupal\Core\Menu\MenuLinkInterface::getTitle and return a render array like:

public function getTitle() {
  return [
    '#lazy_builder' => [
      'user.toolbar_link_builder:renderDisplayName',
      [],
    ],
    '#create_placeholder' => TRUE,
    '#lazy_builder_preview' => [
      // Add a line of whitespace to the placeholder to ensure the icon is
      // positioned in the same place it will be when the lazy loaded content
      // appears.
      '#markup' => ' ',
    ],
  ];
}

This borrows from this approach in user module. The nice thing about this if it worked, would be that BigPipe would take care of intelligently replacing the username without our own custom Javascript, and handling the no JS case well too.

However, it white screens the site. Per the docs on the interface, that method should return a string.

Ultimately I don' t think the flicker is that bad, so at the most, I'd suggest we handle that in a follow up.

Also, I realize the issue description doesn't accurately describe the latest developments on this issue. Here is a crack at revising it.

Added a functional javascript test.

@m4olivei yeah menu link placeholder is tricky. I opened 📌 Use a placeholder for the username in the navigation user menu Active to track the possibility, definitely shouldn't try to tackle it here (although thanks for trying, but I think that's enough to demonstrate that it's hard).

• Matt made a great job addressing the last feedback suggestions.
• New FunctionalJavascript test has been added to ensure name replacement is working
• MR is up to date with latest 11.x
• A followup was created to try to address the menu link placeholder trickyness.

I think we're in a good position to mark it as RTBC.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Back to RTBC once conflicts have been solved.

Merge conflicts on the MR unfortunately.

It's green again.

Sorry it took me this long to realise this - because navigation is beta stability, we need a post update that creates the system.menu.navigation-user-links menu. Because it's beta, this doesn't need any test coverage, just the upgrade path and probably some manual testing would be good.

I was just thinking about how great it was that the MR doesn't need an upgrade path, then it hit me that it does :/

Upgrade path added and moved back to RTBC.

Pretty sure the update should be a post update, not a hook_update_N(), because it relies on the entity API. Also was it manually tested?

Upgrade path is now a post_update hook. Marking as Needs Review to get a valid manual review from the community.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

MR updated. Back to Needs Review.

Review steps

1. Install Drupal 11.x
2. Enable Navigation module
3. Run drush updb

See evidences:

Update database

New user navigation link

Everything works as expected ✅

Moving to RTBC.

Updating issue credit, unfortunately this needs another rebase.

Committed/pushed to 11.x, thanks!

catch → closed merge request !9904

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