New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins

Wait... what's the justification for this? Didn't we just put a crapton of work into making links entities? Can we please stop confusing plugins and entities? :-)

@crell
The better place is #2178723-47: [meta-2] Finalize module-facing API of declaring menu links and followed.

So would the suggested YML files be hardcoded like local tasks, routes, etc? Ie. not in the config system but as their own YML files?

Also, what would people do if they have a view that has an admin path that they want to ship with? Is it shipping the view with the module? Shipping the view and the YML file manually created somehow? In other words: how do you turn a UI created admin view to an exported view shipped with a module?

As per @pwolanin the response is the following:

- yes, admin menu items would be static one-off plugin YML like local tasks, routing, etc.
- yes, UI created views would be shippable in modules because Views would have a derivative plugin handler that would expose the views configured admin items, so these would be translatable as part of the View with config translation

These work for translation.

That sounds good to me. @pwolanin and I talked about this yesterday. I think this model matches how blocks are implemented and would be fine for translatability as well as far as I understand. It may not be the nicest solution but it applies a pattern established elsewhere even if that pattern is not the work of perfection.

If this includes changes to the storage (and I believe it does) then it'd be beta-blocking.

Just a patch representation of the current sandbox for people who like that sort of thing. CNR for bot, not humans.

pwolanin asked me to try out their current sandbox user experience. I DID NOT review the code or the approach. Comments:

- That you cannto edit the built-in item title/path is confusing, it shows "Path: Administration" linking to the right place though; adding "This is a module provided link. Label and path cannot be changed." or something along those lines would be good and fixing so the display of it makes more sense.
- Custom menu links have the path field way too down in the form, should be up like in D7, that and title are the primary 2 things about the menu item.
- If you translate a built-in menu item the toolbar regressed that it looses the icon of the item. That is not the case in D8.
- Menu items don't yet have a #language_select element on their form; so although you can configure new entity defaults and language selector visibility for them in language module, it does not actually appear; see admin/config/regional/content-language with language module enabled and other entity types for #language_select use
- Menu link fields title and description appear for translation settings (revisit admin/config/regional/content-language with content translation also enabled) BUT the translation settings cannot be saved on them; other entity types, eg. basic page works
- Then I wanted to go and see how does the menu item show, on trying to manage my menu (ie. not even edit the menu item just displaying the list of menu items on the admin page). I got Fatal error: Call to a member function isTranslatable() on a non-object in /Users/gabor/Web/2031809/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationManager.php on line 39
- Since now I cannot edit any menu, I went and disabled content translation again. Then I tried moving around the translated menu item to a different place and then different tree position and it still all worked. And it was still translated.

That concluded my quick review. Again I DID NOT review the code or the approach.

The fatal can be reproduced by just enabling content translation module and then visiting any admin/structure/menu/manage/* no special configuration needed.

Just another patch snapshot for visibility + simplytest.me.

Trivial merge fix just to get bot to run.

Looks like #2256497-28: [meta] Menu Links - New Plan for the Homestretch has this as a major.

Here is little help to move it fwd.

1. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +   * @var \Drupal\Core\Menu\MenuLinkInterface
   

   desc line missing.

2. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +    $this->menuTree = $menu_tree;
   

   $this->menuTree is not defined in this class.

3. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +      $this->menuLink->build(),
   

   Are we missing something here?

4. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +      '#title' => $this->t('Enable menu link'),
   

   Isn't it "Enable" currently in core instead of "Enable menu link"?

5. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +//      '#title_display' => 'invisible',
   

   not needed

6. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +       '#description' => $this->t('If selected and this menu link has children, the menu will always appear expanded.'),
   

   wrong indentation

7. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +    // @TODO Should we expose expanded?
   

   is it still pending?

8. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,171 @@
   +      $this->translationManager = \Drupal::translation();
   

   Why are we not using injection here?

9. +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
   @@ -0,0 +1,91 @@
   +   * @var array
   ...
   +   * @var \Drupal\Core\Menu\StaticMenuLinkOverridesInterface
   

   desc line missing.

10. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
    @@ -0,0 +1,177 @@
    +   * Update and save values for a menu link.
    

    Updates and saves

11. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +use Drupal\Component\Utility\String;
    +use Drupal\Component\Utility\Unicode;
    +use Drupal\Component\Utility\UrlHelper;
    +use Drupal\Core\Cache\Cache;
    +use Drupal\Component\Utility\NestedArray;
    +use Drupal\Core\Entity\EntityManagerInterface;
    +use Symfony\Cmf\Component\Routing\RouteObjectInterface;
    +use Drupal\Component\Plugin\Exception\PluginException;
    +use Drupal\Core\Access\AccessManager;
    +use Drupal\Core\Cache\CacheBackendInterface;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    +use Drupal\Core\Language\LanguageManagerInterface;
    +use Drupal\Core\Plugin\Discovery\ContainerDerivativeDiscoveryDecorator;
    +use Drupal\Core\Plugin\Discovery\YamlDiscovery;
    +use Drupal\Core\Plugin\Factory\ContainerFactory;
    +use Drupal\Core\Routing\RouteProviderInterface;
    +use Drupal\Core\Session\AccountInterface;
    +use Symfony\Component\HttpFoundation\RequestStack;
    

    Can we just reorder them? vendor last and etc.

12. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +class MenuLinkTree implements MenuLinkTreeInterface {
    

    There are still some missing public functions in this class not defined in interface.

13. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +  protected $factory;
    

    Can we use some better name here?

14. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +  protected $preferredLinks = array();
    +  protected $buildAllDataParameters = array();
    

    doc block missing.

15. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +    $this->factory = new ContainerFactory($this);
    

    Can we move it to the end or start of the function? and Maybe add a comment.

16. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +  public function getInstance(array $options) {
    

    I can't find it on interface.

17. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +    $cid = 'links:' . $menu_name . ':page:' . $system_path . ':' . $language_interface->id . ':' . $page_is_403 . ':' . (int) $max_depth;
    ...
    +      $cid .= ':trail';
    

    Can we make it array and then implode?

18. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +   * @TODO should this accept a menu link instance or just the ID?
    

    @todo.

19. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
    @@ -0,0 +1,992 @@
    +  public function getParentDepthLimit($id) {
    ...
    +  protected function parentSelectOptionsTreeWalk(array $tree, $menu_name, $indent, &$options, $exclude, $depth_limit) {
    

    doc blocks missing.

20. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   * This should only be called when the link data has already been removed from
    +   * any external storage.  This method will not attempt to persist the deletion
    

    more then 80 chars.

21. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   * @param string $route_name
    +   * @param array $route_parameters
    +   * @param bool $include_hidden
    

    desc missing.

22. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   *     condition key/value pairs; see _menu_build_tree() for the actual query.
    

    more then 80 chars.

23. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   * Fetches a menu link which matches the route name, parameters and menu name.
    +   * @param string $route_name
    

    Space missing.

24. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   * @param array $route_parameters
    +   * @param null $selected_menu
    +   * @return mixed
    

    desc missing and space missing.

25. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   * @return \Drupal\Core\Menu\MenuLinkInterface
    

    I think it also returns noting if id in not there.

26. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +   * Get a form instance for editing a menu link plugin.
    ...
    +   * Get the options for a select element to choose and menu and parent.
    ...
    +   * Get a list of menu names for use as options.
    

    Gets

27. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
    @@ -0,0 +1,330 @@
    +  public function menuNameExists($menu_name);
    ...
    +  public function getParentDepthLimit($id);
    

    doc block missing.

28. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +class MenuLinkTreeStorage implements MenuLinkTreeStorageInterface {
    

    class doc missing.

29. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Execute a select query while making sure the database table exists.
    

    Executes

30. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Using the link definition, but up all the fields needed for database save.
    

    @param block missing.

31. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +    // If no corresponding parent link was found, move the link to the top-level.
    

    more then 80 chars.

32. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +      //  will be 0 even if there are children if those are hidden. has_children
    

    more then 80 chars.

33. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Find the relative depth of this link's deepest child.
    ...
    +   * Set the materialized path field values based on the parent.
    ...
    +   * Using the query field values and original values, move the link's children.
    

    @param block missing.

34. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Load the parent definition if it exists.
    

    Loads and @param block missing.

35. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Set the has_children flag for the link's parent if it has visible children.
    

    Sets

36. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +    // @todo - does this make more sense than using the system path?
    

    have we finalized yet?

37. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Load all table fields, not just those that are in the plugin definition.
    

    loads and @param block missing.

38. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +  protected  function loadFullMultiple(array $ids) {
    

    doc block missing.

39. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Prepares the data for calling $this->treeDataRecursive().
    

    @param block missing.

40. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   *   An array of the menu link ID values that are in the path from the current
    ...
    +      // We need to determine if we're on the path to root so we can later build
    

    more then 80 chars.

41. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorage.php
    @@ -0,0 +1,1177 @@
    +   * Check if the tree table exists and create it if not.
    

    Checks

42. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +  public function rebuild(array $definitions);
    

    desc line missing.

43. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +   * Load a plugin definition from the storage.
    ...
    +   * Load multiple plugin definitions from the storage.
    ...
    +   * Load multiple plugin definitions from the storage based on properties.
    ...
    +   * Load multiple plugin definitions from the storage based on route.
    

    Loads

44. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +   *   Array of menu Link definitions
    ...
    +   *   Array of menu link definitions
    ...
    +   *  Array of menu link definitions keyed by ID.
    

    An array.

45. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +   * @param string $route_name
    +   * @param array $route_parameters
    +   * @param bool $include_hidden
    

    desc missing.

46. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +   * Save a plugin definition to the storage.
    

    saves

47. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +   * Load all the visible links that are below the given ID.
    ...
    +   * Load all the IDs for links that are below the given ID.
    ...
    +   * Load a subtree rooted by the given ID.
    

    Loads

48. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +   * @param $id
    ...
    +   * @param $id
    ...
    +   * @param string $menu_name
    +   * @param array $parents
    ...
    +  public function getExpanded($menu_name, array $parents);
    

    desc line missing.

49. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeStorageInterface.php
    @@ -0,0 +1,199 @@
    +  public function findChildrenRelativeDepth($id);
    ...
    +  public function menuNameExists($menu_name);
    ...
    +  public function getMenuNames();
    

    doc block missing.

50. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuTreeStorageTest.php
    @@ -0,0 +1,298 @@
    +  public function testBasicMethods() {
    ...
    +  public function doTestEmptyStorage() {
    ...
    +  public function doTestTable() {
    ...
    +  public function testSimpleHierarchy() {
    ...
    +  public function testMenuLinkMoving() {
    ...
    +  public function testMenuHiddenChildLinks() {
    

    doc block missing.

51. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuTreeStorageTest.php
    @@ -0,0 +1,298 @@
    +   * Add a link with the given ID and supply defaults.
    ...
    +   * Move the link with the given ID so it's under a new parent.
    ...
    +   * Test that a link's stored representation matches the expected values.
    

    Adds, moves, tests

52. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuTreeStorageTest.php
    @@ -0,0 +1,298 @@
    +   * @param $id
    

    typehint missng.

53. +++ b/core/modules/views/lib/Drupal/views/Plugin/Menu/ViewsMenuLink.php
    @@ -0,0 +1,156 @@
    +   * @var array
    ...
    +   * @var \Drupal\Core\Entity\EntityManagerInterface
    ...
    +   * @var \Drupal\views\ViewExecutableFactory
    

    desc line missing.

54. +++ b/core/modules/views/lib/Drupal/views/Plugin/Menu/ViewsMenuLink.php
    @@ -0,0 +1,156 @@
    +  protected $view;
    

    doc block missing.

Take my patch to add a link "translate" on the page - admin/structure/menu/manage/main

Can we use some better name here?

Nope, this is the standard for plugin managers.

I can't find it on interface.

PluginManagerInterface => MapperInterface

Can we make it array and then implode?

We just do it like HEAD does it.

more then 80 chars.

Hope, exactly 80

I think it also returns noting if id in not there.

Sorry but the review does not offer enough context. Sadly I can't find which one you need.

Pushed a non-small doc cleanup.

Thank you very much for the fixes @dawehner.

Sorry but the review does not offer enough context. Sadly I can't find which one you need.

I blame dreditor for this. :)

It should fix some tests:

Drupal\shortcut\Tests\ShortcutLinksTest
Drupal\simpletest\Tests\DrupalUnitTestBaseTest
Drupal\toolbar\Tests\ToolbarAdminMenuTest

@pwolanin The main idea MenuLinkDefaut is that we need notify anothers when link is updated. At this case we need notify the module toolbar to pass the test AdminToolbarMenuTest. It needs invokeAll. I tried To implement it according to DIC. So it looks weird.

@jibran
Fixed some of yours

@likin
I think we should do that in a generic place.
Also merged in some of your fixes.

@dawehner Could you give a git access for the repository?

@likin
Granted ...

This time it should be green.

So.

That's in now.

Okay, we need to come up with 40 kb more documentation in order to get a sane size.

@everyone_else
Talk to us so we can help you to help us to review the patch.

@dawehner Describe what help is realy needed.

I have started looking at the patch. Below are a few things I noticed, and questions of someone who hasn't spent too much time with D8 core yet.

Note, that I have only managed to get through the first 30-40% of the patch; I haven't looked at stuff related to the menu content entity nor any of the tests.

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -0,0 +1,205 @@
   +/**
   + * Default object used for LocalTaskPlugins.
   + */
   +interface MenuLinkInterface extends PluginInspectionInterface, DerivativeInspectionInterface {
   

   docblock description doesn't seem to match the Interface?

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -0,0 +1,1182 @@
   +  function getChildIds($id) {
   

   Missing scope ("public").

3. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
   @@ -0,0 +1,386 @@
   +/**
   + * Provides an object which returns the available menu links.
   + */
   +interface MenuLinkTreeInterface extends PluginManagerInterface {
   

   This interface/service seems to cover quite a few different functionalities:

   • discovery of menu link plugins
   • create/update/delete/reset operations on plugins
   • reading/listing plugins
   • retrieving link trees (i.e. reading multiple plugins?), as well as retrieving subtrees, parents, children, ...
   • other utility functions for accessing tree storage features (e.g. maxdepth)
   • functionality related to rendering a menu tree
   • functionality related to determining active menus and the active trail
   • functionality related to forms for menu items (?)

   I'm wondering if some of these could be separated into more interfaces/classes, to make it easier to understand. Not sure about multiple services, but maybe at least separate, single-purpose interfaces (+use of inheritance)?

4. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
   @@ -0,0 +1,386 @@
   +  /**
   +   * Trigger discover save, and cleanup of static links
   +   *
   +   * @todo find a better name?
   +   */
   +  public function rebuild();
   

   Seems incomplete. Also missing a dot at the end.

   Maybe "rediscover" instead of "rebuild"? In my opinion renaming is not necessary, as long as the docblock is descriptive enough. :)

5. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
   @@ -0,0 +1,386 @@
   +  public function menuNameExists($menu_name);
   

   Missing docblock.

6. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1233 @@
   +  /**
   +   * Defines the schema for the tree table.
   +   */
   +  protected static function schemaDefinition() {
   +    $schema = array(
   +      'description' => 'Contains the menu tree hierarchy.',
   +      'fields' => array(
   

   From what I understand, the purpose of the "MenuTreeStorage" is currently two-fold: a) to store information on discovered menu link plugins, and some of their parameters/configuration etc.; and b), to store the hierarchical organization of menu items and to make it easy to quickly query information from that hierarchy.

   Reading the class name "MenuTreeStorage", I primarily think of the second part, i.e. the storage of hierarchical info.

   I understand that these two "tasks" are currently implemented with one database table for performance reasons, but I think that DX could profit from splitting the interface (and class) into two parts for a) CRUD on "plugin definitions", and b) managing hierarchical information (getParentPath, getParent, getChildren, etc.).

   Maybe the CRUD part could be moved to an (abstract?) base class MenuLinkPluginStorage, which is extended by MenuTreeStorage (adding hierarchical things). This would make it easier to replace the materialized path implementation with something else.

7. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1233 @@
   +  /**
   +   * Loads the parent definition if it exists.
   +   *
   +   * @param $link
   +   * @param $original
   +   * @internal param $
   +   *
   +   * @return array
   +   */
   +  protected function findParent($link, $original) {
   

   Incomplete docblock.

8. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
   @@ -0,0 +1,243 @@
   +  /**
   +   * Loads multiple plugin definitions from the storage based on properties.
   +   *
   +   * @param array $properties
   +   *   The properties to filter by.
   +   *
   +   * @return array
   +   *   An array of menu link definitions.
   +   */
   +  public function loadByProperties(array $properties);
   

   I think this method could make things difficult for alternative tree storage implementations because it seems fairly generic; it's not clear which properties are valid and commonly used for querying via this function (to me anyway), so optimising an implementation will likely also be difficult. From what I saw in the patch, this method is only called from the outside with the 'menu_name' property, so maybe it can be removed (or made protected) with the addition of a new "loadByMenuName" method?

9. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
   @@ -0,0 +1,243 @@
   +   * This function may be used build the data for a menu tree only, for example
   +   * to further massage the data manually before further processing happens.
   +   * MenuLinkTree::checkAccess() needs to be invoked afterwards.
   

   There's a "to" missing in the first sentence.

10. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,243 @@
    +  /**
    +   * Returns all the internal materialized IDs.
    +   *
    +   * @param string $id
    +   *   The parent menu link ID.
    +   *
    +   * @return array
    +   *   An array of plugin IDs that represents the path from this plugin ID
    +   *   to the root of the tree.
    +   */
    +  public function getMaterializedPathIds($id);
    

    The keyword "internal" somewhat contradicts the fact that it's exposed on a public interface. ;-)

    Also, as far as I know the "materialized path" describes a very specific way of storing hierarchical information; so in order to decouple the interface from a specific storage implementation, I think this could be renamed to something like getParentPath(), which is more neutral imo.

11. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,243 @@
    +  /**
    +   * @param string $menu_name
    +   *   The menu name.
    +   * @param array $parents
    +   *
    +   * @return array
    +   *   The menu link ID that are flagged as expanded in this menu.
    +   */
    +  public function getExpanded($menu_name, array $parents);
    

    Incomplete docblock.

12. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,243 @@
    +  /**
    +   * Finds the relative depth of this link's deepest child.
    +   *
    +   * @param string $id
    +   *   The parent menu link ID.
    +   *
    +   * @return int
    +   *   Returns the relative depth.
    +   */
    +  public function findChildrenRelativeDepth($id);
    

    According to Wikipedia: The depth of a node is the length of the path to its root (i.e., its root path).

    Therefore, I think this could just be named getDepth($id).

    Using get* instead of find* might also be more consistent with other method names; on the other hand, there seems to be quite a few different patterns (load*, get*, find*, count*, and others), so not sure.

13. +++ b/core/modules/menu_link_content/lib/Drupal/menu_link_content/MenuLinkContentAccessController.php
    @@ -0,0 +1,58 @@
    \ No newline at end of file
    

    newline.

I definitely agree that the separation of storage and "other things" is already a big step forward! :)

One argument for keeping the MenuLinkTree monolithic is that it allows the tree storage to be private to it. If we split it up the tree storage is positioned as more of a public service, which isn't really my intent. Also, much more importantly, there is a subtle optimization that makes splitting it up problematic. In function checkAccess the code does: [...]

Can you elaborate on why it should not be made public? What's the benefit of keeping it private?

(Personally, I wish we had a more generic concept for storing hierarchical content; I do understand that that's out of scope for this issue, but wouldn't making the tree storage public help us go more into that direction?)

Also, I have to admit I don't completely understand how that checkAccess optimization works at the moment; I do understand that it helps prevent database queries, but could that not be formalized somehow, i.e. extracted into a separate method? it does not seem to have much to do with "checking access", nor is it mentioned as a side-effect in the docblock at the moment.

The materialized path value that's return is not implementation specific except, perhaps, in its naming. Happy for better naming suggestions. It's just the list of ancestor IDs leading to the root of the tree. What do you generically call the leaf to root path?

I think that would be called the "root path" of a given node/leaf.

findChildrenRelativeDepth() Is not the actual depth, it's the difference in depth between the link of the given ID and its deepest child. Happy for naming suggestions to clarify that too.

Sorry, I must have misread the description. I think what you're describing is actually called the "height" of a (sub-)tree.
Wikipedia: "The height of a node is the length of the longest downward path to a leaf from that node. The height of the root is the height of the tree."

---------------

I have read the patch partially from the perspective of a contrib developer, i.e. trying to figure out how to override certain functionality or pieces of code with custom versions. One example of something which contrib developers have wanted to adjust in D7, is the code surrounding the active trail/preferred menu item; I'm wondering how I would override the logic of getActiveTrailIds() and menuLinkGetPreferred() with the patch in D8?

This sounds like a good idea, we already have similar concepts in core - and this kind of stuff creates a lot of WTF's when you are translating. As long as its easy to overwrite in the code, when you do want to change it - it should all be fine.

Let me know when there is a UI issue, I'd like to make sure we provide a very understandable help text.

Yeah there are 3 ways to translate those menu items and in fact 3 UIs. I don't think that is a show-stopper, we also advocated introducing the basic functionality at least for translation and then contrib can ease the flow. Eg. blocks have the same problem, module provided blocks are translated as UI strings (eg. who is online block) while content blocks are translated as content entities BUT both of their titles are on the config entity and translated as configuration. So that is also a 3-way translation intersection for blocks. This is not really different for menus. That is where contribs like TMGMT will shine. If we'd consider this a showstopper, then even low level translation support would not be there which affects contribs very negatively because they will not assume translatable things and not interoperate with them.

I think we should probably a look a bit at how views provided items will be translated. I should in fact try that. :) Because you can translate the menu title as config translation on the view but does that end up in the menu display?

This translation test turned out to be a bit different in result than what I was expecting.

1. So the default main nav menu has a module provided Home link that is not editable. That is translatable with the locale UI. That works.
2. I created a custom menu item (content entity). Configured it to be translatable and translated the title. It shows up translated in the admin summary translation table of the entity. NOT in the menu though.
3. I created a menu item via the views UI with a page display. Then enabled config translation and translated the menu item there. This worked flawlessly showing the translated menu item as I expected.

When I switched back to my original English, it worked fine also. So the content translation translated menu item does not end up in the rendered menu:

It is of course confusing to translate these, only the content one has a direct translate tab in the menu UI. I don't think this is anywhere more complex than blocks which even combine different methods of translation in one object's different properties. Here it is "just" different object per object. So I don't think this is a desired UI overall but it is how the technology works and we accepted similar confusion for blocks and pushed that to contrib to improve the UI. Would be similar here. Solving the translation itself in core is crucial for contrib collaboration though.

I read (some parts less thoroughly than others) through the patch, and overall, like it a lot, and think it's a huge improvement in terms of architecture and code relative to HEAD. While per #74.3, MenuLinkTree is ridiculously monolithic, I don't think it's any more monolithic than HEAD's MenuTree and menu.inc functions that it's replacing.

My biggest concern with the patch is this:

+++ b/core/modules/menu_link_content/lib/Drupal/menu_link_content/Plugin/Menu/MenuLinkContent.php
@@ -0,0 +1,196 @@
+  public function getTitle() {
+    return $this->getEntity()->getTitle();
+  }

Not just 'title', but the plugin instance to entity proxy in general. Can we get xhprof and db query comparison of patch vs HEAD for rendering a menu with many content links?

@effulgentsia
Previously $menu_link['title'] caused a __get call so I can't really imagine how the new code could be much slower than the old one. It is one call to this function per rendered menu link.

@pwolanin: ok, well, so a fundamental difference between content entities and config entities is config entities are always loaded with overrides in the page's language and you need to work to NOT load them translated. For content entities, it is the other way around. For some reason. If you would know the language you would use getTranslation($langcode) on the entity. But since you don't know and want to use the language negotiation/fallback system, you would use the entity manager, so $this->entityManager->getTranslationFromContext($entity); (I see you already have the entity manager injected). More docs on Drupal 8 entity translation API at https://drupal.org/node/2040721

Re #85: but at what point is the entity loaded? Does it benefit from a loadMultiple()?

@effulgentsia
Really good point. Previousyl MenuTree::doBuildTree() had that multi oading. I do agree 100% that this would be a critical regression,
much like #2250315: Regression: Bring back node access optimization to menu trees is at the point, but noone reviewed it yet.

I wonder how to best provide this functionality. Technically this certainly does not belong into the storage but in the rendering kind of code. We could throw some event
so other modules can work on the tree. An alternative could be to collect the plugin IDs per plugin class and call some static method. Any oppinions?

@gabor
Pushed a new version, see the interdiff. Is it just me or should cache_context.language be added to pretty much everything? Output most always have either a t() or some entity translation involved.

@dawehner: yeah in your case the menu item is possibly translated with one of these methods: (1) content entity translation for custom menu items (2) t() for static module shipped items (3) config translation for views provided items. So all items may be language dependent somehow.

@Gábor
Well, I talk about all possible blocks. Basically all of them should have some sort of translation involved in some step, when they generate the content.

@dawehner #91: yup!

I'll be bold here and mark this issue fixed, and suggest that the next patch should be posted to #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, where detailed reviews can proceed. If anyone has remaining concerns about the architecture in general, please reopen this issue so we can discuss them, but IMO, we're ready to move on to detailed reviews in that issue.

Also, resetting priority to match the parent issue.