Move editing UI as-is from Navigation to dedicated new Navigation Layout module to remove runtime Layout Builder dependency

Adding some related issues from the XB issue queue.

I've created a MR that removes the dependency on layout_builder while keeping the same config format for navigation.block_layout. The dependency is removed by simply calling the Layout and Block plugin managers directly.

The UI is still to be replaced. I only emptied navigation LayoutForm and added a new route "navigation.layout" to replace "layout_builder.navigation.view".

This currently moves down the dependency to layout_discovery. Is that a module that stays around in core? (There are a lot more criteria of course, but it is also not depended on by Experience Builder or UI Suite that I could find at least).

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.

@gábor hojtsy layout_discovery provides the layout plugin manager service definition.

#3483307: New `ComponentSource`: `Layout`, to allow for a Layout Builder-to-Canvas migration doesn't depend on the layout_discovery service but it does depend on the same layout plugin manager class that the service uses (which is in Drupal\Core).

So I think that even if we eventually deprecate the layout_discovery module, the layout plugin manager itself will need to exist for quite a long time to support the just in time layout builder -> xb upgrade path. That could mean that navigation module ends up adding it's own version of the layout plugin manager or something like that - but it should be a relatively small incremental step to remove that dependency if/when we get to that point.

Rebasing this and trying to move this forward.

The MR currently removes the layout builder integration completely. This seems like a pretty hard change to land as we'd then for now would not have a UI at all.

How about we split this so that we make the layout_builder integration optional just for the UI? My goal for moving this forward is to avoid having to depend on layout_builder at runtime. Because layout_builder comes with significant performance implications right now and has its hooks deep in all kinds of places.

The UI page would then just display a message to enable layout_builder to customize it.

I reverted quite a bit of changes. Navigation works without layout builder being enabled. The backend shows a message to enable layout builder if you don't have it enabled. To make it work, I essentially had to do two non-trivial/annoying things:

* Copy the code of the two traits from layout builder, because missing traits is a parse error and those form classes are loaded during route building. We could possibly maintain two different form classes and dynamically register them, but even the existence of a form class is going to be trouble with route attribute discovery and so on.
* Hardcode the route definition including all the magic bits that layout_builder adds to make things work.

Additionally to the existing duplications like the schema definition. I think that's acceptable as an intermediate step for #3521155: Remove Layout Builder dependency for the UI of Navigation admin config, still use Layout Builder for backend and rendering.

Still struggling with some test fails that I can't reproduce or fail differently for me.

Thank you for stepping into this.

Checked the code and looks really promising. Regarding how to deal with the configuration page, maybe a temporary navigation_ui module that enables the access to that page might simplify the things a bit?

maybe a temporary navigation_ui module that enables the access to that page might simplify the things a bit?

Do you mean like a feature flag module? Except in this case the module would have the layout builder dependency?

I think that might be a good idea.

Since we're changing navigation config schema here, does it make sense to address or keep in mind changes that we'll need for #3587499: Navigation stores plugin configuration with dependencies in a simple config object?

Sounds like in #3521155: Remove Layout Builder dependency for the UI of Navigation admin config, still use Layout Builder for backend and rendering, the thinking might be to move the UI to a separate module navigation_ui that eventually moves to contrib, so it probably makes sense to land this close to as is now and then do a follow up to move relevant code including tests to the new module.

I like this too! We need to define if the customization UI is at all 80% use case assuming contrib can still provide that :)

I agree that the customisation UI does not need to be in core. Drupal CMS does modify the layout but this is in a recipe so the UI is never needed. There will be sites that want to modify it, but certainly not very often, and it could be done locally for example so you don't need all of the dependencies in production ever.

Merged main and resolved conflicts (hopefully correctly). @berdir mentioned some outstanding test failures in #12, and there are test failures now. I have not checked if they are the same, but I might look into the test failures later.

Looked a bit at the tests, the UI really is broken right now and doesn't load. Instead of trying to fix that, if we agree that we want to move forward with a navigation_ui module, then I'll refactor that and go back to using the layout builder traits there, that will hopefully fix that.

@berdir: let us know when the MR is again ready for review or if you need helping hands :)

This is waiting on a decision about having a navigation_ui module. See #21. Changing to needs review for that discussion/decision.

And I did a rebase.

I think moving the Layout Builder related code out to its own UI module is the best choice. The UI is very awkward anyway (last I tried a couple months back and nothing changed in the meantime) and Layout Builder is on its way out of core.

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.

I used some LLM assistance to rebase and start resolving some new fails. The moved code sections for shortcuts especially needed to be taken care of.

Now it all passes green again, so I'm looking at decoupling the LB integration to its own module :)

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

Update title and issue summary.

MR is green.

I think this is done-done unless we want to install the Navigation UI automatically on a site update (which I think is debatable). The release notes snippet explains you can even uninstall Layout Builder if you only used for this. (I don't think we have a way to automatically detect that situation though).

Please try and review. :)

Added screenshots from my local setup.

Adding this priceless quote from @berdir to the issue summary:

layout builder is IMHO too heavy and has too many performance implications just to be able to press that button in the backend

"Navigation UI" is a confusing name to me, because the primary purpose of the Navigation module is already to provide a UI. Even the issue summary says "Navigation is the modern admin navigation UI", using the same name in a different context.

Is "Navigation Layout Builder" a better name, seeing as it combines those two things?

Agree re: naming, but "Navigation Layout Builder" also feels misleading because you were never really building a layout, just placing and reordering menu blocks (you can't choose columns etc which to me is what building a layout really means).

What about just "Navigation Layout" (consistent with Block layout) or "Navigation Manager"?

I agree navigation UI was very confusing but I did not have better ideas. Navigation Layout is simple and very good IMHO :) Did that.

Renaming issue following module rename. Also making it clear that this is the existing code split, its not new code :)

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

The IS says:

Layout Builder is on its way to be removed from Drupal core

Is this true? Where is the issue that made that decision?

Rebased with a lot less changes since shortcut has been removed.

Used our new fancy performance test update environment variable to regenerate the metrics too :)

@kim.pepper: I did not find where we documented that, so opened #3619180: [policy, no patch] Move Layout Builder module to contrib now. However that was at the same discussion at DrupalCon Vienna where we discussed also removing the block UI, see #3555047: Add a way to ship block placement statically (will be useful when block UI is removed). Basically the idea is that Drupal core would not have a UI to structure pages and you would pick whichever you wanted, be if layout builder or canvas or paragraphs or display builder. Eg in #3521155: Remove Layout Builder dependency for the UI of Navigation admin config, still use Layout Builder for backend and rendering @catch writes It seems likely that this won't be the last blocker to deprecating layout builder.

We also need an issue about manage display which is the third layout builder in core on the side of Layout Builder and blocks, but I don't 100% remember what was the direction discussed there, maybe @catch does?

Just tweaking the title

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.

✅ Tested and works — MR !11972. A do fresh Standard install no longer enables Layout Builder (only Layout Discovery), and the navigation sidebar and top bar render fine without it. With Navigation Layout uninstalled the "Navigation blocks" page is gone and its route 404s; installing Navigation Layout brings Layout Builder back in as a dependency and restores that page, where placing a menu block and saving it showed up in the sidebar right away. Navigation Layout can then be uninstalled on its own without taking Navigation with it, and no errors were logged in any of the three states. Based on this I would suggest RTBC.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

Make title clearer as people were taken by surprise that its proposing to add a new layout tool. Its just moving code around.

Updating issue summary for current module name. Also mention upgrade path and permissions.

All concerns on the issue are resolved so should be ready for review again :)

Even more clearer title. Sorry for ping-ponging on this.

Added extensive performance measurements. Tagging for that too, since that is a key motivation for this issue.

Formatting.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

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.

@gábor hojtsy I tested again and it looks good and same as on #46 previous state. Based on this I would suggest 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.

Great initiative!
I'll rebase and test.

I tested various layout change scenarios—everything works as expected!
+1 for RTBC - frontend is good! Thank you!

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.

Re-confirming RTBC!

There's a merge conflict in the MR, so back to NW.

I rebased the MR on main.

Thanks, well done! Back to RTBC!