Problem/Motivation

The longrunning goal is to pretty much move any logic from core into menu_link as it is pointless to use it without actual available menu links.

Proposed resolution

Move all tree related functions into a MenuTree service on the menu_link module. We could iterate on additional methods to provide interfaces etc. if we can agree what we actually need at the end.

Remaining tasks

None, it has been already committed.

User interface changes

None.

API changes

See the change record.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner’s picture

Status: Active » Needs review
FileSize
49.46 KB

Status: Needs review » Needs work

The last submitted patch, 2: menu_tree-2207893-2.patch, failed testing.

The last submitted patch, 2: menu_tree-2207893-2.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit
FileSize
69.09 KB
12.94 KB

Removed a couple of old tests and replaced it with proper unit testing.

Status: Needs review » Needs work

The last submitted patch, 5: menu_tree-2207893-5.patch, failed testing.

tim.plunkett’s picture

Ugh, running phpunit via simpletest sucksssssss

Drupal\menu_link\Tests\MenuTreeTest must implement \Drupal\Tests\UnitTestCase::getInfo().

It runs fine from the CLI of course.

dawehner’s picture

Status: Needs work » Needs review
FileSize
69.86 KB
3.54 KB

/**
* {@inheritdoc}
*/
public static function getInfo() {
return array(
'name' => 'Tests \Drupal\menu_link\MenuTree',
'description' => '',
'group' => 'Menu',
);
}

Go along, there is no helpful information here.

xjm’s picture

Issue tags: +Needs change record, +beta target, +API change
Parent issue: » #2177035: [Meta] Menu system home-stretch

Neat. We should probably try to get this in by beta; I'm not sure if this is a required part of #2177035: [Meta] Menu system home-stretch or not? Also, nice summary... ;)

xjm’s picture

Erg. Stupid d.o bug...

dawehner’s picture

Issue summary: View changes

Well, better than nothing.

pwolanin’s picture

This is more-or-less what I'm doing for book module. However, I'm a little unclear if this will actually play nicely with menu link entities?

Possibly the menu link entity itself should only know about plid and maybe has_children?

The hierarchy stuff could be in the same table or a different table, but it could be updated acting on an entity save, rather than being part of the entity itself.

The last submitted patch, 8: menu_tree-2207893-8.patch, failed testing.

dawehner’s picture

@pwolanin
I kinda think that all these tasks can be done on top of the patch later. This just provides a solid foundation to iterate on top of it.

amateescu’s picture

This patch will impact #1842858: [PP-1] Convert menu links to the new Entity Field API quite a bit and since it's still postponed on other issues, I'm going to add this one to the list.

dawehner’s picture

FileSize
69.86 KB

No idea what happened, just reuploading the last patch.

Status: Needs review » Needs work

The last submitted patch, 16: menu_tree-2207893-16.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
72.44 KB
13.01 KB

This was probably easier than expected.

jibran’s picture

  1. +++ b/core/includes/menu.inc
    @@ -332,13 +332,7 @@ function _menu_link_translate(&$item) {
     function menu_tree($menu_name) {
    
    @@ -356,62 +350,7 @@ function menu_tree($menu_name) {
     function menu_tree_output($tree) {
    
    @@ -435,52 +374,7 @@ function menu_tree_output($tree) {
     function menu_tree_all_data($menu_name, $link = NULL, $max_depth = NULL) {
    
    @@ -501,11 +395,7 @@ function menu_tree_all_data($menu_name, $link = NULL, $max_depth = NULL) {
     function menu_tree_set_path($menu_name, $path = NULL) {
    
    @@ -519,7 +409,7 @@ function menu_tree_set_path($menu_name, $path = NULL) {
     function menu_tree_get_path($menu_name) {
    
    @@ -545,120 +435,7 @@ function menu_tree_get_path($menu_name) {
     function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail = FALSE) {
    
    @@ -686,84 +463,7 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
     function menu_build_tree($menu_name, array $parameters = array()) {
    
    @@ -775,18 +475,7 @@ function _menu_build_tree($menu_name, array $parameters = array()) {
     function menu_tree_collect_node_links(&$tree, &$node_links) {
    
    @@ -799,47 +488,7 @@ function menu_tree_collect_node_links(&$tree, &$node_links) {
     function menu_tree_check_access(&$tree, $node_links = array()) {
    
    @@ -869,44 +518,7 @@ function _menu_tree_check_access(&$tree) {
     function menu_tree_data(array $links, array $parents = array(), $depth = 1) {
    

    we should ads @deprecated in the doc block.

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,622 @@
    +   * @var \Drupal\Core\Entity\Query\QueryFactory
    

    QueryFactoryInterface I think.

  3. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,622 @@
    +    // Use $mlid as a flag for whether the data being loaded is for the whole tree.
    ...
    +    // Generate a cache ID (cid) specific for this $menu_name, $link, $language, and depth.
    ...
    +          // The tree is for a single item, so we need to match the values in its
    +          // p columns and 0 (the top level) with the plid values of other links.
    ...
    +      // whether we have the menu already in cache, or otherwise, build a minimum
    ...
    +            // Find a menu link corresponding to the current path. If $active_path
    ...
    +              // we'd needlessly re-run _menu_build_tree() queries for every menu
    ...
    +                // If we are asked to build links for the active trail only, skip
    ...
    +        // Build the tree using the parameters; the resulting tree will be cached
    

    more then 80 chars.

  4. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,622 @@
    +  protected function doBuildTree($menu_name, array $parameters = array()) {
    ...
    +  protected function doCheckAccess(&$tree) {
    ...
    +  protected function doBuildTreeData(&$links, $parents, $depth) {
    ...
    +  protected function menuLinkGetPreferred($menu_name, $active_path) {
    ...
    +  protected function menuLinkTranslate(&$item) {
    

    @param block missing.

  5. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php
    @@ -0,0 +1,195 @@
    +   * @param $menu_name
    ...
    +   * @param $path
    ...
    +   * @param $tree
    ...
    +   * @param $node_links
    ...
    +   * @param $menu_name
    ...
    +   * @param $tree
    ...
    +   * @param $node_links
    

    Type hint missing.

  6. +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php
    @@ -0,0 +1,342 @@
    +  protected $connection;
    +  protected $cacheBackend;
    +  protected $languageManager;
    +  protected $requestStack;
    +  protected $entityManager;
    +  protected $entityQueryFactory;
    +  protected $state;
    

    @var doc block missing.

  7. +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php
    @@ -0,0 +1,342 @@
    +  protected function menuLinkTranslate(&$item) {
    

    doc block missing.

  8. +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/MenuTreeTest.php
    @@ -0,0 +1,342 @@
    +  protected function menuLinkGetPreferred($menu_name, $active_path) {
    +  }
    

    I like this function.

dawehner’s picture

FileSize
35.78 KB

Here are some of the docs, more will come later

dawehner’s picture

FileSize
90.17 KB
31.55 KB

we should ads @deprecated in the doc block.

Let's just replace the few instances which are left. This aren't that often used APIs so let's clean up menu.inc already.

QueryFactoryInterface I think.

Yeah I ran into that trap as well. We have two level of factories here: The Entity\query\QueryFactory takes care about
getting the query factory for a specific storage type (config, sql) and asking that specific query factory (which has the queryfactoryInterface) for an instance. The QueryFactory itself should not be swappable.

The reason for this was that the dependencies for the query is just known per storage type (for example the database connection).

more then 80 chars.

Fixed all the instances.

Type hint missing.

Good catch, fixed all of those.

@var doc block missing.

I like this function.

Well, it is how it is. We can extend the test coverage later

Status: Needs review » Needs work

The last submitted patch, 21: menu_tree-2207893-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
90.57 KB
1.9 KB

This should be it.

dawehner’s picture

23: menu_tree-2207893-23.patch queued for re-testing.

andypost’s picture

awesome clean-up, maybe better to decouple storage logic from the service and then name it as MenuTreeBuilder

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -34,6 +35,13 @@ class MenuFormController extends EntityFormController {
+   * The menu tree.

is this a service?

dawehner’s picture

is this a service?

Yes it is.

sun’s picture

Issue summary: View changes
  1. +++ b/core/includes/menu.inc
    @@ -1112,7 +524,9 @@ function menu_navigation_links($menu_name, $level = 0) {
    +    /** @var \Drupal\menu_link\MenuTreeInterface $menu_tree */
    +  $menu_tree = \Drupal::service('menu_link.tree');
    
    +++ b/core/modules/menu/menu.module
    @@ -263,10 +263,13 @@ function _menu_get_options($menus, $available_menus, $item) {
    +    /** @var \Drupal\menu_link\MenuTreeInterface $menu_tree */
    +  $menu_tree = \Drupal::service('menu_link.tree');
    

    Bogus indentation.

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,651 @@
    +  public function getRenderedTree($menu_name) {
    ...
    +  public function output($tree) {
    

    Hm. Two concerns:

    1. The "output" (render array) generation does not really belong to the responsibilities of a menu tree class — I think we should separate the concerns of menu tree building vs. rendering.

    2. output() is a slightly strange method name, because nothing is actually output. ;) How about renaming these methods to renderMenu($menu_name) and renderTree($tree)?

  3. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,651 @@
    +  protected function checkNodeLinksAccess(array $node_links) {
    

    Ugh, we're still hard-coding Node Access...?

    Not a fault of this nice and innocent patch and thus OT here. But I hope that this refactoring will allow us to inject an Entity Access Controller later on. :)

  4. +++ b/core/modules/toolbar/toolbar.module
    @@ -358,6 +358,9 @@ function toolbar_toolbar() {
    +    /** @var \Drupal\menu_link\MenuTreeInterface $menu_tree */
    +  $menu_tree = \Drupal::service('menu_link.tree');
    

    And once more...

    Do we really need to add those comments everywhere? (I personally find them pretty disturbing) :-/

    Anyway, not to be discussed here, we just need to fix the faulty indentations throughout the patch.

dawehner’s picture

Issue summary: View changes
FileSize
90.59 KB
7.58 KB

Bogus indentation.

Right, probably caused by a copy and paste error. In general though I really prefer using that in our existing procedural code. For example this allows really easy refactoring, as the IDE has enough context
available.

1. The "output" (render array) generation does not really belong to the responsibilities of a menu tree class — I think we should separate the concerns of menu tree building vs. rendering.

Sounds like a great idea. What about doing that in a follow up?

pwolanin’s picture

This patch incorporates the bug fix at #2206235: Optimized access check for node menu links is broken.. Also tries to make all the access munging more internal to the manager.

Still has at least 1 test fail, but I need to run out for a bit.

Status: Needs review » Needs work

The last submitted patch, 29: menu_tree-2207893-29.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.16 KB
90.69 KB

Ok, hopefully this bring it down to 1 fail.

I think the test may be broken - or the menu code is broken in terms of handling unpublished nodes with an access query.

In Drupal 7, links to unpublished nodes never appeared. A little weird (maybe a bug) - so Drupal 8 is doing the same now that the optimizaiton is fixed.

Status: Needs review » Needs work

The last submitted patch, 31: menu_tree-2207893-31.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
90.61 KB
1.28 KB

Rerolled. I hope this also fixes all the test failures.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! This is most certainly a big step in the right direction.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

So, discussed with catch in IRC, I think we should just rip out the node access optimization here (but leave it for book which was always the more importatnt use of it).

I also don't think the logic change in the interdiff is actually right.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
88.26 KB

This simplifies the logic quite a bit.

The public method names are still all too similar and hard to understand, but that's a bit hard to unwind at this point.

pwolanin’s picture

It's kind of sad that we are wrapping _menu_link_translate.

Really feels like that should be build into the menu link entity?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Oh great, even better if we don't need it anymore.

Opened a follow up for the _menu_link_translate bit: #2219073: Figure out a good place for _menu_link_translate

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: menu_tree-2207893-36.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

36: menu_tree-2207893-36.patch queued for re-testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

catch’s picture

Status: Reviewed & tested by the community » Needs review

Only small issues, overall this looks good and it'll make further cleanup simpler.

  1. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,602 @@
    +   * Stores the menu menu tree keyed by a cache ID.
    

    menu menu tree?

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,602 @@
    +  protected $menuFullTrees;
    

    Should this be menuTreeData? It's not clear how it's different from menuTree from the comment.

  3. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
    @@ -0,0 +1,602 @@
    +      if ($cache && isset($cache->data)) {
    

    No need for isset($cache->data) if $cache is not FALSE.

  4. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php
    @@ -0,0 +1,174 @@
    +   * Breadcrumbs are built using the preferred link returned by
    +   * menu_link_get_preferred(). If the preferred link is inside one of the menus
    

    Just code moving around but this is no longer true at all. Wonder whether some of this logic is redundant now and whether we're testing it anywhere - not for this issue though.

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php
    index 974208f..0000000
    --- a/core/modules/system/lib/Drupal/system/Tests/Menu/TreeAccessTest.php
    

    Nice to see this gone and the unit test added :)

dawehner’s picture

FileSize
88.38 KB
3.38 KB

Should this be menuTreeData? It's not clear how it's different from menuTree from the comment.

Yeah one of them contains the tree without the hidden items and the other one contains them.

No need for isset($cache->data) if $cache is not FALSE.

Sorry for that dejavu moment.

Nice to see this gone and the unit test added :)

Yeah unit tests just makes it much easier to test the edge cases.

jessebeach’s picture

Issue tags: -beta target +beta blocker

This is a beta blocker, not a target, since it blocks other blockers.

catch’s picture

Priority: Major » Critical

Also critical then.

Boobaa’s picture

Chasing HEAD: the patch didn't apply, so I rerolled it. Three small nitpicks.

  1. Drupal\menu_link\MenuTree had a protected $language<strong>r</strong>Manager (with a typo in it); I fixed this to $languageManager (as it is the thing being used later on).
  2. Drupal\menu_link\MenuTree::buildPageData() may use $page_not_403 uninitialized. I don't know if this is acceptable or not.
  3. Drupal\menu_link\MenuTree::renderTree() may use $data as a leftover from a previous foreach cycle. I don't know if this is by design or not.
dawehner’s picture

FileSize
88.38 KB

Drupal\menu_link\MenuTree had a protected $languagerManager (with a typo in it); I fixed this to $languageManager (as it is the thing being used later on).

Glad you found the issue!

Drupal\menu_link\MenuTree::buildPageData() may use $page_not_403 uninitialized. I don't know if this is acceptable or not.

This will never happens. $system_path will just exist if $page_not_403 is also set.

Drupal\menu_link\MenuTree::renderTree() may use $data as a leftover from a previous foreach cycle. I don't know if this is by design or not.

Yeah we just want to ues the menu name, so this leaking is always expected and will always happen. Nevertheless we can and should introduce a proper variable instead here.

Boobaa’s picture

Status: Needs review » Reviewed & tested by the community

Patch still applies (with some offsets, but anyway); tested by hand and all looks OK to me.

catch’s picture

47: menu_tree-2207893-47.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: menu_tree-2207893-47.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
24 KB
88.38 KB

In #1194136: Re-organise core cache bins, the bin for menu cacheing changed from cache.menu to cache.data. Since we're injecting the cache backend to the MenuTree class, I've updated the menu_link.services.yml to inject the @cache.data service instead of the @cache.menu service.

Otherwise, the cache bin changes caused a big reject in menu.inc; that was just deleting functions.

Both changes are represented in the interdiff.

I'll get started on the change notice.

dawehner’s picture

Thanks for the rerolls! I was actually quite sure that we would have already added one here, but I can't fit on https://drupal.org/list-changes/review
so this is probably not the case.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Just fixed the <?php> tags, but beside form it, perfect!

jessebeach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
88.39 KB
6.24 KB

Just nit-picky comment code standard stuff. We had a few lines over 80 characters.

jessebeach’s picture

I didn't change any code. Back to RTBC when it comes back green.

Boobaa’s picture

Status: Needs review » Reviewed & tested by the community

Bot said green, so it's RTBC (again). Interdiff is fine this time. ;)

dawehner’s picture

+1

  • Commit 4a57034 on 8.x by catch:
    Issue #2207893 by dawehner, pwolanin, jessebeach, Boobaa: Convert menu...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks!

jessebeach’s picture

Published the change notice: https://drupal.org/node/2226481

Boobaa’s picture

Issue summary: View changes
jessebeach’s picture

Issue tags: -Needs change record

Removed 'Needs change record'.

Status: Fixed » Closed (fixed)

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