[meta] Menu Links - New Plan for the Homestretch

Should #2227179: Step 2: Move the menu tree storage to a separate service. be closed as a duplicate of this?

Also, let's continue to treat this as beta-blocking. It is critical and includes significant data model changes that would affect almost all modules.

Phase 1 is currently being developed in a sandbox:
http://drupalcode.org/sandbox/dereine/2031809.git/tree/refs/heads/2227441

git clone --branch 2227441 http://git.drupal.org/sandbox/dereine/2031809.git menutree

Should #2227179: Step 2: Move the menu tree storage to a separate service. be closed as a duplicate of this?

I closed it as a dupe of this issue's phase 1 child issue: #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins

It is critical and includes significant data model changes that would affect almost all modules.

I agree with the "significant data model change" part, but not the "affect almost all modules" part. It will only affect modules that interact with MenuLink entities (or what they turn into via this issue) which I think is a very small minority of contrib modules, though possibly not so small a minority of contrib themes.

Also, let's continue to treat this as beta-blocking.

Well, the question is whether there's sufficient consensus on this plan to consider it the primary approach being pursued in terms of issue queue tagging. If there is, then we should also close #1842858: [PP-1] Convert menu links to the new Entity Field API as a duplicate and tag the 3 child issues of this issue as beta blocking.

Link title (translated) always taken from entity label

Does that mean that if I have a page, and its entity label is "Contact Software Solutions", I couldnt have "Contact Us" on my custom menu?

Thanks for such a thorough issue summary.

For multilingual's concern, the most important is we don't have glaring regressions like the admin menu not localized. The content based menu plan for that was that (a) all menu items are content entities (b) they are translated with content translation => so therefore we were in need of a content translation system that can ship with default content translations and update them from the community (based on software translation updates). While we already have such a custom integration built for configuration (for shipped configuration), we don't have that integration for content. Unfortunately neither menu items as entity NG is close to done, and there is still no content shipping solution that I know of.

ATM menus are still perfectly translated with the runtime t() translation system, see this screenshot I just shoot with D8 head installed in Hungarian:

However currently menus are neither content (entity NG) nor config entities, and remain their own subsystem. The definition of shipped menu items is also pretty simple and there are no regressions from the Drupal 7 menu features in current HEAD, are there? The criticality of these tasks is therefore defined by where do we want to get menus to in Drupal 8 in terms of capability and (internal) developer API. All proposals want us to get to more capability and closer to developer API of other systems in Drupal 8 instead of retaining the D7-like API.

Please correct me if I'm wrong.

In short:

- Regression: D7 translates menu items if they have the same title/description than the shipped one; D8 OTOH translates menu item only if it is not modified at all (even other properties)
- Although this may be solvable with a local fix/hack, it is argued that D8 menu system is already more hacked together than D7
- Instead the proposal is to take time now to rearchitect it so it makes sense in the D8 context and uses less hacks

If all else fails, we could still solve this with a small local fix to compare titles and get back the exact D7 feature but not an ideal D8-like menu API.

Correct me if I'm wrong but, I just added a menu link in my D8 local and then translated it and it worked fine. How is this a beta blocker? Shouldn't we be focusing on releasing this long-overdue version?

If, as Gabor says, we can fix this with a "small local fix" I would say we should just do that and continue this branch for a . There are many other criticals to resolve and this would appear to increase rather than reduce that number.

Wait, if you added a menu item **on the UI** and translated that ... I don't think that should be possible in any way now?! How did you do that?

Sidenote: postponed #2119551: Add string context support to menu system on this one although added title context support to extraction in #2254561: Support string extraction for Drupal 8 menu_links.yml with the expectation of it using the same syntax as all other menu-like YAMLs. But implementing it on the backend *now* would be kind of pointless due to this process.

The issue summary does a good job of explaining what we need to accomplish before release, but I really struggle to see how this arrives at the conclusion of "let's use plugins".

The entire "Some people will ask "Why plugins?"." section does little to convince me, it just seems to say "because we can".

At NYCCamp @pwolanin and I had a long discussion about what the benefit of using plugins would be, and there was no real consensus, just a promise of "some proof of concept code".

Now instead of some proof of concept code to evaluate, there is a 400K patch with a diffstat of +5499,-4821, a complete removal of the Menu UI, dozens of arbitrary changes, and still zero explanation of how this will improve DX for developers or be neutral or an improvement for site builders.

In fact, the plan of removing the UI now and adding it back later piecemeal really frightens me. What if we merge in this 400K patch and the rest never happens? How will we feel if all other beta/release blockers are finished except we're without a Menu UI? Now *that* is a regression.

Views was a pre-existing module with 4 people working on it, and plenty of reviews and people helping after the merge, which was now 18 months ago.
Around the same time the 400K blocks patch went in, which burnt out a lot of core devs, and needed 2 follow-up rewrites. That also was more than 14 months ago.

Now in this final push to beta, we're introducing at least 400K in the first of 3 phases, written by two people, and against the desires of one of the current menu_link maintainers.

If anyone could make this all happen, it's @dawehner. But I'm worried all the same.

I really struggle to see how this arrives at the conclusion of "let's use plugins".

For me, the two main reasons are:
- We want module-defined-in-YAML links (primarily for admin menus) to not get copied to a content entity, but instead, have a config-based (i.e., deployable) storage of overrides. But, we want site-builder-created custom menu links (primarily for website-visitor-facing menus) to be stored as content entities, in order to be fieldable. So we have a single concept, "menu links", that requires for any given site, some instances to be of one implementation and some of another. That's the problem-space for which we use plugins almost everywhere else in core.
- We already use plugins for local tasks, local actions, and contextual links. So doing so for menu links adds yet another level of consistency across our various link managers.

I agree with the other concerns listed in #20. While I think the architecture is a good choice, I'm worried about how late in the D8 cycle it's being introduced. I'm not yet clear on any sane alternative though.

That is not an answer. That does not make it better. In fact, it makes it worse, since everything is piling up without anyone else having a chance to review this.

I tried to discuss this with @pwolanin in IRC, to no avail. Basically, this issue is proceeding as though it has sweeping consensus, when there is actually zero.

From the issue summary:

Menu links don't really fit the notion of plugins as mostly being distinct implementations/classes (but neither do local tasks, etc., so this is water under the bridge)

Which basically says "we're using plugins for other unrelated things, we might as well do that again here"

----
Additionally, the issue summary claims that there will be no regressions from D7 once the dust settles. Except #11 describes a big regression: You can't control a custom menu link title from the node form. As a site builder, we actively discourage and avoid using the Menu UI to create links to nodes, since node authors can't access that page. That is a glaring problem.

Re: Plugins

People often think of plugins as some classes which are selected via some UI, classical example: the style of a view, the widget of a field etc.
On a more abstract level though plugins are basically discoverable implements of interfaces, so you can have different behavior.
As we all agree static menu links are really different to custom defined menu links, or menu links coming from a node. Pushing all those
different concepts into a single one makes things really fragile, hard to understand and unflexible.

This is basically argument 1 from #21.

Menu links don't really fit the notion of plugins as mostly being distinct implementations/classes (but neither do local tasks, etc., so this is water under the bridge)

Ehem, what does this tells us. If your interfaces are designed in a way that a plugin is thin and don't need the same logic as many other ones,
it is actually a bad idea to use it?

Except #11 describes a big regression: You can't control a custom menu link title from the node form.

Noone actually forces us to implement the generic entity menu link field to not have a title. Things are open for discussion, so I don't think at all that we have to introduce a regression here ...

That is not an answer. That does not make it better. In fact, it makes it worse, since everything is piling up without anyone else having a chance to review this.

There is no problem to take someone by the hand and help people to review the big monster.

In fact, the plan of removing the UI now and adding it back later piecemeal really frightens me. What if we merge in this 400K patch and the rest never happens?

Well, at least for my personal background step1,2 should either happen relative fast or continue at the end of july.

To put the plugin part of this proposal in context, the 400K patch isn't just about that. What it does is:

1. Decouples the tree management (i.e., p1-p9 columns) from the (Custom)MenuLink entity type. I think nearly everyone, including amateescu, was in agreement about the need for that.
2. Makes YAML-defined menu links not content entities. amateescu objected to that. He argued that we should just make core support YAML-defined content entities, while catch argued that that would be an even bigger can of worms this late in D8 than what this issue is suggesting. It's hard to really settle that disagreement without a patch that does the alternative to compare against this one. Because amateescu's was the minority opinion, neither he nor anyone else rushed to write such a patch.
3. Uses a plugin architecture to unify the non-content-entity implementation of YAML-defined menu links and the content-entity implementation of custom menu links into a single "menu link" interface, so that they can be mixed and matched within the same menu. catch suggested that this is more of a nice-to-have than a must-have, and would have been ok with us introducing restrictions like having admin menus only support YAML-defined links and non-admin menus only support content-entity links.

(@amateescu, @catch: please correct me if I'm misrepresenting your positions)

Under ideal conditions, I would have preferred to see the issues/patches broken up along the above steps, rather than lumping the plugin architecture into the same patch as tree decoupling. That would have made it easier to review the contentious parts in isolation (e.g., the plugin part alone would not be 400K worth). But I understand that such a partitioning would have created more work in writing the code, so I don't mind helping to review it as a single patch.

catch suggested that this is more of a nice-to-have than a must-have, and would have been ok with us introducing restrictions like having admin menus only support YAML-defined links and non-admin menus only support content-entity links.

Yes this is pretty accurate. I don't see the advantage of having these mixed together (except the fact that they currently are), and the drawbacks are all the things about this issue that people don't like.

I’m afraid this isn’t a beta blocker.  As far as I can tell, there are no important regressions of HEAD compared to Drupal 7, except for translating default menu link titles, which can be fixed without rewriting the whole system. While the current menu link system implementation (neither a ConfigEntity nor a ContentEntity) isn’t pretty, and would require a contrib module to support translating custom link titles and integrating with REST, that is not a regression compared to Drupal 7. It’s been a year since we started the API freeze and we should focus on getting the release out. We can live with those limitations for one release.  

However, if this patch brings significant performance improvements (in the critical path and not just on menu rebuild) or other similarly compelling benefits, I can still see it go in before or after beta. But it would have to go in as one big patch without interim critical regressions.

I appreciate all the great work that pwolanin, dawehner, and others have done and are doing in improving the menu system for D8, and if this patch ends up making it in, that will be great, but not at the cost of extending the Drupal 8 release. I know this is not the answer pwolanin and dawehner are looking for, and I feel sorry for that, but it is the reality of situation.

Add appropriate D8MI topic tag.

I disagree strongly with downgrading this to major. Even if it's not a beta blocker (i.e. could be committed after beta), we closed 2 other critical issues based on this plan and we need to fix the known problems before release.

In https://drupal.org/node/45111, critical task is defined as:

Drupal core release managers may identify certain strategically important non-bugs as release blockers for a particular release; e.g.:
- Severe performance regressions.
- Removal of a backwards compatibility layer for a new API.
- Significant regressions in user experience, developer experience, or documentation (at the core maintainers' discretion).
When a core maintainer decides that an issue should not block a release, its priority is downgraded accordingly.

In #28, Dries said that he doesn't consider HEAD's situation of MenuLink being neither ConfigEntity nor ContentEntity (and therefore, not translatable without a contrib module) as being a regression from D7, or improving it being strategically important enough to block release. Therefore, this is Major. I updated the relevant issues that were postponed or closed as duplicated on this one to Major as well, for the same reason. However, I also opened #2265847: [PP-0.5] Regression from D7: default titles of customized menu links aren't translated as an actual critical bug, postponed on this issue for now, because I still think this issue has a good chance of making it, but that one could be done without this if need be.

A first-order check on admin performance using devel query log shows these gains:
HEAD, cold cache: /admin: 513 queries | /admin/config: 557 queries
HEAD, warm cache: /admin: 128 queries | /admin/config: 165 queries

with patch, cold cache: /admin: 379 queries | /admin/config: 446 queries
with patch, warm cache: /admin: 108 queries | /admin/config: 131 queries

Per above, fixing "severe performance regressions" justifies a task being critical; I'll leave it to core maintainers to decide if the above numbers qualify for that. If they do, we should raise this back to critical, and IMO, to beta blocker, since our criteria for that are a) we know that something is critical before beta, and b) we know that the fix requires a data model change.

But, even if this stays Major, that doesn't exclude it from being committed either before or after beta. I could see any of the following scenarios playing out:
1) #31 happens without a hitch and this is ready to land before we're ready to release a beta anyway.
2) Between now and beta, we discover something that requires this issue to be critical and beta blocking, and so even if this takes longer than expected, we hold up beta on it.
3) This doesn't land before beta, but after beta we discover something that makes it critical, and so we commit it then. It would require a potentially annoying hook_update_N() function, and cause contrib modules that interact with menu link entities to do some rework, but that will be true for other critical bugs we discover after beta as well.
4) This doesn't land before beta, we don't discover anything that justifies it being critical, but once it's done, decide that it's worth committing anyway, because what it offers is more valuable than the cost of breaking a few early ported contrib modules (note, the percentage of contrib modules that interact with menu link entities isn't that high).
5) The patch doesn't reach RTBC before RC and doesn't deliver on enough benefit to justify it being committed after RC.

Any of the above is a possibility, and the only thing it means for this to be Major is that nothing has yet been identified that makes Dries willing to block a D8 release on this.

HEAD, cold cache: /admin: 513 queries | /admin/config: 557 queries
HEAD, warm cache: /admin: 128 queries | /admin/config: 165 queries

with patch, cold cache: /admin: 379 queries | /admin/config: 446 queries
with patch, warm cache: /admin: 108 queries | /admin/config: 131 queries

I'd want to see xhprof results to see if we're paying for the query reduction elsewhere, but that's a lot of queries saved.

Adding #2258299: dblog_menu_link_defaults_alter() must set 'machine_name' to avoid duplicates as a related issue - that's a critical bug recently found in the current code that this issue would entirely remove. It's relatively straightforward to fix but I expect there'll be more of these.

HEAD, cold cache: /admin: 513 queries | /admin/config: 557 queries
HEAD, warm cache: /admin: 128 queries | /admin/config: 165 queries

with patch, cold cache: /admin: 379 queries | /admin/config: 446 queries
with patch, warm cache: /admin: 108 queries | /admin/config: 131 queries

I see no reason we can't apply render caching to those pages, which would reduce the number of queries on these pages even more. A quick test learns that /admin/config in HEAD with warm cache does 153 DB queries, and if I simulate render caching, that'd become 121. This is for just rendering the page's main content.

(The numbers in the blockquote are not just for the main content, but for *all* things that rely on menu links, which includes the toolbar, shortcuts etc., so to get a more accurate number of the actual improvement on /admin & /admin/config, you'd need to subtract the win on non-menu link-focused pages, such as the frontpage or /node/1.)

That being said, those are indeed huge improvements that will help, but I'd question whether those improvements will continue to be impressive or even worthwhile once we render cache the main content of pages like /admin and /admin/content, and once we fix the performance of the toolbar, which also does quite a few DB queries, which is being fixed at #2254865: toolbar_pre_render() runs on every page and is responsible for ~15ms/17000 function calls (reduces the number of queries by 31 per page, many/most of which can be attributed to menu system related queries).

So, let's keep in mind that those quoted numbers are most likely inflated due to inefficiencies elsewhere (lack of render caching things that heavily rely on menu links).

I think it's the performance on cache misses that's more interesting for this patch - assuming the query reduction isn't being replaced by something else bad.

I think Peter refers to the 'View own unpublished content' permission which is indeed problematic. However, we've already dealt with somewhat similar needs with 'Edit own comments' permission and render caching comment links, Perhaps we can use similar technique with menu links.

Node/entity level permissions are considered when displaying menu items when they belong to a node/entity I believe, so not just one permission but a whole subsystem.

The render cache system can't deal with the infinite flexibility that node access system provides. So we decided a long time ago to render cache based on roles, by default. Contrib modules that implement entity access are going to have to alter in their cache keys as needed. If anyone wants to discuss this more, we should probably find another issue.

That's #2099137: Entity/field access and node grants not taken into account with core cache contexts.

Core works fine with per-role caching for all core entity types, except for the 'view own unpublished nodes' permission (which ought to be fixable with #post_render_cache).

The menu link translation issue in HEAD looks less straightforward than originally thought (when it was being ignored because other criticals meant it would just disappear) #2265847-4: [PP-0.5] Regression from D7: default titles of customized menu links aren't translated.

Whatever you do just please make sure multilingual and quickedit works with the links.

@chx: they don't currently work either, do they?

Who cares about now? I care about release.

@chx: I care for this issue resolving its scope vs. expanding its scope and not being done in Drupal 8. Totally not against solving all the problems with menus, maybe not in one swoop.

I think this issue is now done. I mean small issues will always exist but at least the main issues got addressed at least in the new jersey sprint.

+1