Summary of the summary

By using a plugin type as a common facade, we can build a single system that manages static (localizable) menu links provided by modules, views-provided link, and custom links backed by a proper fieldable and translatable content entity while establishing a very flexible API for further core and contrib innovations.

The original proposal of phased commits for Phases 1 & 2 seems impractical since we'd have needed to remove many tests and because of concerns about committing known regressions. The full proposal below has been revised to focus on a single patch to actually commit for Phase 2 that has no loss of features compared to HEAD, with Phase 1 just being for initial API review.

The proposed changes in phase 2 would resolve a number of pre-existing critical issues that were marked duplicate, postponed, or closed on favor of this, including:
#2178723: [meta-2] Finalize module-facing API of declaring menu links
#1842858: [PP-1] Convert menu links to the new Entity Field API
#2227179: Step 2: Move the menu tree storage to a separate service.
#1966398: [PP-1] Refactor menu link properties to multilingual
#2265847: [PP-0.5] Regression from D7: default titles of customized menu links aren't translated
#2177611: Check test coverage for menu link content entities with entity query
#2271659: Split up _menu_link_translate() to unblock render caching of menus
#2277979: Menu link storage does not implement SqlEntityStorageInterface

It might also facilitate critical issues like:
#474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables

Our intended timeline for phase 2 was 4-5 weeks from posting this issue on May 1. This seems to be on track based on the progress in the sandbox.

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

Original report by @pwolanin & @dawehner

Problem/Motivation

Since Drupalcon Portland many core/WSCCI contributors have been working (struggling) to bring a reasonable DX and coherence to the parts of hook_menu that were separate from routing but did not have a clear roadmap in the WSCCI initiative. This effort lead to developing local tasks, local actions, and contextual links as plugins using YAML discovery (after a detour through annotation-based discovery).

Menu links have remained the odd character out. A plan to convert them all to content entities has been in progress for a long while. pwolanin spear-headed the move of the "default content" that creates these entities from hook_menu (allowing use to remove hook_menu) to hook_menu_link_defaults, but it remained the only core system creating such default content. In addition, there have been unresolved performance problems moving these to "NG" entities. Of course, there were important motivations for using content entities, especially for user-facing links where one might add additional fields to the entity to expand the possible rendering options.

A much bigger problem than this being a one-off system is that Drupal 8 core does not have (and is very likely not to get before 8.0.x) a system for providing default content translations and keeping them in sync from localize.drupal.org. Since menu links were being implemented as content entities, this meant that 8.0.x was likely to have a major regression from 7.x in that a most of the admin interface would not be localizable upon install.

Brainstorming at DevDays Szeged, a possible solution became apparent. We could use a plugin type as an over-arching framework to build both static menu links that could be localized (for the admin sections) and also, in analogy to the custom_block entity type that is mapped to a block plugin instance, we could make a custom_menu_link content entity. This would be mapped to a menu link plugin instance and provide for the more user-facing sections of the site all the features that originally motivated the conversion of menu links to content entities.

Some people will ask "Why plugins?".
Some arguments in favor:

  • local task, local action, and contextual links are plugins using YAML discovery - thus making menu links plugin brings greater coherence to the new system
  • The existing plugin code can be leveraged to avoid writing a lot of new one-off code
  • It helps enforce a clean abstraction of the plugin definition as totally distinct from the manager & storage that knows how to efficiently build and manipulate trees
  • The exact behavior of any individual link can be controlled by setting/altering its class - doing away with hacks (that pwolanin originally wrote) like hook_translated_menu_link_alter()

Some against:

  • 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)
  • We are creating and updating definitions via the plugin manager without doing a full discovery (rebuild). This is really a scaling measure - if we were willing to assume sites would have relatively few menu links, it could all be managed via discovery. But considering sites with 1000s of nodes each with a menu link, this would fail. Some plugin manager in core already use such a mix of discovery and static addition of definitions.

Proposed resolution

Based on this plugin-based plan we (mostly dawehner) executed a first step to move from the hook to YAML files so that at least the storage format of menu links would immediately be consistent with other hook_menu-derived plugins and so upgraded modules would not need additional changes to declare their admin links even as the back-end was changed.

From this first step (#2226903), we contemplated 2 additional steps, including an incremental refactoring of the tree storage out from the entity (#2227179), and then a conversion to plugins (#2227441). However, as dawehner and pwolanin started working on the 2nd step, it became apparent that much of the work would be wasted if we intended to attempt the final step, and that the work to go from current to an initial version (phase 1 below) was roughly equal in scope. Thus, we pivoted to focus on the final step instead.

Based on the progress to date and limited discussions with core committers, we propose that we proceed to this final step. To avoid endless HEAD conflicts, build momentum, and get this new code in use, we also propose that the new system be committed in phases with the full knowledge that there will be some apparent regressions relative to 7.x until the full plan is completed.

Proposed phases (on top of existing YAML conversion):

  • Phase 1: review new APi and classes, but commit with phase 2
    1. New Features
      • Define an extensible framework for different types of links
  • Phase 2: Admin links and views, editable via menu_ui module, menu_link_content module and content entity
    1. New Features
      • Manage links defined in YAML and links defined by views
      • Full l10n support for localizing the D8 admin interface
      • i18n support via config translation for translating the links provided by Views.
      • An NG content entity whose base table stores the plugin definition
      • The custom menu link entity would be fieldable
      • Link title is the (translatable) entity label
      • Menu link content entity can be added/edited via the node form (substituting for existing functionality)
    2. Regressions relative to 7.x: none
  • Phase 3: menu link field
    1. New Features
      • Define a field that provides a menu link for an entity
      • Field API goodness: multiple values or fields per bundle
      • Add the field to any fieldable entity
      • Link title (translated) always taken from entity label
      • Link from the field is specifically connected to the entity, unlike D6/7/HEAD where the 1st link that matched the node path was used in the node edit form.
    2. Regressions relative to 7.x: none

The menu link field in Phase 3 was not part of our original plan in Szeged, but has emerged as a logical solution from discussion of how to generically provide a menu link while editing any entity. This field would store a very limited amount of data - basically weight, menu_name, and parent values, and when the link is rendered would load the entity to get the current label. The corresponding definition stored via the manager would include those field values and a few more like the route name and parameters.

Architecture and scaling:

It would not really be possible to register all the proposed custom menu links and menu link fields (node menu links) using the normal strategy of plugin discovery. You could end up trying to build an array in memory with as many elements as there are pieces of content on the site - potentially 10's to 100's of thousands or more. We propose to extend the plugin manager to allow plugin definitions to be added or updated one by one. In the case of plugins that do participate in discovery (current static menu links and Views menu links), they also need to persist the update so that it is not overwritten the next time discover happens. For example, by also updating the View config entity.

The pattern of a plugin manager that uses a combination of "normal" plugin discovery with additional definitions that are directly added is already present in core. For example, in \Drupal\Core\Validation\ConstraintManager. This makes use of a class extending \Drupal\Component\Plugin\Discovery\StaticDiscovery while has a setDefinition() method to add a definition directly.

The architecture supporting menu link plugin manager is that it utilizes a tree storage service which knows how to add a definition to the tree hierarchy, and how to retrieve all the definitions that are part of of the request tree. In this way, the tree storage replaces the use of a cache to store discovered or statically added plugin definitions, while also providing optimized retrieval. This forces the tree storage and rendering be totally decoupled from the plugin registration/discovery side which helps enforce a clean separation of concerns.

The default tree storage implementation in the patch is simply an adaptation of the materialized path schema and code that's backed menu links since Drupal 6. However, the storage service now has a clean interface, so the implementation could be replaced with a different SQL algorithm, or a noSQL store. The plugin manager only gets the plugin definition back, not the value of the mlid, p1...p9, or other internal details of the tree implementation.

User interface changes

Overall, the final site building experience will be similar or better than 7.x. Significant user-facing changes relative to 7.x:

  • Users can only enable/disable and edit menu, parent, and weight for static (YAML-defined) links.
  • A field providing a menu link means that configuration is via field/widget settings, not via node-type settings.

Timing and remaining effort

Phase 1 is already significantly complete (> 50%) in approx 10 days of part-time effort by pwolanin and dawehner. We have the plugin manager and storage working, the admin interface and toolbar works, main and secondary menus work, and you can use the menu_ui interface to re-order and re-parent links. You can create links in Views and have them appear together with other links, and editing the link updates the related view. Especially if we can get additional assistance, it's possible this could be done in another < 2 weeks.

Phase 2 could be subdivided if we want to accelerate it. Phase 2a could save the data to a table using a form provided by the plugin but nothing from the entity API. This table that would later become the entity base table, but we could make custom links work without implementing the full entity. Phase 2b would building the full entity. This work could begin in parallel with polishing Phase 1 for commit, and could follow by 2-3 weeks.

Phase 3 will build on the code and data model already defined, so the work really just involves defining a custom field and its widget, and integrating the plugin definition save to the field save code. This could start in parallel with Phase 1 or 2. Since almost all the pieces will be reusing existing code, 2-3 weeks after Phase 2 should be possible.

Follow-up work: using a different theme function or updating theme_links and related functions to work with a #type => link render array would be needed allow us to fully leverage the power of using entities and fields. It's possible this could be incorporated into Phase 2 or 3.

API changes

For modules already using YAML discovery for admin links: none

For modules interacting more directly with the menu link system, they would need to be re-written to interact with the new menu.link_tree service (the plugin manager).

Also postponed on #2:

#2273803: menu_ui_get_menus() does not perform access check
#491022: Remove weight field from menu item entity forms

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

pwolanin’s picture

pwolanin’s picture

Issue summary: View changes
xjm’s picture

Issue tags: +beta blocker

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.

xjm’s picture

xjm’s picture

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
effulgentsia’s picture

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.

Mixologic’s picture

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?

pwolanin’s picture

@Mixologic - in what we are currently planning, you'd need to create a custom menu link if you want an independent title. If you did it via a field on the entity edit form, it would always use the entity title.

In other words (based on IRC discussion):

if you are *on* the entity form and selecting "this should appear on menu X" it wold always use the entity label (which would thus use any translated values)

if you are on the Menu X admin screen and adding links, they could point link to any path and have any label (which could be translated independently)

larowlan’s picture

Thanks for such a thorough issue summary.

Gábor Hojtsy’s picture

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.

pwolanin’s picture

@Gabor - the use of t() on what are actually entity labels in current 8.x is very much a hack, and if you edit (even re-ordering by changing the weight) one you lose the localization. However you slice it, I don't think we can release that way, hence the existing issues to convert menu links to more full-formed entities.

Note that this is an existing regression compared to Drupal 7, since we had code to compare against the menu router title to decide if it was still safe to localize the title of a link after it was edited in some way.

Gábor Hojtsy’s picture

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.

tkoleary’s picture

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.

Gábor Hojtsy’s picture

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?

Gábor Hojtsy’s picture

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.

pwolanin’s picture

@Gábor Hojtsy - I think we will already have context included when the patch is ready to review. dawehner included it a while ago for the base implementation for static links that use t().

tim.plunkett’s picture

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.

effulgentsia’s picture

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.

pwolanin’s picture

FYI, the sandbox at this point is actually into phase 2, since re-writing the tests without a custom menu link entity was more of a pain than creating it.

So the patch looks big since we are removing one entity class and adding another, as well as moving around all the tree code.

tim.plunkett’s picture

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.

dawehner’s picture

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.

effulgentsia’s picture

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’s picture

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.

pwolanin’s picture

@catch - I think mixing them together is really necessary, and we already have it working for static links, Views-defined links, and entity-backed custom links. This is the real advantage of using the plugin system - it makes this mixing pretty trivial. It also enables a tremendously flexible API for contrib.

@tim.plunkett - I would see the ability to add a menu link field to any content entity and having the title in sync with the entity label and its translations as a net feature gain compared to manually entering it on the edit form. However, you could easily have a module that gives you something like the D7 experience by building this as a reference field to a custom menu link entity.

Dries’s picture

Priority: Critical » Major
Issue tags: -beta blocker

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.

pwolanin’s picture

@Dries - I do expect this to be a significant performance gain, especially on admin pages. Obviously we'll need to collect the data. It's also not clear to me that the code in HEAD will effectively support localization.

I'm not sure what you consider a "critical" regression here. We have the Phase 2 code nearly done, which will have some new (planned, previously beta-blocking) features compared to HEAD, but removes the menu OTF form from the node edit form.

pwolanin’s picture

Priority: Major » Critical

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.

pwolanin’s picture

Revising the issue summary for a somewhat revised/accelerated plan that dawehner and I are executing now:

Phase 1 will be for review for the new classes, but probably just for discussion/feedback not commit. I will post that patch there now.

Phase 2 will use the existing menu link form on the node edit form with the new menu link content entity, so we'll have no visible functional change compared to HEAD. This is mostly done but we are still resolving test failures and possibly blocked on a critical bug in the content translation system. I feel strongly this should be in the 1st beta.

Phase 3 will add a menu link field, ideally replacing the form altered into the node form, or simply as an optional enhancement. This is clearly post-beta or possibly a 8.1.

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
Issue tags: +D8MI
pwolanin’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue tags: +language-content

Add appropriate D8MI topic tag.

effulgentsia’s picture

Priority: Critical » Major

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.

catch’s picture

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.

pwolanin’s picture

Issue summary: View changes
Wim Leers’s picture

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).

catch’s picture

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.

pwolanin’s picture

@Wim Leers - In the issue about render caching of menu tree, it seems an entirely open question as to whether it's possible, since e.g unpublished nodes or other entites might appear for one user but not another even if they have the same role?

moshe weitzman’s picture

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.

Gábor Hojtsy’s picture

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.

moshe weitzman’s picture

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.

catch’s picture

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).

catch’s picture

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.

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Nothing that we should track #2136559: Config entity admin lists show configuration with overrides (eg. localized) as related, since we'd potentially want to list non-localized admin links for the admin listings/forms (but this isn't solved in core yet).

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
chx’s picture

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

Gábor Hojtsy’s picture

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

chx’s picture

Who cares about now? I care about release.

Gábor Hojtsy’s picture

@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.

pwolanin’s picture

dawehner’s picture

Status: Active » Fixed

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.

Wim Leers’s picture

+1

Status: Fixed » Closed (fixed)

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