Allow modules to hook into top of content section of new core navigation

Thanks for your work! It's great having your work on env indicator to get a look at the real world pain points in integration.

Adding a related issue around Navigation's extensibility. I'm not sure ATM if we want to merge the issues or keep both.

Not sure if the aim of this issue is exactly the same, but discussion about Config Actions for Navigation has been opened in #3478224: Provide Config Action to add new blocks to navigation from recipes.

This issue has been discussed internally and for now it seems that we are going to rely on Layout Builder's capabilities to manage this.

#3478224: Provide Config Action to add new blocks to navigation from recipes Will give the possibility to modify the config using recipes, as this is the way to extend layout builder config is expected to use.

3rd party integrations without recipes might need the manual step of adding blocks to the navigation bar once the modules are installed.

3rd party integrations without recipes might need the manual step of adding blocks to the navigation bar once the modules are installed.

If "manual" here means "the user has to do it", isn't that quite a regression from what was previously possible with hook_toolbar() and hook_toolbar_alter()?

A bit more context for #6: in #3425081: Integrate Navigation with Workspaces I proposed a new hook_navigation_alter() hook that would provide a similar DX to the Toolbar module.

Was that option considered?

If "manual" here means "the user has to do it", isn't that quite a regression from what was previously possible with hook_toolbar() and hook_toolbar_alter()?

Yes, I agree.

Difference is that new navigation allows to modify the order of the sections through the UI, and it adds an extra layer of complexity that makes the automatic integration of new elements slightly more complex.

This issue was postponed because we currently have other priorities in the roadmap to convert Navigation into a stable module and integrate it with Drupal CMS. #3421969: [PLAN] New Navigation and Top Bar to replace Toolbar Roadmap: Path to Stable

A proposal for having 2 "toolbar" like sections for the navigation top and footer could be a possible approach. That would leave only the "content" region managed through LB.

Or provide a simpler API for adding Navigation blocks to the LB during module installation, that could be exported as config afterwards.

Moving back to active since other issues like #3425081: Integrate Navigation with Workspaces or #3441100: Integrate Umami message

Implemented an initial approach to create a new "promoted" region in the Navigation bar that allow modules to place their content programmatically.

My initial feeling was to name it "top", but it could cause confusion with the top bar, so ended up using the "promoted" name for the section and the related hooks. I'm totally open to better naming suggestions.

Regarding the footer, given that's a region that already exists, this change might need more internal discussion. I'm more inclined to leave it out of the scope of this MR for now.

Suggesting a different name of 'header' to go with existing 'content' and 'footer'. Let me know what you think.

Also, do we want to address the region just above the footer, which is asked for in the issue description?

Otherwise, I think this is a fine way to cover this requirement. It offers a lot of flexibility to module authors to put whatever render array they want there. To @finnsky's point he's been arguing for in various places, we'll have to flesh out our components more in order for module authors to author things that fit well in these spots.

@m4olivei Property renamed. Could you please take a look into this again?

Static tests are failing due to an unrelated issue in 11.x.

Looking good to me!

The MR doesn't address footer_top? I'm wondering if there was a specific reason why not that I'm missing. We either need to add that in as well, or update the issue description to reflect only content_top being covered here. Marking as Needs Work for this reason, but not wanting to discourage integrations from leaving thoughts as well, so please do feel free to leave review comments.

Thank you for you review @m4olivei.

Footer is a more complex region than content and I would rather to discuss it in more detail in a follow-up issue.

Merging this one would unblock other integrations like Dashboard, Environment Indicator, Workspaces or Umami.

If you are OK with that, i think the only remaining step here would be to write the corresponding Change Record.

Thanks @plopesc! That makes sense. Thanks for the updates.

I've written a draft change record.

Also, updating the description to remove one more reference to footer_top as well as updating the screenshot to reflect the actual implementation.

As we only bumped back to needs work on docs related issues, marking this all the way to RTBC!

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

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

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.

This looks good to me, just a couple of comments about the test - one about avoiding random failures we've only just found in the last week or so - and another about refining how we clear the cache

Will keep an eye out for the re-RTBC but feel free to ping me

Thanks for the review. I've addressed the feedback. Back to Needs review.

Will play a bit and work out a solution to my comment.

Not sure if it matters, but it was found that environment indicator module does not need this patch in order to integrate into the navigation module.
See https://git.drupalcode.org/project/environment_indicator/-/merge_request...

@trackleft2 correct!

However, the sub-module version of that issue hasn't received broad consensus per a read through of the issue. Also, both MRs require that the admin configure an additional Navigation Block via the UI. This patch will mean that's not necessary, it can be included via a hook and also be included in a consistent spot for all sites.

I ended up not liking the empty div. Instead, what I did was introduce a new #theme hook for content top to carry the cacheability metadata of empty items if present. This way we collect and render all necessary cacheability metadata, respect the API we set, and don't end up with empty divs.

I think this checks all the boxes, but I'm very open to suggestions.

Setting to NR in any case. Thanks!

I like the new approach, also, I've updated the change record to include OOP/attribute example for the hooks.

Matt did a great job addressing the outstanding comments, so moving it back to RTBC.

Issue credits

Committed to 11.x and backported to 11.1.x because we want to get Navigation to stable.

Setting to 10.5.x for further backport.

Created new MR against 10.5.x

Note that the commits from #39 were not pushed.

Seems like a good re-roll for 10.5

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.

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. MR is still mergeable against 10.5.x.

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.

Turning off the bot.

Committed d93c93c and pushed to 10.5.x. Thanks!