Problem/Motivation

see: #2256497: [meta] Menu Links - New Plan for the Homestretch

The architecture and general implementation is being reviewed in #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. Comments on those aspects of the patch should be posted to that issue.

This issue is to focus on implementation details, tests, and code and comment polish necessary to get the patch committed.

Proposed resolution

This issue to to build to a patch that will replace almost entirely the system in HEAD with a new one with minimal user-facing changes but substantial new features under the hood and performance gains.

  • Define an extensible framework for different types of links, with a plugin type as a common facade
  • The developer-facing APi is a plugin manager that also handles building trees into render arrays and access checks. The tree storage is a separate service that hides the implementation details of efficient hierarchy handing
  • 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.
  • A NG content entity whose base table stores the plugin definition
  • The custom menu link entity is defined as being fieldable
  • Link title is the (translatable) entity label
  • Menu link content entity can be added/edited via the node form (substituting for existing functionality)

Remaining tasks

  1. #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage
  2. #2301273: MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests
  3. #2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI
  4. #2301317: MenuLinkNG part4: Conversion
  5. #2301319: MenuLinkNG part5: Remove dead code; and party!

User interface changes

Minimal. Module-defined menu links will have static title, description, and path that are not editable via the UI.

API changes

A new menu link system.

For reviewers there are 3 very notable changes:

  1. All menu links operate behind the facade of the \Drupal\Core\Menu\MenuLinkInterface. This means that multiple different implementations can operate together in the same tree. In addition, since we consider them as plugins, the details of the optimized tree storage are not exposed. The MenuTreeStorageInterface takes only a plugin definition and returns it again, while the details of the implementation (e.g. the p1-p9 columns) are not accessible or part of the API.
  2. The methods for loading and rendering menu link trees have been completely consolidated and made consistent in just 3 methods on the MenuLinkTreeInterface. A lot of previously crufty code that was forward-ported from 6 and 7 has been removed, aided by the decoupling of breadcrumbs form menu links
  3. The code for performing these steps has been broken up into multiple services, each with a clear interface so that in the cases where the behavior needs to be customized for a site or for a different tree storage (e.g. noSQL) only a limited amount of logic needs to be re-implemented to affect the change.

Patch map

This patch completely removes the menu_link module. It moves some of that module's former logic into \Drupal\Core\Menu, some into a new menu_link_content module, and removes some former logic. Details:

  • hook_translated_menu_link_alter(): removed. Can override plugin class instead.
  • hook_menu_link_CRUD_OP(): removed. Menu links are no longer entities, but for custom links, there's now hook_menu_link_content_CRUD_OP().
  • hook_menu_link_defaults_alter() -> hook_menu_links_discovered_alter().
  • menu_link_schema() -> MenuTreeStorage::schemaDefinition()
    • fields changed:
      • uuid + machine_name -> id
      • plid -> parent
      • link_path -> url OR route_name and route_parameters. url is only populated for external links
      • langcode + link_title -> title + title_arguments + title_context
      • module -> provider (in line with all other plugins)
    • fields removed:
      • customized -> (replaced with MenuLinkInterface::isResetable())
      • external (replaced by check if url is populated or Url::isExternal() after getting the Url object)
      • updated (this was outdated for D5 -> D6 update)
    • fields added:
      • description
      • class
      • metadata
      • form_class
    • private fields (in the schema of MenuTreeStorage, but not accessibly via any API):
      • route_key
      • discovered
      • p1..p9 (these were public in HEAD, but are made private by the patch)
  • menu_link_help(): removed. Do we need a menu_link_content_help()?
  • menu_link_uri() -> MenuLinkInterface::getUrlObject()
  • menu_link_CRUD_OP(): removed as menu links are no longer entities. Interact with MenuLinkManager instead (e.g., $menu_link_manager->getInstance(), $menu_link_manager->updateLink(), etc.).
  • menu_link_maintain(): removed. See above.
  • menu_link_system_breadcrumb_alter() -> menu_ui_system_breadcrumb_alter().
  • Entity\MenuLink ->
    • Storage-related functionality of link hierarchy and rebuild moved to MenuTreeStorage.
    • Storage-related functionality of custom links moved to MenuLinkContent.
  • MenuLinkAccessController ->
    • For routes (e.g., menu_ui.link_edit) that only need to check "administer menu" permission, check just uses _permission.
    • Access control of custom links moved to MenuLinkContentAccessController.
    • Access control of menu_ui.link_reset moved to MenuLinkResetForm::linkIsResetable().
  • MenuLinkDeleteForm -> MenuLinkContentDeleteForm
  • MenuLinkForm -> MenuLinkFormInterface (with MenuLinkDefaultForm and MenuLinkContentForm implementations)
  • MenuLinkInterface: removed as obsolete. The new MenuLinkInterface contains different methods.
  • MenuLinkStorage ->
    • Tree-related storage moved to MenuTreeStorage
    • Entity related storage removed as obsolete, because MenuLinkContent can use regular content entity storage.
  • MenuTree -> split into various new classes in \Drupal\Core\Menu.
  • StaticMenuLinks: removed. Static link discovery embedded into MenuLinkManager.

This patch also moves a bunch of procedural code from menu.inc to appropriate classes and removes code made dead by the new architecture. Details:

  • _menu_item_localize(): removed. MenuLinkInterface::getTitle() and other getter methods return localized strings.
  • _menu_link_translate() ->
    • unserializing moved to MenuTreeStorage::prepareLink()
    • access checking moved to a "menu tree manipulator" (DefaultMenuLinkTreeManipulators)
    • title/URL/etc. expansion moved to MenuLinkInterface getter methods.
  • menu_(get|set)_active_menu_names(): removed. Was obsolete code even before this patch due to refactored breadcrumbs already in HEAD; this patch just surfaced that.
  • menu_link_get_preferred() -> MenuActiveTrail::getActiveLink().
  • menu_reset_static_cache(): removed, because that static cache is removed.
  • _menu_link_save_recursive() -> MenuTreeStorage::saveRecursive().
  • menu_link_rebuild_defaults() -> MenuLinkManager::rebuild() which passes the discovered definitions into MenuTreeStorage::rebuild().
  • menu_load_links() -> MenuTreeStorage::loadByProperties().
  • menu_delete_links() -> MenuLinkManager::deleteLinksInMenu().
  • _menu_(update|set)_expanded_menus(): removed. This optimization was not carried into the new architecture for simplicity. This might need profiling to determine the performance cost, but once we render cache menus, it might change the value of this optimization.
Files: 
CommentFileSizeAuthor
#144 Screenshot 2014-07-11 08.08.17.png152.83 KBlarowlan
#144 Screenshot 2014-07-11 07.38.26.png69.77 KBlarowlan
#144 Screenshot 2014-07-11 07.38.15.png47.53 KBlarowlan
#144 Screenshot 2014-07-11 07.37.53.png19.5 KBlarowlan
#144 17vyt0zgrb5o8jpg.jpg17.84 KBlarowlan
#143 2256521-142.patch617.18 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,741 pass(es).
[ View ]
#143 increment.txt13.72 KBpwolanin
#140 2256521-140.patch614.56 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,740 pass(es).
[ View ]
#140 increment.txt11.69 KBpwolanin
#135 new_plan_phase_2-2256521-134.patch612.31 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,727 pass(es).
[ View ]

Comments

larowlan’s picture

Willing to help here

pwolanin’s picture

Issue tags:+beta blocker
xjm’s picture

Priority:Critical» Major
Issue summary:View changes
Status:Active» Postponed
Issue tags:-beta blocker

Setting status per #2256497-28: [meta] Menu Links - New Plan for the Homestretch, and postponing on step 1.

pwolanin’s picture

Issue tags:+beta target

I still consider this a beta target, and plan to get it done.

pwolanin’s picture

Status:Postponed» Active
pwolanin’s picture

Title:New plan, Phase 2: custom_menu_link module and content entity» New plan, Phase 2: menu_link_content module and content entity
Priority:Major» Critical
Related issues:+#1848552: Toolbar icons disappear with translated menu, +#2143291: Clarify handling of field translatability

This issue will solve translation of menu links, so it's critical as a replacement for this other critical issue: #1966398: [PP-1] Refactor menu link properties to multilingual

Also related: #1848552: Toolbar icons disappear with translated menu Though marked closed/fixed, the current code would break again as we make the title translatable, so we need to address that problem as part of this patch.

however, we make be blocked with PHP warnings in tests until this is resolved: #2143291: Clarify handling of field translatability

sun’s picture

This functionality should not live in a separate module.

There's no need for that level of pluggability. A separate module only harms UX as well as installation and test performance for no benefit in return.

pwolanin’s picture

@sun - which functionality should not be separate? The content entity and the menu UI?

pwolanin’s picture

Title:New plan, Phase 2: menu_link_content module and content entity» New plan, Phase 2: Admin links and views, editable via menu_ui module, menu_link_content module and content entity
Issue summary:View changes
pwolanin’s picture

Title:New plan, Phase 2: Admin links and views, editable via menu_ui module, menu_link_content module and content entity» New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module
effulgentsia’s picture

Priority:Critical» Major

Resetting priority to match the parent issue. For details, see #2256497-36: [meta] Menu Links - New Plan for the Homestretch. This could still be raised to critical in the future if warranted (e.g., if a core maintainer decides that performance of admin/config warrants critical priority or if critical bugs are discovered in HEAD that require this).

effulgentsia’s picture

From #2227441-94: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins:

Discussed multi-load of MenuLinkContent entities with catch and alexpott in IRC today. Both agreed we could do that in a follow-up...
[09:52am] pwolanin: catch: since they are not created by core, I'm not sure if we need to address that in the 1st patch...
[09:58am] catch: pwolanin: hmm, it is probably OK as a follow-up - especially if it's a performance improvement otherwise.

Not my call, but I disagree with the above. In #2256497-28: [meta] Menu Links - New Plan for the Homestretch, Dries requested no interim critical regressions. Core doesn't ship with any nodes either, but performance of node rendering is considered critical. Similarly, I think the performance of front-end menus is more critical than performance of admin menus/pages. Especially since this issue implements custom menu links as EntityNG relative to HEAD's non-NG, I think even with multiloading differences aside, we need to know whether it's a performance regression relative to HEAD or not prior to commit, but to measure that, we need the patch to implement multiloading so at least that part is equivalent to HEAD. That said, I have an idea for how to do that, which I'll post here after there's an updated patch that applies against HEAD.

effulgentsia’s picture

Status:Active» Needs review
StatusFileSize
new558.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,274 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
new4.37 KB

Here's a rough multiload implementation. Interdiff relative to #2227441-97: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. Further refinement of it should be discussed in #2275297: Implement multiload for menu_link_content, not here.

pwolanin’s picture

The current code is in a new sandbox branch: http://cgit.drupalcode.org/sandbox-dereine-2031809/log/?h=2256521

Will post a patch here one I'm sure things work after the PSR-4 move, the multi-load fix, and some other changes.

likin’s picture

Should we move the patch from this branch to the sandbox?

pwolanin’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new99.25 KB
new1007.62 KB
new1006.73 KB
new556.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,220 pass(es).
[ View ]

@linkin - not sure what you mean. All the most current patch code is in that branch in the sandbox.

Working from current HEAD (34c6f66988fcbd0ef1222e5653be38d8f0eeb9a3) with a clean install or the patch on top of that (attached), I get a pretty dramatic difference in performance for /admin/config.

It's a bit hard to read in into the changes per method, since many of them were renamed or moved.

This comparison was made with a warm cache (page reloaded a few times before collecting the xhprof run). The actual xhprof data is attached. The run1 in the diff is HEAD, while run2 is with the patch, so the 9% reduction in function calls is consistent with what I saw before.

NOTE: the memory difference seems to have been inflated by opcode cache being full - going to re-run.

pwolanin’s picture

Ok, re-running it, still the same improvement in # function calls and time, but the difference in memory use is trivial.

An example of a single method that shows a big change is Drupal\system\SystemManager::getAdminBlock, where HEAD and the patch have the same number of function calls, but the patch code is much more efficient.

effulgentsia’s picture

StatusFileSize
new2.58 KB

I think the performance of front-end menus is more critical than performance of admin menus/pages. Especially since this issue implements custom menu links as EntityNG relative to HEAD's non-NG, I think even with multiloading differences aside, we need to know whether it's a performance regression relative to HEAD

Attached is a patch that contains a tester module I wrote that generates a menu with 100 custom links on install, and renders it on /menu-bench. On my machine, running ab -c1 -n100:
HEAD: 295ms
Patch (#17): 417ms

That's a pretty serious regression. I think we need to get to the bottom of what's causing it: is it all due to EntityNG, and can it be optimized?

pwolanin’s picture

@effulgentsia -Are all 100 showing on one page? It would seem so since they are all using that route.

menu links were not converted to NG entities previously because of the performance issues it caused. So we should dig in, but this result doesn't surprise me. We are providing user-created links as NG content entities for the features like translation support and field-ability which have overhead.

As far as performance optimizations, dawehner and I have discussed with berdir making the several of the base table fields into one field like is proposed for shortcut as a follow-up, though I'm not sure how much that matter if you just load the entity to get the title and description.

Also, our phase 3 proposal of a link field would also potentially eliminate need to load a menu link content entity for links connected to nodes, etc.

I'm sure we'll want to get some general entity performance improvements in core regardless.

effulgentsia’s picture

One difference I'm seeing is that in HEAD, the "tree-data" cache stores the MenuLink entity objects, meaning they do not need to go through the whole loadMultiple() flow on every request. However, with the patch, that cache stores the plugin definition arrays only, so the plugin instance objects need to be factory created and the MenuLinkContent entities need to be loaded on every request. The latter part might get better with #597236: Add entity caching to core.

I think we'll need more discussion with catch and Dries on how much of a perf regression is acceptable, especially considering #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees will end up caching the entire rendered output anyway, and also considering various follow ups that could optimize things. However, a concern that amateescu and others raised from the beginning was that the plugin architecture would add overhead, and I think we have here some early indication that it is, though possibly we can still improve that.

effulgentsia’s picture

StatusFileSize
new2.83 KB

Improvements to the tester module. #19 had all 100 links going to the route that was being displayed, causing all 100 to be loaded by menu_link_get_preferred() despite the tree-data caching mentioned in #21. This one points those links elsewhere to avoid that.

With this test, here are the numbers with xdebug disabled (the numbers in #19 were with it enabled):
HEAD: 137ms
Patch (#17): 217ms

Are all 100 showing on one page?

Yes. The point of this is to benchmark a not-all-that-rare case of a site with a large site (non-admin) menu.

pwolanin’s picture

@effulgentsia - if HEAD is actually caching (serializing) the entities that sounds pretty broken to me, or at least a lot more fragile and prone to getting stale depending on how the entities are updated.

I'd rather rely on caching by the entity system: #597236: Add entity caching to core

I'd be surprised here if the plugin element is adding any significant overhead - there is very little code or logic in it.

In IRC, berdir confirms that content entities are known to be slower that base entities. So again, I see this approach as allowing for a faster OOTB admin experience and toolbar, adds critical i18n features, and gives us a lot of flexibility.

It would be easy in contrib using this plugin facade to add a non-entity custom link module for mono-lingual sites. You could just rely on the tree storage, so this would be fast and simple. BUT you don't get translations or fields. The flexibility of the plugins will enable a lot of contrib innovation, and is another benefit.

effulgentsia’s picture

How about this: what if for this issue's scope, we change the MenuLinkContent plugin to not forward getTitle() and getDescription() to the content entity, but instead have it return what's in the plugin definition (and on createLink(), updateLink(), etc. have it populate that definition with the monolingual value)? That would not be a functional regression compared to HEAD. Then, we can have a followup issue to make multilingual titles/descriptions work. The benefit of doing it that way is it completely removes the entity loading and TypedData overhead from the render path, bringing the number from #22 down to 158ms: still slower than HEAD, but not by nearly as much, and perhaps we can still find other things to optimize to get it even closer.

This way, we can clear the path to getting the general 600k refactor in, and then have a separate issue to discuss how much performance trade-off is acceptable to support multilingual menus by default. Perhaps even the followup issue could implement something smart like only forward to the entity if more than one language is enabled on the site.

pwolanin’s picture

@effulgentsia - that's an interesting suggestion, since we could get the best of both. I'll take a crack at it.

pwolanin’s picture

StatusFileSize
new11.23 KB
new553.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,400 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Here's a re-roll to account for #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities and to try to implement the optimization suggested by @effulgentsia. It looks like the isMultilingual() is a reasonable check to use, but happy to have suggestions if there is something better or more reliable for this use case.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new553.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,416 pass(es).
[ View ]

Missed one entity schema change needed in that test:

-    $this->installSchema('menu_link_content', array('menu_link_content', 'menu_link_content_data'));
+    $this->installEntitySchema('menu_link_content');
pwolanin’s picture

StatusFileSize
new39.56 KB
new557.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

This has a lot of code style and comment cleanup, but no functional code changes other than removing the default table name for the tree storage.

pwolanin’s picture

StatusFileSize
new557.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,900 pass(es), 0 fail(s), and 14 exception(s).
[ View ]

oh, silly me - needs to be public.

+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
@@ -35,7 +35,7 @@ public static function getInfo() {
   /**
    * {@inheritdoc}
    */
-  protected function setUp() {
+  public function setUp() {
pwolanin’s picture

Status:Needs work» Needs review
pwolanin’s picture

Status:Needs work» Needs review

And small test fix due to constructor change.

+++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
@@ -50,7 +50,7 @@ public static function getInfo() {
   protected function setUp() {
     parent::setUp();

-    $this->treeStorage = new MenuTreeStorage($this->container->get('database'), $this->container->get('url_generator'));
+    $this->treeStorage = new MenuTreeStorage($this->container->get('database'), $this->container->get('url_generator'), 'menu_tree');
     $this->connection = $this->container->get('database');
   }

pwolanin’s picture

StatusFileSize
new557.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,403 pass(es).
[ View ]
pwolanin’s picture

StatusFileSize
new557.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,343 pass(es).
[ View ]

re-roll for conflict in system.schema.yml

pwolanin’s picture

StatusFileSize
new222.86 KB

While the proposed architecture was reviewed and marked fixed/accepted in the prior issue, catch asked for it to be posted here again to facilitate final review.

Here's a patch for review only containing only the new files added by the patch, which are:

core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
core/lib/Drupal/Core/Menu/MenuLinkBase.php
core/lib/Drupal/Core/Menu/MenuLinkDefault.php
core/lib/Drupal/Core/Menu/MenuLinkInterface.php
core/lib/Drupal/Core/Menu/MenuLinkTree.php
core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
core/lib/Drupal/Core/Menu/MenuTreeStorage.php
core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php
core/lib/Drupal/Core/ParamConverter/MenuLinkPluginConverter.php
core/modules/menu_link_content/menu_link_content.info.yml
core/modules/menu_link_content/menu_link_content.install
core/modules/menu_link_content/menu_link_content.local_tasks.yml
core/modules/menu_link_content/menu_link_content.module
core/modules/menu_link_content/menu_link_content.routing.yml
core/modules/menu_link_content/src/Controller/MenuController.php
core/modules/menu_link_content/src/Entity/MenuLinkContent.php
core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
core/modules/menu_link_content/src/MenuLinkContentAccessController.php
core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
core/modules/menu_ui/src/Form/MenuLinkEditForm.php
core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
core/modules/system/config/install/menu_link.static.overrides.yml
core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
core/modules/system/tests/modules/test_page_test/test_page_test.menu_links.yml
core/modules/user/src/Plugin/Menu/MyAccountMenuLink.php
core/modules/views/src/Plugin/Derivative/ViewsMenuLink.php
core/modules/views/src/Plugin/Menu/Form/ViewsMenuLinkForm.php
core/modules/views/src/Plugin/Menu/ViewsMenuLink.php
core/modules/views/views.menu_links.yml
core/profiles/standard/standard.menu_links.yml
core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeTest.php

effulgentsia’s picture

I reran the benchmark from #19/#22/#24 and found the same result as #24 if the site is monolingual (default install of Standard):

HEAD: 136ms
Patch (#36): 159ms

@catch: given #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees, are you concerned about this regression? I can try to dig in and find the source of it (I suspect the fairly large call chain in instantiating the plugins is partially responsible) and ideas to optimize, but how important is that if that issue will cache it anyway?

pwolanin’s picture

Even with standard it should be monolingual?

So, if this is a general problem for plugins, we should address it in a general way for Drupal 8 as a performance enhancement?

In this specific case, I could try to instantiate all the plugins initially to see what kind of speed gain that gives, though I'm not really fond of the extra complexity and potential fragility that adds.

effulgentsia’s picture

I was wrong about plugin instantiation being a significant contributor. I'm still unclear on what's causing the difference.

pwolanin’s picture

StatusFileSize
new787 bytes
new557.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,361 pass(es).
[ View ]

Here's a small tweak to make the base form more consistent with the entity form and use an HTML5 input element.

pwolanin’s picture

Using xhprof, it's easy to identify a notable difference:

Symfony\Component\Routing\Generator\UrlGenerator::doGenerate and Drupal\Core\Utility\LinkGenerator::generateFromUrl

are called a lot more with the patch since the link is generated at run time from the route and parameters. In contrast, current HEAD saves the system path as part of the entity data.

i.e. in preSave() HEAD does:

$this->link_path = \Drupal::urlGenerator()->getPathFromRoute($this->route_name, $this->route_parameters);

Using getPathFromRoute() which is marked as deprecated and to be removed before 8.0.

I would argue the HEAD behavior is at best fragile and arguably incorrect since it's basically storing forever something that should only be cached or not used at all. We could certainly speed this bit up in the patch potentially by generating the href and caching it earlier, though that would be a bunch of churn and maybe a change to the LinkGenerator that I'd rather do as a follow-up. I know there is also already a patch in the works to add caching to the UrlGenerator.

The change in function theme_menu_link(array $variables) is basically:

-  $element['#localized_options']['set_active_class'] = TRUE;
-  $output = l($element['#title'], $element['#href'], $element['#localized_options']);
+  /** @var \Drupal\Core\Url $url */
+  $url = $element['#url'];
+  $url->setOption('set_active_class', TRUE);
+  $output = \Drupal::linkGenerator()->generateFromUrl($element['#title'], $url);

The l() function call we removed calls UrlGenerator::generateFromPath(), which is marked as deprecated and to be removed before 8.0, so it seems this is moving us closer to release in that regard. This relates also to our other discussion about uses of _system_path in core and trying to get rid of all uses except pre-boot and actual route matching.

catch’s picture

Given #42 that's an issue with using the generator rather than this patch.

The menu tree caching patch will hide it, I think we also need a separate issue to profile generator performance - I really didn't like it from a performance perspective when it went in originally, and it's at least as bad as expected.

pwolanin’s picture

StatusFileSize
new971 bytes
new557.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,373 pass(es).
[ View ]
new76.77 KB
new98.52 KB
new92.87 KB
new81.35 KB
new103.71 KB
new113.66 KB
new42.53 KB
new76.72 KB
new95.46 KB
new87.19 KB

Here's a series of screenshots plus a small update to the patch.

Embedding the key edit forms:

Editing a static (module-provided) link

Adding a new menu link content link

After enabling multi-lingual, the link has a language selector and Translate tab

Adding a link via Views, it has a form similar to the static link, but the title can be edited.

Bojhan’s picture

I see no major issues with this, I think that its clear to users that it cannot be directly translated or configured and we inform them what is happening.

I would suggest a few textual changes:

For static links:
This link is provided by the X module. The label and path can not be changed.

To provide more information where its exactly coming from, because thats the first thing a user would probably wonder. Especially when these links are coming through contrib modules.

For links where you know the source, such as Views.

This link is provided by Views module. The path can be changed by editing the Frontpage view.

To provide more information where this link is coming from, to decrease the amount of searching you have to do to find this link - when you have dozens of views.

pwolanin’s picture

StatusFileSize
new82 KB
new111.31 KB
new24.79 KB
new561.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,459 pass(es).
[ View ]

Ok, here's a 1st implementation of the 2 form changes suggested by Bojhan, plus more code style cleanup and a little test fix.

Screen shots show the updated edit forms.

larowlan’s picture

Attempting to review this behemoth, using https://github.com/larowlan/drupal/pull/3/files because dreditor not an option and would hate to loose my comments

pwolanin’s picture

Looking there, it seems we have a schema.inc change in the patch that may be unrelated. Looks like it carried over from one of the other entity or field patches we needed before. Let me re-make it without.

pwolanin’s picture

StatusFileSize
new3.39 KB
new561.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,485 pass(es).
[ View ]

Here's that cleanup and some comment fixes.

AndyThornton’s picture

Postponed/duplicated https://drupal.org/node/2266629#comment-8855469 (
Fix menu link reparenting logic) on @pwolanin's recommendation

larowlan’s picture

Issue tags:+Needs manual testing

My code review is here https://github.com/larowlan/drupal/pull/3

I will keep it up to date with any interdiffs posted here as well as 8.x

I have some concerns around the size/scope of the plugin manager.

I have some concerns around misuse of the config system to store overrides.

No UI or manual testing yet.

pwolanin’s picture

StatusFileSize
new14.54 KB
new560.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,480 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

This starts addressing the feedback in that PR review.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new19.13 KB
new560.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,535 pass(es).
[ View ]

Oops - ignore #52. This has the updated patch and changes since #49

pwolanin’s picture

StatusFileSize
new9.78 KB
new562.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256521-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Responding to feedback from @larowlan that there needs to be a way to override the form element used to select the menu parent, this patch adds a method to the manager so that that element could be changed by subclassing and setting the subclass as the service.

In the prior issue for general review, we had some debate about the size/scope of the plugin manager but @effulgentsia felt he was comfortable enough going forward as-is so I'd rather not re-debate that unless we missed something. #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins

pwolanin’s picture

conflicts in comment.module and SystemController.php

The latter is not immediately trivial to resolve because of the changes in #2085571: admin/content should not depend on node.module.

That adds a $path parameter to SystemController::overview() that needs to be changed to a link ID. I'll have to finish re-rolling in the morning.

pwolanin’s picture

So, the SystemController fix seems not hard, but then there is a circular dependency problem due to the access manager and param converter. This is due to recent core change:

Issue #2250239 by dawehner | sun: Remove needless ContainerAware dependency

A work around suggested by daniel of setting injection, doesn't completely solve the problem so far. I think this may be the reason to split the actual plugin manager from the tree code that does access checking.

effulgentsia’s picture

In the prior issue for general review, we had some debate about the size/scope of the plugin manager but @effulgentsia felt he was comfortable enough going forward as-is

Reference for that is #2227441-84: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins. While decoupling more would be nice, I don't think it needs to hold up this patch unless there's something that makes things more monolithic than HEAD is already.

I think this may be the reason to split the actual plugin manager from the tree code that does access checking.

If needed to resolve bugs / circular dependencies, or even if you just feel like it, I certainly have no objections to you splitting MenuLinkTree into less monolithic pieces.

pwolanin’s picture

Yes, working on the split - basically we will have 2 services:

  plugin.manager.menu.link:
    class: Drupal\Core\Menu\MenuLinkManager

and

  menu.link_tree:
    class: Drupal\Core\Menu\MenuLinkTree

both will have the menu.tree_storage service injected, and the menu.link_tree will also have the plugin manager injected.

Places like the param converter and edit forms will only need the plugin manager service, and that will fix the dependency loop in terms of the param converter and access mananger.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new569.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,958 pass(es), 52 fail(s), and 13 exception(s).
[ View ]

This will still have some test fails, but it's mostly working and you can see the logic of the class split.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new557.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,377 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I think the tests will pass now.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new716 bytes
new557.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,107 pass(es).
[ View ]

oops - renamed that resetDefinitions() method and don't need to call it anyhow in the test.

pwolanin’s picture

StatusFileSize
new3.53 KB
new558.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,106 pass(es).
[ View ]

Some small doxygen fixes.

pwolanin’s picture

A github PR with this new patch is at https://github.com/pwolanin/drupal/pull/1/files for anyone who wants to review it using that format (but please summarize your comments back here).

pwolanin’s picture

StatusFileSize
new5.88 KB
new559.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,110 pass(es).
[ View ]

Here are some other small fixes from self-review.

effulgentsia’s picture

There's a new source of performance slowdown for content links. Updated numbers from #38:

HEAD: 159ms (HEAD seems to have slowed down since then)
Patch (#68): 238ms

This is about the same magnitude slowdown as in #22 prior to the isMultilingual() optimization of #26, so I thought maybe something new is causing MenuLinkContent entities to get loaded despite that optimization, but their constructor isn't running, so it's something else. Possibly an xhprof comparison could help find it.

pwolanin’s picture

Wow - that's frustrating. Let me make sure the static caching of definitions in the menu tree storage is working correctly - that would be the most likely cause since it would mean an extra 100 SQL queries on that page.

pwolanin’s picture

StatusFileSize
new38.83 KB
new556.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,183 pass(es).
[ View ]

So, indeed, that was probably the problem causing worse performance, but the solution was more involved than just finding a logic error. I needed to push the tree cache down to the menu tree storage (which is really just a fancy definition cache anyhow) so that it could get back the relevant definitions from the cache entry instead of re-loading each one from the database.

I'm not sure the incremental diff has 100% of the changes since the last patch, but you can at least see the major changes.

pwolanin’s picture

So, I ran the ab test 2x before and after patch #71:

Patch #68
Time per request: 647.767 [ms] (mean)
Time per request: 677.689 [ms] (mean)

HEAD:
Time per request: 434.074 [ms] (mean)
Time per request: 429.293 [ms] (mean)

Patch #71:
Time per request: 438.852 [ms] (mean)
Time per request: 432.480 [ms] (mean)

So this brings it back very close to HEAD (obviously my machine is slower than Alex's)

effulgentsia’s picture

Thanks for fixing the definition caching! On my machine, I get a larger regression of #71 relative to HEAD than what's reported in #72. I get:

HEAD: 159ms
Patch (#71): 183ms (so, better than #69)

However, I wanted to test the hypothesis of #42 that most of that is due to UrlGenerator, and indeed it is. I filed #2289319: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache to move that from theme_menu_link() to the tree-data cache, so as to bring the runtime rendering logic closer to what HEAD does. With that patch applied, I get:

Patch (#71 + #2289319): 160ms (essentially parity with HEAD, yay!)

@catch: given #43, do you think we should commit #2289319: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache (or a refinement of it) into this issue's sandbox, in order to remove the interim regression, but then potentially undo it once alternate caching is in place, or would you rather keep the code cleaner despite an interim regression?

pwolanin’s picture

See #42 - getPathFromRoute() is marked deprecated, so I don't think adding that back is the right way forward.

Wim Leers’s picture

pwolanin and I discussed in detail (3.5 hour long chat detail) how to move this forward and combine with #2289033: Refactor MenuTree to be simpler and have full unit test coverage.

Even though pwolanin set out to refactor just Menu Link entities/storage, and I set out to just add render caching of menu trees (in #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees), and those logically have close to zero overlap, the sad reality is that the menu system in HEAD is in a poor state. Due to the tight coupling in MenuTree, where lots of things live together that don't belong together, and worse yet, MenuTree calling out to procedural functions in menu.inc, the unfortunate consequence is that we have both cleaned up in different ways, with different goals in mind.

So, alas, even though we both had best intentions, we're now left to somehow reconcile both patches.

After careful deliberation, we've agreed to move forward by:

  1. Closing #2289033: Refactor MenuTree to be simpler and have full unit test coverage as a duplicate.
  2. "Upgrading" #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module (this issue) to incorporate the logic of that closed issue.
  3. That will then still allow the main "render tree caching" logic over at #1805054-123: Cache localized, access filtered, URL resolved, (and rendered?) menu trees to work, and apply relatively cleanly. Conceptually, this patch is "just" merged with that in #2256521.

We both began the discussion sad and frustrated, and came out hopeful — particularly because we should be able to move faster by combining our efforts on the same patch :) Particularly, we both want this mammoth, long overdue work to be behind us :P

The plan

In short: the following will be moved into pwolanin's patch from mine: MenuTreeItem, MenuTreeParameters, the menu tree manipulators concept, DefaultMenuTreeManipulators, MenuTree::render() (which brings all rendered menus to a single template: menu-tree.html.twig), the use of MenuTreeParameters everywhere and all added unit test coverage.

The rendering process becomes:

  1. $tree = build($menu_name, $parameters) or buildSubtree($menu_name, $parent)
  2. $tree = transform($tree, $manipulators)
  3. $rendered = render($tree)

The plan: detail

pwolanin and I have also discussed how to do this exactly. Key to this all is that we agree on MenuTreeInterface (pwolanin renamed this to MenuLinkTree), so that's what we discussed in detail, and where we've agreed on how to combine everything.

In pwolanin's latest patch, over at #2256521-71: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module, it looks like this:

<?php

interface MenuLinkTreeInterface {

  public function maxDepth();
  public function buildRenderTree($tree);
  public function getActiveTrailIds($menu_name);
  public function getActiveMenuNames();
  public function setActiveMenuNames(array $menu_names);
  public function buildPageData($menu_name, $max_depth = NULL);
  public function buildAllData($menu_name, $id = NULL, $max_depth = NULL);
  public function buildTree($menu_name, array $parameters = array());
  public function buildSubtree($id, $max_relative_depth = NULL);
  public function getChildLinks($id, $max_relative_depth = NULL);
  public function menuLinkGetPreferred($route_name = NULL, array $route_parameters = array(), $selected_menu = NULL);
  public function getParentSelectOptions($id = '', array $menus = NULL);
  public function parentSelectElement($menu_parent, $id = '', array $menus = NULL);
  public function getMenuOptions(array $menu_names = NULL);
  public function getParentDepthLimit($id);
  public function resetStaticCache();

}

And we're going to refactor that to this, where "commented = planned to be deleted":

<?php

interface MenuLinkTreeInterface {

  public function maxDepth();
  //public function buildRenderTree($tree);
  public function render(MenuTreeItem[] $tree);
  //public function buildPageData($menu_name, $max_depth = NULL);
  //public function buildAllData($menu_name, $id = NULL, $max_depth = NULL);
  //public function buildTree($menu_name, array $parameters = array());
  public function build($menu_name, MenuTreeParameters $parameters);
  public function transform(MenuTreeItem[] $tree, array $manipulators);
  public function buildSubtree($id, $max_relative_depth = NULL);
  // @todo Reconsider later, after the above changes have been made.
  public function getChildLinks($id, $max_relative_depth = NULL);
  //public function resetStaticCache();

}

interface MenuActiveTrailInterface {
 
  public function getActiveTrailIds($menu_name);
  public function getActiveMenuNames();
  public function setActiveMenuNames(array $menu_names);
  public function menuLinkGetPreferred($route_name = NULL, array $route_parameters = array(), $selected_menu = NULL);
 
}

interface MenuParentFormSelectorInterface {

  //public function getParentSelectOptions($id = '', array $menus = NULL);
  public function parentSelectElement($menu_parent, $id = '', array $menus = NULL);
  //public function getMenuOptions(array $menu_names = NULL);
  //public function getParentDepthLimit($id);
 
}

Those changes amount to:

  1. Introducing a value object to represent nodes (in the CS sense, not the Drupal sense) in the menu tree; I called them MenuTreeItem, on par with existing terminology in HEAD. The name and exact properties may change, but the concept is needed.
  2. Making MenuLinkTree as focused as MenuTree in my patch. Notes:
    1. The MenuLinkTree::maxDepth() method is the successor for the MENU_MAX_DEPTH constant. We'd both like to get rid of it, but can't think of a better place for now. It's minor enough that it doesn't matter in the grand scheme of things, so barring a sudden insight, this is acceptable.
    2. Just like in my patch, buildPageData(), buildAllData() and buildTree() are removed in favor of a single build() method, which accepts parameters.
    3. We introduce the "tree manipulators" concept from my patch. DefaultMenuTreeManipulators can almost be copy/pasted verbatim from my patch, including test coverage.
    4. We introduce a MenuTreeParameters value object (just like in my patch), but pwolanin preferred to have the tree manipulators to not be defined as part of the MenuTreeParameters. This is a distinction that is also fine by me: MenuTreeParameters is then solely for determining which menu links to load, and the tree manipulators are applied as part of a new transform() step that accepts the tree manipulators.
    5. pwolanin strongly felt that buildSubtree() was necessary/valuable (to get the tree below a given parent item). This will just be an alias:buildSubtree($menu_name, $parent) === build($menu_name, $parameters = array('parent' => $parent)) (pseudocode).
    6. To neither of us, it was clear whether getChildLinks() belonged in the interface/was worth keeping around, but we figured that'd become clearer once all of the above had been done.
    7. The resetStaticCache() method was already planned to be removed, and will be.
  3. Splitting off the methods relating to the active trail into MenuActiveTrail(Interface). This is like in my patch. But unlike in my patch, pwolanin has already done the work to get rid of the procedural menu.inc/menu_link_get_preferred() (YAY!) — though we probably want a better name for the method now :)
  4. Moving the methods relating to presenting a menu tree in some shape in a form (to select a parent menu item, primarily) into MenuParentFormSelectorInterface. We both would like to remove this, but since we want modules to (finally) be able to override this form item cleanly (e.g. to use something that scales better, like Hierarchical Select), we need this to be a service.
    All other methods should go away though, but that may only be addressed in a follow-up issue.

@pwolanin: if I misrepresented anything, ping me, and I'll correct it.

effulgentsia’s picture

Priority:Major» Critical
Status:Needs review» Needs work
Issue tags:-beta target+beta blocker

AFAICT, there are no fundamental objections or concerns left with the refactor of the MenuLink non-content non-config entity type into a plugin type, thereby allowing static links, content links, and Views links, to each make independent choices on how/where to store their data. The implementation in #71 has already reached a state where there are no critical feature, UX (see #45), or performance (see #73) regressions. Meanwhile, it solves a critical regression, #2265847: [PP-0.5] Regression from D7: default titles of customized menu links aren't translated, and also adds the fantastic feature of custom links being translatable out of the box, which while per #2256497-28: [meta] Menu Links - New Plan for the Homestretch, isn't a critical requirement for releasing D8, is still a fantastic thing to get in if we can.

However, as part of implementing that refactor, the menu_link module got removed (as part of the MenuLink entity type getting removed), so its MenuTree class and a bunch of related code from menu.inc needed to move into a new \Drupal\Core\Menu\MenuLinkTree class, and along the way some of the implementation needed to get cleaned up to adjust for links as plugins rather than links as entities. Because of that, the overlap with Wim's parallel cleanup of that same code in #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees is substantial.

Therefore, I think the approach in #75, and merging that into this issue, makes sense. However, that makes this a blocker for #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees, so raising this to critical. And, since there's substantial data model changes in this patch, that also makes this a beta blocker. Given #2256497-28: [meta] Menu Links - New Plan for the Homestretch, I would have been against this combining and the resulting escalation if #71 wasn't already so far along. But since it is that far along and doesn't have any known critical problems left to solve (it could still use some docs and method name cleanup though), I think this is ok, especially since incorporating #75 gives us the benefit of Wim's prior work in the other issue in figuring out those docs and method names.

Looking forward to seeing the next patch with #75 implemented! Until then, this is "needs work".

larowlan’s picture

Put me down for a review once you have merged approaches.

Gábor Hojtsy’s picture

Yay, yay, yaaaaaaaay! Also: Yay! And: yay!

pwolanin’s picture

A small note - all 4 methods on MenuParentFormSelectorInterface will be in the patch for this issue. We should just push to a follow-up the removal of getParentSelectOptions() since it requires re-doing various ajax form calls on the node type form.

Also, I think it may make more sense to pass a single manipulator instance in like this:

public function transform(MenuTreeItem[] $tree, MenuTreeManipulatorInterface $manipulator);

If the interface has a fixed set of methods we call (or even just 1 method) it would seem you could mix and match via subclassing or composing them.

-Peter

pwolanin’s picture

Actually, I'm not even sure why we need transform() to be on the MenuLinkTree class at all? The manipulator can just be called directly, or maybe pass that into the render call?

A lot of what the manipulators were doing in the prior patch is outdated since we are using Url objects and the plugin instances know how to localize/translate themselves.

pwolanin’s picture

Discussing with EclipseGC in IRC. There is no particular need to define value objects in general if associative arrays are already in core. Instead, he suggests simply omitting array type-hinting so that it's possible to convert to objects later if appropriate: See: #2170775: Remove array typehint from $plugin_definition

So, I'm quite sure we should not bother with MenuTreeItem, and I'm unclear in terms of the parameters array/object whether there is going to be any value.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.27 KB
new557.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,317 pass(es).
[ View ]

So, I think I also missed considering the implication of moving the access check external to the menu tree class:
This would mean we could re-merge the plugin manager and menu tree since it was the requirement for the access manager that lead to the circular dependency. I think that would be ok, since we are pulling a lot of the code to other services.

I also think we can remove the getChildLinks() method since it's only used one place and you can actually get the same thing from buildSubtree() with a little extra work.

This patch is a re-roll for minor HEAD conflicts. it also removes some dead code and fixes a (new?) test.

pwolanin’s picture

StatusFileSize
new33.81 KB
new559.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,302 pass(es).
[ View ]

1st step - create interface MenuParentFormSelectorInterface and class MenuParentFormSelector move all the parent selector code into a new service and re-factors consumers to use that service.

Also, cuts that new interface down to just 2 methods, one of which is marked deprecated.

pwolanin’s picture

StatusFileSize
new31.01 KB
new564.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,254 pass(es).
[ View ]

This patch adds MenuActiveTrail and MenuActiveTrailInterface and removes the related methods from MenuLinkTreeInterface

Also changes the signature of MenuLinkManagerInterface::loadLinksByRoute(), since it was never used to load hidden links, but by allowing restriction to a menu we can possibly use it in SystemManager (and any similar place) instead of menuLinkGetPreferred()

pwolanin’s picture

So, here's what I'd see as the next incremental work:

  1. Remove all the static and other caching from MenuLinkTree and (I expect) clean up the constructor for now unused dependencies.
  2. add methods like:
    pageDataParameters()
    allDataParameters()

    to MenuLinkTreeInterface and MenuLinkTree. These would return the parameters array corresponding to what's internally generated now. I think it is a must to have helper function for this to avoid repeating the same code in several places to build those parameters.

  3. Remove methods
    buildPageData()
    buildAllData()

    From MenuLinkTree by using the new Parameters() methods above instead and then directly calling buildTree()

Wim Leers’s picture

StatusFileSize
new568.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,247 pass(es).
[ View ]
new23.81 KB

#83: looks good.
#84 & #85: look good, except missing docblocks.

I did #86, and cleaned up remnants from #84 and #85.

effulgentsia’s picture

Status:Needs review» Needs work

According to pwolanin, #87 isn't the complete merge yet, so back to needs work.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new9.82 KB
new564.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,248 pass(es).
[ View ]

Yes, not sure how best to mark it "needs review" for tests, but not ready for human review. This is NR for tests only!

Here's a patch that starts adding a DefaultMenuTreeManipulators service, though it's not used yet, and refactors some of the other code to make it easier to move the access checks, etc out to the external manipulators.

Gábor Hojtsy’s picture

Huge +1 to this in terms of unifying the how dynamic menu items are defined. As per #2291773: Creating dynamic hook_menu() successor items use different APIs menu items are now wildly different than any other hook_menu() successor (see views implementing an alter hook pre this patch to implement all its dynamic menu items). With this patch, menu items are brought in line with the other links (local tasks, local actions, contextual links) in terms of how dynamic items are defined.

Wim Leers’s picture

StatusFileSize
new583.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,275 pass(es).
[ View ]
new49.33 KB

Note that the #89 interdiff excludes some of the commits that I'd pushed to the sandbox after I posted #87. Just some small changes there, but still (reversely chronological):

  • Removed the unused MenuLinkTree::defaults.
  • s/buildTree/build/, s/buildRenderTree/render/
  • Actually remove buildPageData().

In this reroll (reversely chronological):

  • Cleaner way of getting at the actual subtree; removed one last direct call to DefaultMenuLinkTreeParameters.
  • Inject MenuLinkTree into SystemController rather than using \Drupal::menuTree().
  • Convert toolbar_menu_navigation_links() to a menu tree manipulator.
  • Remove the calls to MenuLinkTree::transform() in MenuLinkTree::build() and MenuLinkTree::buildSubTree(). Move the responsibility to the caller of ::build(SubTree()) instead.
  • Introduce MenuLinkTree::transform().
  • Ported the DefaultMenuLinkTreeManipulators test coverage; fixed a bug thanks to it.
  • DefaultMenuLinkTreeManipulators doesn't use the route provider service.
  • s/DefaultMenuTreeManipulators/DefaultMenuLinkTreeManipulators/
  • Step 2 in actually using DefaultMenuTreeManipulators: make buildSubtree() build a tree, not a tree node, to allow tree manipulators to be applied; removed access checking and tree sorting from MenuLinkTree in favor of the manipulators that perform those tasks.
  • Step 1 in actually using DefaultMenuTreeManipulators: remove getChildLinks(), convert to buildSubTree().
  • Simplify the checkAccess() tree manipulator.

@pwolanin: I encountered

class MyAccountMenuLink extends MenuLinkDefault {

  /**
   * {@inheritdoc}
   */
  public function isHidden() {
    // The path 'user' must be accessible for anonymous users, but only visible
    // for authenticated users. Authenticated users should see "My account", but
    // anonymous users should not see it at all.
    return (bool) \Drupal::currentUser()->isAnonymous();
  }

}

This is fundamentally incompatible with menu tree render caching, because by doing this, you're creating two mechanisms for dynamically determining the visibility of a menu link for a given user: through access checking and by overriding this method. This ability must be removed completely in favor of access checking only. "hidden" should only be a flag that is configurable, and hence does not depend on the request context.
The same goes for the isExpanded() method.


@pwolanin: The data-drupal-link-systempath attribute does not have a value in the primary and secondary menus, hence the default "Home" link doesn't get marked as active when you're on the front page.


@pwolanin: Because menu links used to be only entities, and the MenuLink entity extended the base Entity, it was easy to have comprehensive test coverage for all cache tag invalidation situations. I'm not seeing anything that resembles the previous MenuLink::postSave(). This needs to be rectified.
Also, I see Cache::invalidateTags(array('menu' => $affected_menus)); — please don't do this, you must load the Menu config entity and call its getCacheTag() method.


For me, the next step is to get all rendered menus, including the primary and secondary menus, to be rendered using the same template. Only then will I be able to convert the custom tree manipulation and rendering into standard tree manipulators and the standardized rendering flow.


@pwolanin, @effulgentsia: As another next step, I'd like to see us evaluate the feasibility of removing buildSubTree().

One tricky aspect about buildSubTree() is that it includes the root (the menu link whose subtree is built). This is unlike the behavior of build(). It used to be worse still: would get a different data structure back from buildSubTree() than you did from build(), but I fixed that. All places where buildSubTree() is used, that root is actively ignored.

Hence we might as well make this change:

  • buildSubTree($menu_name, $id, 1) is equivalent to build($menu_name, $parameters = array('expanded' => $id)). In fact, this is what SystemManager::getAdminBlock() has been doing all along.
  • buildSubTree($menu_name, $id, N) (with N>1) is equivalent to build($menu_name, $parameters = array('expanded' => $id, 'max_depth' => depth_of($id) + N)).

I think most of the perceived complexity lies in the parameters being difficult to understand.

If we decide against this, then at the very least I'd like to see us unify both methods into a single one. build() is for getting the entire tree, i.e. with root = NULL. buildSubTree() is for getting a subtree, i.e. with root = <some ID>. Having both methods creates unnecessary confusion. Let us at least move towards build($menu_name, $parameters = array(), $root = NULL). No confusion is possible anymore then.


@pwolanin, @effulgentsia: pwolanin has expressed some reservations about porting the value object that I had in my patch to represent nodes in the tree (MenuTreeItem); reasons being: not enough wins, except for better documentation. Well, I say that on top of that, we'd get better DX because IDE code completion, and much more understandable code. If you see something like this:

$this->assertEquals($this->links[4]->getPluginId(), $tree['50000 bar test.example2']['below']['50000 baz test.example3']['below']['50000 qux test.example4']['link']->getPluginId());

Who wouldn't run away screaming?

With a value object, that'd look like this:

$this->assertEquals($this->links[4]->getPluginId(), $tree['50000 bar test.example2']->children['50000 baz test.example3']->children['50000 qux test.example4']->link->getPluginId());

It's much clearer what's going on there; it no longer is an incredibly nested array of doom. And when developers *do* make a mistake (e.g. when writing their own menu tree manipulator), then they'll get explicit failures rather than something crashing somewhere deep down in the menu tree handling code because you used a wrong array key.

pwolanin’s picture

@Wim Leers:
re: the account link. This was directly ported from HEAD:

<?php
/**
 * Implements hook_translated_menu_link_alter().
 */
function user_translated_menu_link_alter(MenuLink &$menu_link) {
 
// Hide the "User account" link for anonymous users.
 
if ($menu_link->machine_name == 'user.page' && \Drupal::currentUser()->isAnonymous()) {
   
$menu_link->hidden = 1;
  }
}
?>

I think we need a way to flag links that are dynamically hidden, or have dynamic text, or a dynamic query string so they can be post-processed also. Otherwise, it's a big regression in terms of what you could do before with hook_translated_menu_link_alter(). This dynamic show/hide cannot be accomplished with an access check, since the route is accessible in both cases.

So, I think we'll need to add a new method to the menu link class like isCacheable() that indicates the rendering must be done dynamically.

I agree allowing hidden to be dynamic is particularly yucky though. In this particular case, we could instead implement the route parameters as dynamic to link to the current user profile, which should never be accessible for uid 0.

So, we could basically define the content of the Url object as dynamic (route, route parameters, title, description) if isCacheable() is FALSE


re: getCacheTag()

The menu names in the tree are actually arbitrary - the API doesn't force you to match them to any menu entity, and the menu entity is just donating this string key which we use in the link data.

So, I think the code as is is basically correct, and loading a menu entity is not even guaranteed to be possible.


re: buildSubTree() there is currently no easy way to get depth_of($id), so it's quite difficult to implement via parameters if relative depth > 1. I think having this simple method is preferable to trying to force everything through build() with parameters. I'm not sure why having the single build() method is important?
re: value objects. Part of my push back on value object is from discussions with a variety of people in IRC to understand where we are trying to go in Drupal 8 when using them. From what I've understood - using public properties is not the norm and should not used for new code being added to core. So, if you are using methods it looks more like:
$tree['50000 bar test.example2']->children()['50000 baz test.example3']->children()['50000 qux test.example4']->link()->getPluginId()

And then you also have to define setters so you can change the children of the tree element, etc. I'm not fundamentally opposed to it, but it seems like a lot of work that's not necessary to get the patch done, plus some performance overhead, and the hugely nested calls seem very test-specific, not something you'd see in practice.

pwolanin’s picture

StatusFileSize
new23.59 KB
new577.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,255 pass(es).
[ View ]

Per discussion w/ effulgentsia adds a MenuLinkInterface::isCacheable() method so we can start to account for links that formerly would be passed to hook_translated_menu_link_alter(). Also added a @todo to MyAccountMenuLink fix it since hidden should NOT be dynamic, but we can make this into a access check if the route parameters are dynamic.

Does some small clean-up in the MenuLinkTree and MenuTreeStorage so we can further consolidate the code in the 2 build methods.

makes a fix to transform to use is_callable() and to try to use the more efficient call_user_func(). It should be possible to pass in a lambda or function name.

changes MenuLinkTreeInterface::build() back to MenuLinkTreeInterface::buildTree(). Reasoning: almost every use of build() in core returns a render array, but this does not.

Also, fixed the naming of DefautlMenuLinkTreeManipulatorsTest.php to DefaultMenuLinkTreeManipulatorsTest.php and updated it to use a new MockMenuLink class so we can re-use that in other unit tests.

Do we want to rip out MenuLinkInterface::build()?

pwolanin’s picture

StatusFileSize
new5.39 KB
new579.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,569 pass(es).
[ View ]

Quick further update. Adds a new test case starting to use the mock menu link class in MenuLinkTreeTest. Also fixes a bug in MenuTreeStorage::treeDataRecursive() that was for some reason still populating the p1...p9 values into the tree element array. No idea how that snuck back in there, but it's someplace in all the squashed commits.

Also simplifies the flatten() code and removes the dangling child trees.

Wim Leers’s picture

#92:

RE: account link.
Interesting that this was directly ported! I forgot about that hook in user.module.
Sadly, this is right: This dynamic show/hide cannot be accomplished with an access check, since the route is accessible in both cases. :(
If I understand this correctly:

I agree allowing hidden to be dynamic is particularly yucky though. In this particular case, we could instead implement the route parameters as dynamic to link to the current user profile, which should never be accessible for uid 0.

Then you're saying that the user.page route needs a route parameter like "show_profile". Then we can distinguish between /user linking to the current user's profile and to the login page. And then the menu link can specifically refer to the profile, in which case we can deny access for anonymous users.
But if we do all that, then why will isCacheable() still be needed?

Very interesting thinking! I don't fully grok it yet though :) Hopefully this will help us solve this.

RE: data-drupal-link-systempath
You didn't answer this, I think?
RE: cache tags
But surely there must be a way of knowing if a menu link is in a certain menu, right? MenuLinkInterface::getMenuName() seems to confirm that. Well, then this should use something like Menu::load($link->getMenuName())->getCacheTag(). If no menu (Menu config entity) is associated, then no cache tag needs to be invalidated.
You didn't address the major gap in test coverage that this patch introduces.
RE: buildSubTree()
Only having build() is important because there's then only a single way to build a menu tree. People don't have to choose, nor do they have to learn a whole lot about how our tree structure implementation details work. Even if internally build() would call either MenuLinkTreeStorage::loadTreeData() or MenuLinkTreeStorage::loadSubTreeData(), that would still be a big improvement, because the main touch point is MenuLinkTree — rarely will they need to talk to MenuLinkTreeStorage directly.
RE: value objects
It's not essential, but it would help significantly with debuggability/maintainability, and it makes it much clearer what you have to work with when you write a menu tree manipulator.
It doesn't matter that "using public properties is not the norm". A value object with methods plus protected properties are better than a value object with public properties, but that's still much, much better than an unstructured array. It's only a lot of work if you don't want public properties. A value object with public properties is the middle ground with the most bang for the buck.

#93:

MenuLinkInterface::isCacheable()
MenuLinkInterface::isCacheable() is unfortunately problematic; because we don't know which context the cacheability depends upon; i.e. by what it is varied. In order for this to work, we'll also need that information. In the example we've been discussing, it's cacheable per role, for example. If we have that information, then we can cache it again, per role, instead of having to render this dynamically on every page load.
Hopefully though, we can just remove MenuLinkInterface (as per the above), in which case this can be ignored :)
RE: using is_callable()
I copied this straight from #2247779-12: Allow #pre_render, #post_render and #post_render_cache callbacks to be service methods, where dawehner said it had to be done this way. Please revert or update the other location in core where exactly this code is being used.
RE: using "the more efficient" call_user_func()
This seems like an extremely premature optimization to me… but ok.
RE: buildTree()
Well, by that reasoning, render() should be called build(), shouldn't it? Then build(Sub)Tree() should be renamed to something else. I can't thing of any good names at the moment. construct() is the best I can think of right now.
Actually, I do have a concrete proposal:
  1. load() (instead of the first half of what the current build(Sub)Tree() does) would just load the needed links from tree storage
  2. transform() (instead of the second half of what the current build(Sub)Tree() does and what the current transform() does) would transform the links into a tree and apply any manipulators to it
  3. build() (instead of the current render()) would turn the tree into a render array

What do you think?

RE: removing MenuLinkInterface::build()
Oh, hah, I hadn't even seen that one! I'd say that we indeed want to remove that. There's only one use of that in all of core. It's conceptually simpler to have menu links only be value objects, instead of them being able to render themselves.
RE: cleanup
Small cleanup in MenuLinkTree and MenuTreeStorage: looks good.
MenuLinkMock: awesome, thank you! :)
Apparently, the documentation of the return value for build(Sub)Tree() (i.e. both methods) was incomplete (which in my patch used to be MenuTreeItemInterface[], it's now been updated to read:
   * @return array
   *   An array of menu links, in the order they should be rendered. The array
   *   is a list of associative arrays -- these have several keys:
   *   - link: the menu link plugin instance
   *   - below: the subtree below the link, or empty array. It has the same
   *            structure as the top level array.
   *   - depth: int. the depth of this link below he root of the tree.
   *   - has_children: boolean. even if the below value may be empty the link
   *                   may have children in the tree that are not shown. This
   *                   is a hint for adding appropriate classes for theming.
   *   - in_active_trail: boolean
   *   - access: NULL.
   */

This just helps make my case that I made above: public properties are still a step forward compared to the inherent lack of structure in arrays.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
@@ -288,6 +288,7 @@ protected function createInstances(&$tree) {
+      $tree[$key]['access'] = NULL;

And another symptom of the problematicness of choosing arrays over value objects. This should not be necessary.


#94:

RE: New test case MenuLinkTreeTest::testCreateLinksInMenu()
Great! But I find it hard to tell what exactly it's testing. Could you add some docs to explain that?
Also: does this mean we can convert MenuLinkTreeTest to a unit test rather than a KernelTestBase test?
Finally: why duplicate DefaultMenuLinkTreeManipulators::flatten() in this test? You might as well make that a static method, so you can call it from here without having to instantiate DefaultMenuLinkTreeManipulators. Furthermore, the only reason you flatten the tree, is to count the total number of nodes. This is yet another reason to have value objects for the nodes in the tree; I had a ::count() method on mine, which removes the need to flatten it just for counting purposes.
RE: simplified flatten()
Wow, very nice simplification! *Excellent* catch :)

Now working on what I said I'd do next in #91.

pwolanin’s picture

re: My Account, I was proposing something simpler even: link to the user.page route, and maybe the route parameter dynamic with the current user uid. This will fail the access check for uid 0.

So, isCachable() is still needed since the rendered URL will be different for every user.

I general, I think we can define isCacheable() as TRUE to mean that it works with the render caching's contexts.


re: is_callable I sent dawehner a note asking for clarification, or I can trying to catch larowlan later.
RE: data-drupal-link-systempath I didn't have a chance to look into it and continued use of system path makes me a bit sad.

That seems like a regression in core?


re: cache tags. I still disagree. The menu_name string on the links is the right cache tag. Menu entities don't have anything to do with it.

The central methods are in MenuLinkManager, and I think we should eventually move the cache invalidates back there.

There is some test coverage in MenuCacheTagsTest, but I'm not sure what you were adding before.


Re: method naming, yes, the final one should not be called render() but rather buildRenderArray() or something more clear.

Let's not re-organized the rest. I really even find the transform() method as is to be a little strange, and I'd rather not expose more of the guts to the developer.

I guess I just basically disagree about the ease of having a very clear method buildSubtree that does what it's called vs. having to grok the docs if you need that functionality.

So, I would prefer we just leave it basically as it is except renaming the last method.


re: value objects, if you feel like doing that work inside the MenuLinkTree code to create the object, I don't really care, but I fell like it's still going to slow things down.

I guess you could just throw it into MenuLinkTree::createInstances() and toughly do:

$new_tree[$key] = new MenuLinkElement($tree[$key]);

In IRC chx suggested "element" was at least better than "item" since "node" would be most correct but confusing, and "vertex" unfamiliar.


re: MenuLinkTreeTest, no it' can't be a unit test, since the storage and entities use the database. However, it's still very fast to run as is.
Wim Leers’s picture

StatusFileSize
new599.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,586 pass(es).
[ View ]
new16.16 KB

Finished what I said in #91:

For me, the next step is to get all rendered menus, including the primary and secondary menus, to be rendered using the same template. Only then will I be able to convert the custom tree manipulation and rendering into standard tree manipulators and the standardized rendering flow.

And with that, the porting of DefaultMenuTreeManipulators in my patch to DefaultMenuLinkTreeManipulators in this patch has been completed — including test coverage. Several of the manipulators I had are made obsolete by the MenuLinkTreeStorage abstraction that this patch introduces.

dcrocks’s picture

Just some history. hook_translated_menu_link_alter was deleted from D8 a long time ago in favor of hook_menu_link_load. It turned out the latter wasn't being called at the right time to prevent the account menu being made visible to anonymous users, so hook_translated_menu_link_alter was reinstated(#1912806: Regression: 'User Account' displayed on front page for anonymous users). It looks now that both will be gone. Assuming the functionality survives, the only possible impact might be on contrib modules(Devel?). But I think this change will cause them all to be redone in any case.

Wim Leers’s picture

StatusFileSize
new603.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256521-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new24.74 KB

Changes since #97, again in reverse chronological order:

  • Fix bug in HEAD/this patch: SystemMenuBlock is not actually cacheable, since access checks are not yet cacheable. This is being restored and fixed in https://drupal.org/node/1805054.
  • Render the toolbar's administration tray menu in a #pre_render callback; this paves the way for render caching it.
  • Add MenuActiveTrailInterface::getActiveTrailCacheKey(). This improves the DX when rendering menus.
  • Added full unit test coverage to MenuActiveTrail; inspired on Wim's old unit tests.
  • Refactor MenuActiveTrail(Interface) to be *significantly* simpler. This was broken in all previous patches. Discussed with pwolanin, we decided to get rid of all the cruft. Now caching per active trail can work again :)

Next steps:

  1. port MenuTreeParameters (but call it MenuLinkTreeParameters instead)
  2. port MenuTreeItem (but call it MenuTreeElement instead)
  3. last bits (compare what my patch did in terms of building menu trees with what this patch does — but it's mostly in line already, so I expect this to be pretty swift

I think it'll make more sense to defer the "render into placeholder" stuff to #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees. It's not essential to this patch and might spark bikeshedding, which we better avoid here.

I expect to complete the combining of my patch with this patch tomorrow. I'll answer to #96 after I finish the above. I need to get it done first, because of an upcoming vacation.

Wim Leers’s picture

#2292025: Convert BreadcrumbBuilderInterface applies() and build() to use RouteMatch changed hunks in both core/core.services.yml and core/modules/menu_link/menu_link.module, hence the patch won't apply. We'll have to rebase the sandbox. For now, I'll just continue. I've done plenty of rebasing, but I don't know what the best practices are around rebasing in a sandbox.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new611.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,319 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new56.62 KB

Talked to pwolanin how he prefers rebasing to be done in this sandbox. Now rebased.


Changes since #99, again in reverse chronological order:

  • Make MenuTreeStorage *also* use MenuLinkTreeElement; just allow MenuLinkTreeElement::link to be *either* the link definition *or* the link instance.
  • Introduce MenuLinkTreeElement; rename "$item" to "$element" everywhere; cleans up typical steps after calling buildSubTree(); renamed collectRoutes() to collectRoutesAndDefinitions() to actually capture what it does.
  • patch for issue #2293163 needed to get tests to pass here

I wanted to introduce the MenuLinkTreeElement value object. At first, I had a lot of trouble doing so, because all the object model changes are effectively undocumented. After a lot of trial and error and trying to put things together, it started to make sense. Here's a high-level explanation that pwolanin will hopefully confirm is mostly correct, indicate what's wrong, and turn into actual documentation so that reviewers don't have to read all the code to understand the architecture and assumptions.
Instead of a menu link knowing where it lives, a menu link is now a separate thing that only knows its "own" (immutable) properties (like title, route_name, menu_name, and strangely also parent), but not those properties that are determined by the (mutable) placement in the tree (like depth and has_children).
Consequently, we deal with 3 different representations of a link (@pwolanin, feel free to change all terms):

  1. the menu link tree definition: essentially a row from {menu_tree}
  2. 1. can be transformed into a link definition (by MenuTreeStorage::prepareLink()), but then loses the tree-related metadata, like depth and has_children
  3. 2. can be transformed into a link instance (by MenuLinkManager::createInstance())

A built menu link tree eventually needs to deal with link instances. But then all the tree-related metadata is lost. This is the purpose of MenuLinkTreeElement (and previously the unstructured array I was unhappy with):

  1. to hold a reference to a menu link instance
  2. to store tree-related metadata (depth and has_children)
  3. to store tree building-related metadata (inActiveTrail, access and options)

I've also documented this on MenuLinkTreeElement's docblock:

/**
* Provides a value object to model an element in a menu link tree.
*
* \Drupal\Core\Menu\MenuLinkTreeInterface objects represent a menu link's data.
* Objects of this class provide complimentary data: the placement in a tree.
* Therefore, we can summarize this split as follows:
* - Menu link objects contain all information about an individual menu link,
*   plus what their parent is. But they don't know where exactly in a menu link
*   tree they live.
* - Instances of this class are complimentary to those objects, they know:
*   1. all additional metadata from {menu_tree}, which contains "materialized"
*      metadata about a menu link tree, such as whether a link in the tree has
*      visible children and the depth relative to the root;
*   2. plus all additional metadata that's adjusted for the current tree query,
*      such as whether the link is in the active trail, whether the link is
*      accessible for the current user, and the link's children (which are only
*      loaded if the link was marked as "expanded" by the query).
*
* @see \Drupal\Core\Menu\MenuTreeStorage::loadTreeData()
*/

[…] if you feel like doing that work inside the MenuLinkTree code to create the object, I don't really care, but I fell like it's still going to slow things down.

Here's a simple microbenchmark with 1000 "menu links" to prove it: http://3v4l.org/XcfvE, see http://3v4l.org/XcfvE#v5411 or http://3v4l.org/XcfvE#v5512 — CPU time is identical (you probably want bigger numbers, even though 1000 is already much more than almost any menu) and memory usage of arrays is *double* that of objects.

Some links to explain that:

So, in this patch, I introduced MenuLinkTreeElement, which can theoretically reduce memory usage while staying equally fast. However, because MenuTreeStorage assumes it's working with "menu tree link definitions" and "menu link definitions" (see earlier), and we probably don't want to instantiate menu link objects from within MenuTreeStorage (if we'd do that, we'd cause a recursive service dependency, so…), I decided to allow MenuLinkTreeElement::link to be of the type \Drupal\Core\Menu\MenuLinkInterface|array. Hence it can be either a menu link definition or a menu link instance.

I didn't do any profiling of the actual code, because 1) the above shows it shouldn't matter, 2) once we have render caching of menus, performance considerations in building of menu trees will be pretty much irrelevant, 3) \Drupal\menu_ui\Tests\MenuTest takes the same amount of time as before, and that arguably is the best benchmark we have.

I also found a fun bug in the Memory cache backend: #2293163: MemoryBackend's cache entries may be modified unintentionally thanks to object references. I had to fix that to get tests to pass.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new611.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,468 pass(es).
[ View ]
new852 bytes

Fixing the sole test failure in #102.

Wim Leers’s picture

StatusFileSize
new621.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256521-105.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new66.87 KB

The steps I went through for this reroll (in chronological order):

  1. I didn't change the signature of TreeStorage at all, but I showed that TreeStorage::loadSubTreeData() and TreeStorage::loadAllChildLinks() really are just special cases of the same generic loading logic. In both cases, it's just about choosing a different root than the "real" root (which in this patch has the id '' (i.e. empty string), and in HEAD used to have the id 0).
  2. Consequently, I was also able to make buildSubTree() just a special case of buildTree().
  3. Hence it became more clear to understand what is happening when replacing buildSubTree() calls with a custom set of parameters being passed into buildTree().
  4. This made buildSubTree() obsolete, so I removed it. Also because it hardcoded the "exclude hidden links" condition, and therefore it was less expressive than buildTree(), leaving it with barely any remaining purpose.
  5. As per the above discussion, I renamed buildTree() not to build() but to load(). "Loading a tree" makes sense, and that's actually already what TreeStorage calls it: TreeStorage::loadTreeData().
  6. Also per the above discussion, I renamed render() to load().
  7. Note that we now have a very clear sequence of steps: 1) load(), 2) transform(), 3) build().
  8. I removed the only_active_trail parameter because it was only around for legacy reasons (see below for more info).
  9. All of the above cleared the path for a MenuLinkTreeParameters value object. Only this time, it's much clearer (than in my patch). There are very clear helper methods for the typical use cases, to construct the parameters fluent-style.
  10. Now that we had a MenuLinkTreeParameters object with a solid DX, it was time to look at build(All|Page)DataTreeParameters(). Turns out one of them had a chunk of dead code that was used by nothing at all in core (all along in this patch!), leaving only a "max depth" setting that can now be set elegantly and simply enough without needing this additional method contaminating the interface. buildPageDataTreeParameters() on the other hand, does do some genuinely useful things, that are indeed relatively complex and hence we might want to shield the user from it. So I kept everything (except the setting of max depth, which, again, can be set elegantly and simply enough now) and renamed it to the much clearer getDefaultRenderedMenuTreeLinkParameters().
  11. Finally: a round of writing solid documentation (ported from my patch, but of course adjusted and cleaned up), plus some clean-up.

And with that, the combining of #2289033: Refactor MenuTree to be simpler and have full unit test coverage with this patch is complete! I intentionally left out the changes to rendering menus, because they can be added in #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees, and can undergo more scrutiny there, and are non-essential to this patch.

The essential parts that have been added by my work are 1) the concept of menu tree manipulators, 2) much better DX in the form of a clean/simple/understandable MenuLinkTreeInterface, value objects instead of unstructured arrays and guiding documentation.


Changes since #104, again in reverse chronological order:

  • Massively improve the documentation, so that developers will actually be able to find their way around rendering menus.
  • Remove two unused services from MenuLinkTree.
  • Reorder the methods in MenuLinkTree(Interface) to be more logical: first getDefaultRenderedMenuTreeLinkParameters(), then load(), transform() and build(), and finally the oddballs maxDepth() and getSubtreeHeight().
  • buildPageDataTreeParameters() was a misnomer, even if only for the fact that it reacts to the active trail, not the page. I've renamed it to getDefaultRenderedMenuTreeLinkParameters() plus clear documentation, that together should make the purpose of this
  • buildAllDataTreeParameters() was only called by the menu UI form builders. On top of that, 90% of the logic in buildAllDataTreeParameters() was never used by core. Remove buildAllDataTreeParameters() and replace the callers with something much simpler.
  • Convert everything to use the new MenuLinkTreeParameters value object instead of an unstructured array.
  • Introduce the MenuLinkTreeParameters value object, including unit test coverage.
  • Remove the 'only_active_trail' parameter, which was added in Drupal 7 for internal use only, and was used for building breadcrumbs. Since breadcrumbs no longer are built using this, we can safely remove this.
  • Rename MenuLinkTree::render() to MenuLinkTree::build().
  • Rename MenuLinkTree::buildTree() to MenuLinkTree::load().
  • Remove the now-obsolete MenuLinkTree::buildSubTree().
  • Show that TreeStorage::loadAllChildLinks() also can rely on the same link querying helper function as the one used by TreeStorage::loadTreeData().
  • Bring back caching to TreeStorage::loadTreeData().
  • Now we can remove the calls to MenuLinkTree::buildSubTree() and replace them with calls to MenuLinkTree::buildTree().
  • Show that TreeStorage::loadSubTreeData() is just a special case of TreeStorage::loadTreeData(); let ::loadSubTreeData() call ::loadTreeData().

Background info for the removal of the only_active_trail parameter:
From D7's menu.inc:

* @param $only_active_trail
*   (optional) Whether to only return the links in the active trail (TRUE)
*   instead of all links on every level of the menu link tree (FALSE). Defaults
*   to FALSE. Internally used for breadcrumbs only.
pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new613.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,404 pass(es).
[ View ]

re-rolled for conflict with HEAD

In terms of removing the active_trail_only functionality - I feel like we should retain this somehow in case people want to re-implement menu based breadcrumbs - though I guess we can already just load the active trail IDs?

Wim Leers’s picture

#107: Thanks for the rebase!

Yes, they can just use MenuActiveTrail::getActiveTrailIds() (just like MenuTree::getActiveTrailIds() in HEAD).

I think/fear #2286681: Make public properties on ConfigEntityBase protected might cause this patch to need another rebase.


#96:

RE: My Account
I see :)
Alex and I discussed this further, and I think he had ideas to kind of unify our opposing angles. I think he'll talk to you about that soon, if he hasn't already.
RE: is_callable()
You told me in chat that dawehner basically said that this would be fine. Great :) Simpler, less mysterious code. Better!
RE: data-drupal-link-systempath
How could this possibly be a regression if it went in many months ago, back when system path was still actively used to determine whether a link is active? I agree that we'd like to move away from system path. But that doesn't mean it's a regression in and of itself.
The actual regression is the one that this patch introduces compared to HEAD: the "front page" link is no longer marked as active.
RE: cache tags
If menu entities are completely unrelated, then why do you use the exact same cache tags?

I think we're just misunderstanding each other.

So let me explain the situation. In current HEAD, we have Menu config entities and MenuLink content entities. It doesn't make sense to have a cache tag for each menu link, because a menu link is not an expensive thing that is rendered on its own, but is always rendered (and manipulated) in the context of a Menu. Hence it made sense to discard MenuLink's own cache tags entirely, and just always make them refer to the associated Menu entity's cache tags (i.e. MenuLink::getCacheTag() can be considered a proxy of Menu::getCacheTag(), for the associated menu).

Looking at this patch, that has been lost:
MenuLinkContent get their own cache tags, instead of referring to that of the associated Menu entity. That's an easy fix; you can migrate the code 1:1 from HEAD's MenuLink entity :)
MenuCacheTagsTest — which tested all this — thankfully still exists, but has been changed in ways that result in decreased coverage. Instead of creating a MenuLink entity (MenuLinkContent in this patch), then modifying it and deleting it), it now does those things with a static menu link (MenuLinkDefault), and then just creates a MenuLinkContent entity. So the basics are still there, but now that there are multiple menu link plugin types, we need to apply the original test coverage to menu links of each each menu link plugin types.
That's the most important part.

On top of that, I'm concerned about using "hardcoded" cache tags (in which it's easy to make typos) instead of using Menu::getCacheTag(). Even though it yields correct results.
RE: method naming
I applied this renaming in the patch in #105.
I also took the liberty of removing buildSubTree(), because after the incremental improvements that I made (which you can see in the sandbox' commit history, plus a full explanation of the rationale in #105), there seemed to be little to no value left in keeping it. I hope you're on the same page after reading #105/looking at the incremental steps.
I agree that the transform() method is a little bit strange. But that's only because we pulled it out from build(Tree)() like I had it in my patch. And the main reason we pulled it out there, is because we have also had buildSubTree(). Now that I've finished the combining of both patches in #105, and especially now that we have a MenuLinkTreeParameters value object, I think you might see the appeal to put the menu link tree manipulators in MenuLinkTreeParameters as well, and let load() (previously buildTree()) apply the menu link tree manipulators?
In any case, I do agree that as of #105, ::transform() and the associated DX (an array with menu link tree manipulator callables) is by far the worst remaining part of MenuLinkTree. If you have any idea on how to make the DX better, but still retain the overall concept, I'm totally open to that.
  1. The only idea that I have is to:
  2. Make ::load() automatically call ::transform() to apply the menu.default_tree_manipulators:checkAccess menu link tree manipulator.
  3. Make ::build() automatically call ::transform() to apply the menu.default_tree_manipulators:generateIndexAndSort menu link tree manipulator.
  4. Then the need for a developer to call ::transform() is limited to only the advanced use cases.

For example, this:

  $parameters = new MenuLinkTreeParameters();
  $parameters->setRoot('system.admin')->excludeRoot()->topLevelOnly()->excludeHiddenLinks();
  $tree = $menu_tree->load(NULL, $parameters);
  $manipulators = array(
    array('callable' => 'menu.default_tree_manipulators:checkAccess'),
    array('callable' => 'menu.default_tree_manipulators:generateIndexAndSort'),
    array('callable' => 'toolbar_menu_navigation_links'),
  );
  $tree = $menu_tree->transform($tree, $manipulators);
  $element['administration_menu'] = $menu_tree->build($tree);

could change to:

  $parameters = new MenuLinkTreeParameters();
  $parameters->setRoot('system.admin')->excludeRoot()->topLevelOnly()->excludeHiddenLinks();
  $tree = $menu_tree->load(NULL, $parameters);
  $tree = $menu_tree->transform($tree, 'toolbar_menu_navigation_links');
  $element['administration_menu'] = $menu_tree->build($tree);

And if this weren't an advanced use case (i.e. if no additional menu link tree manipulator had to be applied), it'd have been changed to:

<code>
  $parameters = new MenuLinkTreeParameters();
  $parameters->setRoot('system.admin')->excludeRoot()->topLevelOnly()->excludeHiddenLinks();
  $tree = $menu_tree->load(NULL, $parameters);
  $element['administration_menu'] = $menu_tree->build($tree);

Downsides:

  • We'd need to be able to tell ::load() to apply a different access checking manipulator.
  • Less clarity: instead of 3 distinct phases, the first and third phase also kind of do bits of the second phase. This is also a DX regression. But arguably a smaller one.
  • Less control: the generateIndexAndSort menu link tree manipulator would always be called prior to rendering; making it impossible to render trees in a different order. But in the extremely rare case that this is needed, the developer could just override the MenuLinkTree service and override this aspect, and keep everything else.

Having written this, the downsides don't look too bad. If you like this, I'll get this done.

RE: value objects
I did the work in #102 and #105. And I followed the naming you/chx preferred: I called it "element".

While working on #105, I also found another bug (on top of the broken "active" marking of the front page, due to missing the value for the data-drupal-link-systempath attribute): menu links defined in YML files can be repositioned in the tree just fine. But whenever caches are cleared, the original position is restored!

pwolanin’s picture

@Wim Leers - most of this sounds fine. I need to look more at how you implemented the subtree handling - as long as we can load a tree with relative depth to the selected root, it's not really a big concern.

As far as cache tags, I'll emphasize again, that the menu links API is not based on Menu entities except that we use to make the expected list and human-readable names. But the API doesn't restrict in any way to using menu_name values that correspond to any actual Menu entity. So the correct cache tags need to be the set found in the links in any given menu. This will always be a single, unique one. But again, may bear no relation to any Menu entity, so it's incorrect to involve the Menu entity in finding the cache tags.

pwolanin’s picture

StatusFileSize
new613.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,487 pass(es).
[ View ]

re-rolled patch for HEAD conflict. Going to make some mall enhancements to #2293163: MemoryBackend's cache entries may be modified unintentionally thanks to object references
Assuming that goes in promptly, we can take it out of this patch.

pwolanin’s picture

StatusFileSize
new14.61 KB
new610.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,411 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

re-roll for #2293163: MemoryBackend's cache entries may be modified unintentionally thanks to object references and other HEAD conflicts

then, renamed class MenuLinkTreeElement to MenuTreeElement, since MenuTreeStorage should be generic (e.g. we should try to re-use it for book module) and not specific to menu links.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
new610.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,408 pass(es).
[ View ]
pwolanin’s picture

StatusFileSize
new42.01 KB
new610.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,445 pass(es).
[ View ]

So, after further thought, reverted the class name change in #111. Instead, I moved all use of that class out of the MenuTreeStorage so tree element instances are only created in MenuLinkTree. This keeps the tree storage more generic and maybe a little simpler to re-implement.

In a similar vein, renamed class MenuLinkTreeParameters to MenuTreeParameters and added some @todos about making it have a full set of methods and an interface.

These changes are pretty much internal to the interaction between MenuLinkTree and MenuTreeStorage, so not really important to the big picture for anyone who has been reviewing.

pwolanin’s picture

StatusFileSize
new7.88 KB
new610.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256521-115.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Some more fixes: use RouteMatchInterface in MenuActiveTrail. Also did away with checking for the _exception_statuscode attribute on the request because of it - I think it only makes sense if some information like that is on the RouteMatch

Also fixes SystemManager to actually limit to the admin menu (I confused myself after adding a link to /admin in the main menu the /admin page was blank - clearly a bug), and fixes the @todo about ordering in the query to load by route.

Added some more code comments too.

A new PR for visual review has been created: https://github.com/pwolanin/drupal/pull/2

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new610.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,482 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
larowlan’s picture

Firstly - some coding standards violations for new files:
<3 phpcs and coder.

phpcs --standard=.~/git/coder/coder_sniffer/Drupal/ruleset.xml --extensions=php,module,inc,install,test,profile,theme core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php core/lib/Drupal/Core/Menu/Form/ core/lib/Drupal/Core/Menu/MenuActiveTrail.php core/lib/Drupal/Core/Menu/MenuActiveTrailInterface.php core/lib/Drupal/Core/Menu/MenuLinkBase.php core/lib/Drupal/Core/Menu/MenuLinkDefault.php core/lib/Drupal/Core/Menu/MenuLinkInterface.php core/lib/Drupal/Core/Menu/MenuLinkManager.php core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php core/lib/Drupal/Core/Menu/MenuLinkTree.php core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php core/lib/Drupal/Core/Menu/MenuParentFormSelector.php core/lib/Drupal/Core/Menu/MenuParentFormSelectorInterface.php core/lib/Drupal/Core/Menu/MenuTreeParameters.php core/lib/Drupal/Core/Menu/MenuTreeStorage.php core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php core/lib/Drupal/Core/ParamConverter/MenuLinkPluginConverter.php core/modules/menu_link_content/ core/modules/menu_ui/src/Form/MenuLinkEditForm.php core/modules/menu_ui/src/Plugin/ core/modules/system/config/install/menu_link.static.overrides.yml core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php core/modules/system/templates/menu-tree.html.twig core/modules/system/tests/modules/test_page_test/test_page_test.menu_links.yml core/modules/user/src/Plugin/Menu/ core/modules/views/src/Plugin/Derivative/ViewsMenuLink.php core/modules/views/src/Plugin/Menu/ core/modules/views/views.menu_links.yml core/profiles/standard/standard.menu_links.yml core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php core/tests/Drupal/Tests/Core/Menu/MenuLinkMock.php core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeElementTest.php core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeParametersTest.php

Reveals:

FILE: ...v/vd8/app/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR | Missing file doc comment
113 | ERROR | Inline doc block comments are not allowed; use "// Comment"
     |       | instead
--------------------------------------------------------------------------------

FILE: ...ands/dev/vd8/app/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
68 | WARNING | Line exceeds 80 characters; contains 83 characters
70 | WARNING | Line exceeds 80 characters; contains 83 characters
--------------------------------------------------------------------------------

FILE: ...rs/rowlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
85 | ERROR | Return comment indentation must be 2 additional spaces
--------------------------------------------------------------------------------

FILE: /Users/rowlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuLinkTree.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  65 | WARNING | Line exceeds 80 characters; contains 84 characters
157 | ERROR   | Inline doc block comments are not allowed; use "// Comment"
     |         | instead
--------------------------------------------------------------------------------

FILE: .../rowlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
91 | ERROR | Missing comment for param "$has_children" at position 2
92 | ERROR | Missing comment for param "$depth" at position 3
93 | ERROR | Missing comment for param "$in_active_trail" at position 4
--------------------------------------------------------------------------------

FILE: ...owlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
59 | ERROR | Return comment must be on the next line
--------------------------------------------------------------------------------

FILE: ...wlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  30 | ERROR | Missing function doc comment
  64 | ERROR | Incorrect spacing between default value and equals sign for
     |       | argument "$menus"; expected 1 but found 2
125 | ERROR | Inline doc block comments are not allowed; use "// Comment"
     |       | instead
--------------------------------------------------------------------------------

FILE: ...v/vd8/app/core/lib/Drupal/Core/Menu/MenuParentFormSelectorInterface.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
56 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...s/rowlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
--------------------------------------------------------------------------------
FOUND 9 ERROR(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
  90 | ERROR | Missing comment for @return statement
105 | ERROR | Missing comment for @return statement
118 | ERROR | Missing comment for @return statement
134 | ERROR | Missing comment for @return statement
148 | ERROR | Missing comment for @return statement
172 | ERROR | Missing comment for @return statement
187 | ERROR | Missing comment for @return statement
197 | ERROR | Missing comment for @return statement
212 | ERROR | Missing comment for @return statement
--------------------------------------------------------------------------------

FILE: ...sers/rowlands/dev/vd8/app/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AND 2 WARNING(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
   58 | WARNING | Line exceeds 80 characters; contains 81 characters
  573 | ERROR   | Missing function doc comment
  696 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
  788 | ERROR   | Last parameter comment requires a blank newline after it
1038 | WARNING | Line exceeds 80 characters; contains 82 characters
--------------------------------------------------------------------------------

FILE: ...v/vd8/app/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
222 | ERROR | Inline doc block comments are not allowed; use "// Comment"
     |       | instead
245 | ERROR | Inline doc block comments are not allowed; use "// Comment"
     |       | instead
249 | ERROR | Inline doc block comments are not allowed; use "// Comment"
     |       | instead
--------------------------------------------------------------------------------

FILE: .../core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
52 | ERROR | Missing comment for @return statement
95 | ERROR | Missing comment for @return statement
--------------------------------------------------------------------------------

FILE: ...vd8/app/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 1 WARNING(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
  68 | WARNING | Line exceeds 80 characters; contains 83 characters
302 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
339 | ERROR   | Inline doc block comments are not allowed; use "// Comment"
     |         | instead
--------------------------------------------------------------------------------

FILE: .../app/core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
22 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: ...rowlands/dev/vd8/app/core/modules/menu_ui/src/Form/MenuLinkEditForm.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
30 | WARNING | Line exceeds 80 characters; contains 83 characters
--------------------------------------------------------------------------------

FILE: ...8/app/core/modules/system/config/install/menu_link.static.overrides.yml
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
   |         | allowed by this install of PHP. This file may be using short
   |         | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...nds/dev/vd8/app/core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
--------------------------------------------------------------------------------
FOUND 14 ERROR(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
  41 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
  75 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
  76 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
  77 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
  93 | ERROR | Missing function doc comment
  94 | ERROR | Inline doc block comments are not allowed; use "// Comment"
     |       | instead
108 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
109 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
110 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
111 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
112 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
113 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
114 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
115 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: .../dev/vd8/app/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
--------------------------------------------------------------------------------
FOUND 20 ERROR(S) AND 2 WARNING(S) AFFECTING 18 LINE(S)
--------------------------------------------------------------------------------
136 | ERROR   | Expected 1 space after comma in function call; 2 found
152 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
153 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
154 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
168 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
169 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
171 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
173 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
184 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
199 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
207 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
293 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
295 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
297 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
299 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
301 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
394 | WARNING | Line exceeds 80 characters; contains 115 characters
394 | ERROR   | No space before comment text; expected "// , "Found expected
     |         | number of children for $id");" but found "//, "Found expected
     |         | number of children for $id");"
394 | ERROR   | Comments may not appear after statements.
395 | WARNING | Line exceeds 80 characters; contains 103 characters
395 | ERROR   | No space before comment text; expected "// , 'Child IDs
     |         | match');" but found "//, 'Child IDs match');"
395 | ERROR   | Comments may not appear after statements.
--------------------------------------------------------------------------------

FILE: .../rowlands/dev/vd8/app/core/modules/system/templates/menu-tree.html.twig
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
   |         | allowed by this install of PHP. This file may be using short
   |         | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...dules/system/tests/modules/test_page_test/test_page_test.menu_links.yml
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
   |         | allowed by this install of PHP. This file may be using short
   |         | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: .../dev/vd8/app/core/modules/views/src/Plugin/Derivative/ViewsMenuLink.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
39 | ERROR | Inline doc block comments are not allowed; use "// Comment"
    |       | instead
--------------------------------------------------------------------------------

FILE: ...v/vd8/app/core/modules/views/src/Plugin/Menu/Form/ViewsMenuLinkForm.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
34 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: /Users/rowlands/dev/vd8/app/core/modules/views/views.menu_links.yml
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
   |         | allowed by this install of PHP. This file may be using short
   |         | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...ers/rowlands/dev/vd8/app/core/profiles/standard/standard.menu_links.yml
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
   |         | allowed by this install of PHP. This file may be using short
   |         | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...re/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AND 4 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
  27 | WARNING | Line exceeds 80 characters; contains 84 characters
  34 | WARNING | Line exceeds 80 characters; contains 88 characters
  82 | ERROR   | Function comment short description must be on a single line,
     |         | further text should be a separate paragraph
  82 | ERROR   | Function comment short description must end with a full stop
  96 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  97 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  98 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  99 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
100 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
101 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
102 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
103 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
136 | WARNING | Line exceeds 80 characters; contains 86 characters
138 | WARNING | Line exceeds 80 characters; contains 86 characters
157 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: ...s/dev/vd8/app/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AND 1 WARNING(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
  44 | WARNING | Line exceeds 80 characters; contains 93 characters
  79 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  80 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  90 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  94 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  98 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  99 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
104 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
108 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: ...d8/app/core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeParametersTest.php
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
117 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
117 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
121 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
121 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
126 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
--------------------------------------------------------------------------------

larowlan’s picture

btw, happy to fix those coding standards violations if you set me up with sandbox access so I can push

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new2.83 KB
new610.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,490 pass(es).
[ View ]

Should fix the fails from #116
Now that MenuTreeStorage is responsible for sorting links, and the ksort has been removed, one of the test cases in the provider is redundant.

Also, needs to use RouteMatch, not RequestStack

All becomes clear in interdiff.

pwolanin’s picture

StatusFileSize
new15.27 KB
new611.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,524 pass(es).
[ View ]

Some of the code style warnings are bogus.

I think we are now using inline @var docs for type documentation like:

      /** @var \Drupal\Core\Menu\MenuLinkInterface $instance */

Though it seems some can be removed due to the conversion to use \Drupal\Core\Menu\MenuLinkTreeElement

The 83 char lines are @param lines like:

   * @param \Drupal\Core\Menu\MenuParentFormSelectorInterface $menu_parent_selector

It also seems unhappy about this, though I think there's no valid way to make it shorter?

Made a new PR due to the re-roll (github apparently doesn't understand how to handle that): https://github.com/pwolanin/drupal/pull/3

@return $this
pwolanin’s picture

StatusFileSize
new10.49 KB
new611.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,473 pass(es).
[ View ]

some more little cleanups:

remove build() method from MenuLinkInterface since it's basically unused and redundant. Wim and I discussed this last week, but hadn't gotten to it.

adds a getExpanded() method to MenuLinkTreeInterface and implements it as a simple proxy to the MenuTreeStorage since otherwise it's impossible for a developer to get the list of expanded links like is done inside the method to get the default parameters.

renames the expanded property to expandedParents on the parameters class and tries to clarify the docs a bit.

mdrummond’s picture

I took a look through this with pwolanin today to see if the way the new menu system works would be compatible with what I was working on in #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables. From what I can see, yes it is, and in a much easier way than before. The parameters for loading a menu tree seem to work much more sensibly than the previous system. This weekend I'll try to set up a proof of concept patch to get #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables working when this issue's most current patch is applied. I think it should work, but it will be good to try.

Nice work here.

effulgentsia’s picture

StatusFileSize
new3.21 KB

A lot of code has changed here since #73, so here's updated benchmarks. First of all, attached is an updated tester module (see #19).

HEAD: 163ms (4ms slower than it was at #73 (2 weeks ago), sigh)
Patch (#122): 203ms
Patch + the hack in #2289319-2: Cache getPathFromRoute() result in the {cache_menu}.tree-data cache: 179ms

In #73, patch+hack was same as HEAD, now it's 10% slower. Probably due to the merging of Wim's work (better code sometimes has a perf cost). Would be good to know what specifically is responsible for that to make an informed decision on whether to accept it or try to optimize.

larowlan’s picture

Manual testing:

  • Created a menu link in main nav
  • Dragged it under home
  • Saved
  • Edited parent
  • Moved to parent of home
  • Set expanded/not expanded
  • Tested output
  • Create node with menu link
  • Deleted link from node form
  • Added new link from menu system to node
  • Link showed up in node edit form
  • Moved link node to top of tree
  • Moved home/manual links under it
  • Edited node and deleted link
  • Home and manual links correctly moved back to top level after their parent was deleted
  • Tried to trigger XSS using title/description - no dice
  • Couldn't fault it

Looks great so far

pwolanin’s picture

@effulgentsia - yes we are doing things like instantiating more of the plugins on the assumption that the later render-cache work will give us quite an added performance boost. There are also some calls to the controller resolver we could potentially avoid by just building the callable, but I'm doubtful that would be a significant difference.

As before, I don't think storing the system path is the correct answer.

pwolanin’s picture

StatusFileSize
new611.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,719 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

had to re-roll for #1987424: seven.theme - Convert theme_ functions to Twig Would love some help watching the RTBC queue to avoid conflicts and bad changes like that.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.2 KB
new612.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256521-129.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, spent a while flailing on this but turns out that the implementation of MyAccountMenuLink didn't actually allow you to toggle the link off. This was exposed by the changed test in #2285381: Bad xpath in UserAccountLinksTests::testDisabledAccountLink() once I'd fixed the changed CSS class name.

Status:Needs review» Needs work

The last submitted patch, 129: 2256521-129.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new612.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,723 pass(es).
[ View ]

Stupid use statement conflict.

joelpittet’s picture

@pwolanin re #128 If I knew what was bad about that change, I'd have not done it to start. Could you explain?

pwolanin’s picture

@joelpittet - it's not as bad as I thought initially. However, I think constructing the whole A tag in the template is a little strange.

I'm also just worried I missed something in the re-roll in terms of Wim's plans for render caching.

joelpittet’s picture

Issue tags:+Twig
StatusFileSize
new1.64 KB
new1.64 KB

@pwolanin ok fewf.

This patch is green and looking good. Attached is just 4 space to 2 space for the twig template and removal of the unnecessary class key check & init as empty array.

joelpittet’s picture

StatusFileSize
new612.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,727 pass(es).
[ View ]
new1.64 KB

jebus getting late...

Oh just a note not probably not this patch but it would be nice if that url object was just __toString() instead of ->toString() method

pwolanin’s picture

Issue summary:View changes

@joelpittet - thanks for that formatting fix. I'll add the incremental change to the sandbox.

There was some debate about toString() vs __toString() - we'll have to search for the issue (wasn't my patch), I agree it's not that intuitive.

effulgentsia’s picture

Issue summary:View changes

To aid reviewers, I added a section to the issue summary that details where various code from the former menu_link module is moved to by this patch.

pwolanin’s picture

Issue summary:View changes

Updated the summary a bit.

Per discussion w/ effulgentsia , I'm working on removing the discovered field from the definition - it's really internal to the tree storage.

renaming hook_menu_links_alter() to hook_menu_links_discovered_alter() to make it more clear what's being passed in.

effulgentsia’s picture

Issue summary:View changes
pwolanin’s picture

StatusFileSize
new11.69 KB
new614.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,740 pass(es).
[ View ]

remove discovered from definition, kill active_menus_default config, rename alter hook

effulgentsia’s picture

Issue summary:View changes

Added menu.inc function removals to the issue summary's patch map.

effulgentsia’s picture

StatusFileSize
new243.61 KB

This patch is a subset of #140. It doesn't contain the \Drupal\Core\Menu additions or their unit tests, the menu_link module removal or the menu_link_content module addition, and I also stripped out the menu.inc function removals documented in the issue summary. Basically, what's left is what the impact of the patch is to the rest of Drupal. This is what I plan to do a detailed dreditor review of first, so uploading the patch in the hopes it's small enough for dreditor to not choke on.

Other reviewers: please feel free to review this portion as well, the full #140 patch, or a subset of your choice.

pwolanin’s picture

Issue summary:View changes
StatusFileSize
new13.72 KB
new617.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,741 pass(es).
[ View ]

after more discussion w/ @effulgentsia, removing function menu_link_rebuild_defaults() in favor of direct calls to the plugin manager rebuild() method. There is only one except in tests.

Also replaces calls to \Drupal::service('menu.link_tree') with \Drupal::menuTree() to get better type hinting.

For clarity, rename $tree_cache_backend to $menu_cache_backend in the MenuTreeStorage

updated PR since the last time I had to rebase: https://github.com/pwolanin/drupal/pull/4

larowlan’s picture

I've reviewed this again manually and the code.
In this code-review I focused on the external-facing API.
Screencast of manual review here: http://youtu.be/LxnKzQharK8

Plus some screenshots of views integration which I missed in the screencast

We have some of the highest phpunit coverage in core/Menu with this patch

Code review comments (copied from https://github.com/pwolanin/drupal/pull/4):

  1. MenuLinkBase::getTitle() -> would adding String::checkPlain here increase security without breaking anything?
  2. MenuLinkContentDeleteForm::submit() calls deprecated watchdog (follow up is fine)
  3. MenuLinkContentForm::doValidate docblock says route_nae instead of route_name (minor)
  4. menu_ui.info.yml missing blank link at EOF (minor)
  5. MenuLinkAdd::getOptions() uses Request->attributes->get(_system_path) which is slated for removal
  6. LinksTest has been gutted and replaced with a @todo - what is the plan here - that surely needs to go back in - up to a committer whether it should block this or be a follow-up
  7. MenuRouterRebuildTest has been removed - same as comment 6
  8. So with the exception of 6 and 7, which need a committer's input, I think most of this can be done in follows/quick edits.
    I'm going to be bold here and say this is RTBC - but we need a draft change notice and a list of change notices that will require updating first in addition to a call on LinksTest and MenuRouterRebuildTest.

  • Is it perfect? no
  • Do we have test coverage of the menu system? yep, lots (although some removed)
  • Does this patch add new tests for new functionality? yep, in bucket loads - see code coverage above
  • Do we want to ship Drupal 8 this decade? yes
  • Can we fix any resulting issues in follow-ups? yes
  • Are we likely to get many reviews of a patch this big? no
  • Are we likely to find issues once this is in HEAD? yes
  • Would we find these issues before release if this sits at 'needs review' for three months? no
  • Will getting it in earlier rather than later risk Drupal 8's release? no
  • Have some of the brightest Drupal minds worked on this? yes

in other words, once we have a change-notice draft and word on LinksTest and MenuRouterRebuildTest:

larowlan’s picture

Embedding the screencast for those who don't want to leave the comfort of d.o

effulgentsia’s picture

Is it perfect? no...Are we likely to get many reviews of a patch this big? no...[ship it anyway]

If someone is willing to RTBC this patch more or less as-is, and a committer is willing to commit it, despite the various problems that would get surfaced with detailed reviews, I won't stand in the way of that.

However, I have my doubts on a committer stepping up to do that. So, I took a stab at splitting the patch up into 5 steps that can be reviewed and committed in sequence without introducing any functional regressions (mostly, because the first 3 steps just add code without changing anything to use it, and the last step just deletes code, so only step 4 is the magic "convert all the things" step). See this issue's "child issues" block for the links.

Wim Leers’s picture

#115: good catch in SystemManager not specifying the menu for which to retrieve the active trail. I indeed missed that one.

#123: +1 for removing build()

#124: Yes! :) This now incorporates much of the refactoring of #1805054: Cache localized, access filtered, URL resolved, (and rendered?) menu trees, and there I took it specifically into account to make #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables trivial — i.e. to provide a sane API to do get any kind of menu tree you could possibly want :)

#125: Thanks for the profiling!


As of #146, this issue is now effectively split up into multiple child issues, to make it more reviewable/committable. It's mentioned in #146, but here are the links inline for your convenience:

  1. #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage
  2. #2301273: MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests
  3. #2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI
  4. #2301317: MenuLinkNG part4: Conversion
  5. #2301319: MenuLinkNG part5: Remove dead code; and party!
heddn’s picture

Title:New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module» [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module
Issue summary:View changes
Related issues:+#2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage, +#2301273: MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests, +#2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI, +#2301317: MenuLinkNG part4: Conversion, +#2301319: MenuLinkNG part5: Remove dead code; and party!

Re-titling to make it clear this is only a meta at this point.

plach’s picture

Sorry, guys, did I dream it or at some point in the sandbox the MenuLink entity used to have a data table? Without it we cannot translate (content) menu link titles/descriptions.

Sorry, that was the MenuLinkContent entity type :)

pwolanin’s picture

This handbook page has been partly updated, but the image needs to be revised, and I would appreciate help fleshing out the node type menu link example.
https://www.drupal.org/node/2118147

xjm’s picture

I've moved the beta blocker tags to the child issues.

Dries’s picture

Status:Needs review» Fixed

Just committed the last child issue so marking this beta blocker fixed! :)

Gábor Hojtsy’s picture

Re #150, I updated the image on https://www.drupal.org/node/2118147 as requested. The image source is in this Google Drawing: https://docs.google.com/drawings/d/1oiHWy9ERY3ySI8uulQtSxbGEsdlmmrN8ojv8... -- shared with the world, so if for some reason I disappear, you can copy and adjust from there if nothing better.

xjm’s picture

I added a link to the Google Drawing in the doc itself so anyone who wants to update it can find that easily to copy from.

Status:Fixed» Closed (fixed)

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