[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

Willing to help here

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

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.

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

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.

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.

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

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?

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.

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.

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.

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?

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

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.

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.

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

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

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.

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.

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.

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

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

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

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

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

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

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

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.

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.

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

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.

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.

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.

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

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:

• https://gist.github.com/nikic/5015323
• http://technosophos.com/2009/05/29/set-objects-php-arrays-vs-splobjectst...
• http://www.yiiframework.com/forum/index.php/topic/11568-php-objects-vs-a...

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.

Fixing the sole test failure in #102.

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.

#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!

Re-roll after #2290129: Menu/routing topic needs an overhaul

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

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

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.

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.

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.

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 re #128 If I knew what was bad about that change, I'd have not done it to start. Could you explain?

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

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

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.

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

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.

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

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:

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

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.

#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!