Show the Navigation Top Bar in 11.1.x and 10.4.x

If the top bar itself is ready to go, should we not remove the feature module and always enable it when navigation is enabled? It's only a feature toggle anyway.

Yep, we should just get rid of the module.

Thanks both, doing all in one issue will make it easier. I've updated the title&issue summary.

@plopesc just mentioned that it'd be great to get ✨ Display content moderation state in the Navigation Top Bar Active merged in first. Not a blocker, but it's make things easier.

Would be great to have this enabled by default just removing the flag module.

Just a couple of questions.

For sites where the flag module is already enabled, how should we proceed? Just removing the module would cause an error. On the other hand, this was an alpha module within an experimental one, so people who enabled it should be able to manage this scenario.

Navigation is used in Drupal CMS, where Gin Toolbar is enabled as well. Removing the flag would automatically enable both top bars for sites using the 1.x branch, I think.

These 2 questions, and maybe others I missed, make me think that we might need a 2 steps path.

I think there's two parts:

1. Ignore the feature module in navigation - should be removing a condition or two.

2. We can mark the feature module itself obsolete https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsolete →

For #2, need to add a post update that self-uninstalls the module, and a hook_requirements() that prevents it being installed. Then we can eventually delete the module when all possible sites have had to update to the version that made it obsolete (probably in 12.x). I don't know if there are any currently obsolete modules in core, probably not, but there should be in either 10.x or 9.5.x branches to compare against.

I think it's fine to do the two parts either in this issue on in two issues - since the module is hidden there's no user-facing confusion to worry about, we just want to enable it for everyone and clean it up safely for any sites that enabled it already.

Created MR that removes the checks for the module flag and marks it as obsolete.

Added a couple of questions to the MR about the bureaucracy of the process of marking the module as obsolete.

Postponing on 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active since some accessibility issues were found. See https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431574#L2723

Besides that, there is another Kernel test failing reporting something related to navigation_top bar module tests. Those tests were removed as part of this MR. Tried to reproduce locally and the test is passing. See https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431573#L659
Any idea about how this test could be addressed?

self uninstall can be a bit of a problem, at least for regular numbered update functions but I think for post updates too.

The problem is that Drupal stores information about executed updates in key value storage. But if a module uninstalls itself, it results in a situation where drupal then stores that for a module that is no longer installed and won't be cleaned up. For regular updates, it may then result in warnings, post updates less so, but might still end up with a little bit of cruft in the database. I'd consider moving it to navigation module just to be extra sure.

Thank you for your feedback @berdir.

Moved the post_update hook to the parent module to reduce possible frictions.

I did a static review and adjusted some tiny things. The main change was to remove the reference to navigation_top_bar in phpunit.xml.dist, which I believe was causing the test failures. We'll see.

The issue that we're postponed on, 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active has been resolved. Marking this as needs work. Will merge the latest 11.x into the branch and see where we're at.

One nitpick on the description, then this looks good to me.

Discussed with @plopesc and @ckrina today. The Top Bar is a Stable blocker. This issue will be needed to mark Navigation as stable. Adding Navigation stable blocker tag.

This had a test fail on some performance metrics, those have been removed in HEAD, needs a rebase.

Branch rebased and MR is green. Back to NR.

Changes look good to me.

Since this was extra-experimental and hidden, not sure if this needs a change record?

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 rebased. Back to RTBC

Committed/pushed to 11.x, thanks!

catch → closed merge request !11256