MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage

+++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
@@ -0,0 +1,385 @@
+   * Constructs a \Drupal\Core\Menu\MenuLinkTree object.

I had a quick scan and only found a wrong class in the MenuLinkManager constructor.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
@@ -0,0 +1,197 @@
+ * Base class used for MenuLink plugins.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
@@ -0,0 +1,87 @@
+ * Default object used for MenuLink plugins.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
@@ -0,0 +1,218 @@
+ * Default object used for LocalTaskPlugins.
...
+   * Delete a menu link.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
@@ -0,0 +1,385 @@
+   * Instantiate if necessary and return a YamlDiscovery instance.

+++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
@@ -0,0 +1,168 @@
+   * Load multiple plugin instances based on route.
...
+   * Determine if any links use a given menu name.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -0,0 +1,1403 @@
+   * Helper function for rebuild that saves a link without clearing caches.
...
+   * Using the link definition, but up all the fields needed for database save.
...
+   * Set the has_children flag for the link's parent if it has visible children.
...
+   * Prepare a link by unserializing values and saving the definition.
...
+   * Recursive helper function to collect all the route names and definitions.
...
+   * Helper function to determine serialized fields.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
@@ -0,0 +1,252 @@
+   * Helper function for testing. Clears all definitions cached in memory.

+++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
@@ -0,0 +1,161 @@
+ * Implementation of the menu link override using a config file.
...
+   * Helper function to get the config object.

+++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php
@@ -0,0 +1,83 @@
+   * Force all overrides to be re-loaded from storage. Useful for testing.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
@@ -0,0 +1,147 @@
+   * Flag this instance as being wrapped in a menu link plugin instance.

+++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
@@ -0,0 +1,234 @@
+ * Provides the menu link plugin for content menu link.s

So that there's an easy warm-up novice task for sprinters to start fixing right off the bat: All these one-line summaries need to be adjusted to conform to the standards: https://drupal.org/node/1354#classes and https://drupal.org/node/1354#functions

Carefully read the class, function, or method, and write a one-line summary describing what it does that follows the standard. (What's there already is probably a good start.) The word "helper" is not really needed/meaningful and shouldn't be used in the one-line summary. (However, it is a good flag that maybe the method should possibly be protected? Check in each case before making that change.)

Be sure to provide an interdiff when creating a new patch. dawehner will be pushing a branch to the sandbox shortly with just the patch for this issue.

I'll have some more meaningful review and improve the issue summary a bit in an hour or so. :)

+++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
@@ -0,0 +1,385 @@
+   * @todo - this should really only be called as part of the flow of
+   * deleting a menu entity, so maybe we should load it and make sure it's
+   * not locked?

+++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
@@ -0,0 +1,218 @@
+ * @todo - add getter methods and make all properties protected.
+ * @todo - define an interface instead of using the concrete class to type hint.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -0,0 +1,1403 @@
+   * @todo - inject this from the plugin manager?
...
+    // @todo - this is probably unneeded.
...
+    // @todo - should we just return here if the link values match the original
+    //   values completely?.
...
+      // @todo - we want to also check $original['has_children'] here, but that
+      //   will be 0 even if there are children if those are hidden.
+      //   has_children is really just the rendering hint. So, we either need
+      //   to define another column (has_any_children), or do the extra query.
...
+    // @todo - only allow loading by plugin definition properties.
...
+    // @todo - does this make more sense than using the system path?
...
+    // @todo - consider making this dynamic based on static::MAX_DEPTH
+    //   or from the schema if that is generated using static::MAX_DEPTH.
...
+      // @todo - cache this result in memory if we find it's being used more
+      //   than once per page load.
...
+    // @todo - go back to tracking in state or some other way
+    //   which menus have expanded links?
...
+    // @todo - may be able to skip hashing after https://drupal.org/node/2224847
...
+        // @todo - test this index for effectiveness.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
@@ -0,0 +1,252 @@
+   * @todo give this a better name.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -0,0 +1,387 @@
+    // @todo use a link field in the end? see https://drupal.org/node/2235457

+++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
@@ -0,0 +1,234 @@
+    // @todo: Flag this call if possible so we don't call the menu tree manager.

All these @todos should be reformatted slightly to match our standards:
https://www.drupal.org/node/1354#todo

They should start with a capital letter, there should be no hyphen, they should be the last thing in any docblock other than an annotation, the subsequent lines should be intented by two characters, they should wrap before but as close as possible to 80 characters.

Most importantly, though, each @todo should either be fixed in the patch, or should have a followup issue to address it. Make sure the link to the followup issue is referenced in the @todo, and also add this as the parent issue on the followup issue itself. Work with @pwolanin or @dawehner to file followup issues about the problem (issues that will be clear to someone not familiar with this patch) or to fix them in the patch if it doesn't make sense to file a followup.

Let's also update the issue summary of this issue to explain the self-contained architecture we're adding in this patch a bit. Another sprinter could take this on. Start by looking at the parent issue and getting an idea of the high-level from there, then explain what this patch adds and what's important about it.

Pushed a branch called 2256521-step1-2301239 to the sandbox. All changes should be applied to the branch as well as the main one: 2256521

YesCT and I am going to work on doc and ToDo lists and see if we can work on it and open other issue if we need to.

I next reviewed the @param @return and @throws docs. Kind of rushed at the end a bit but there's plenty here.

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -0,0 +1,218 @@
   +   * @param bool $title_attribute
   +   *   If TRUE, add the link description (if present) as the title attribute.
   ...
   +  public function getUrlObject($title_attribute = TRUE);
   

   This should indicate that it is optional and defaults to TRUE.

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -0,0 +1,218 @@
   +   * @param bool $persist
   +   *   TRUE to have the link persist the changed values to any additional
   +   *   storage.
   ...
   +  public function updateLink(array $new_definition_values, $persist);
   

   Is there a reason we don't provide a default for $persist here?

3. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -0,0 +1,385 @@
   +  /**
   +   * Performs extra processing on plugin definitions.
   +   *
   +   * By default we add defaults for the type to the definition. If a type has
   +   * additional processing logic they can do that by replacing or extending the
   +   * method.
   +   */
   +  protected function processDefinition(&$definition, $plugin_id) {
   +    $definition = NestedArray::mergeDeep($this->defaults, $definition);
   +    $definition['parent'] = (string) $definition['parent'];
   +    $definition['id'] = $plugin_id;
   +  }
   

   This protected method is missing parameter documentation.

4. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -0,0 +1,385 @@
   +  /**
   +   * Instantiate if necessary and return a YamlDiscovery instance.
   +   *
   +   * Since the discovery is very rarely used - only when the rebuild() method
   +   * is called - it's instantiated only when actually needed instead of in the
   +   * constructor.
   +   */
   +  protected function getDiscovery() {
   +    if (empty($this->discovery)) {
   +      $yaml = new YamlDiscovery('menu_links', $this->moduleHandler->getModuleDirectories());
   +      $this->discovery = new ContainerDerivativeDiscoveryDecorator($yaml);
   +    }
   +    return $this->discovery;
   +  }
   

   This protected method is missing return value documentation.

5. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -0,0 +1,385 @@
   +  /**
   +   * Helper function to delete a specific instance.
   +   */
   +  protected function deleteInstance(MenuLinkInterface $instance, $persist) {
   +    $id = $instance->getPluginId();
   +    if ($instance->isDeletable()) {
   +      if ($persist) {
   +        $instance->deleteLink();
   +      }
   +    }
   +    else {
   +      throw new PluginException(sprintf("Menu link plugin with ID %s does not support deletion", $id));
   +    }
   +    $this->treeStorage->delete($id);
   +  }
   

   This protected method is missing parameter and @throws documentation.

6. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
   @@ -0,0 +1,168 @@
   +   * @param string $menu_name
   +   *   (optional) The menu name to count by, defaults to NULL.
   

   And what happens when it's null? Does it count links for all menus?

7. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
   @@ -0,0 +1,168 @@
   +   * @return array
   +   *   An ordered array of IDs representing the path to the root of the tree.
   +   *   The first element of the array will be equal to $id, unless $id is not
   +   *   valid, in which case the return value will be NULL.
   

   A sample array structure would be helpful here.

8. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
   @@ -0,0 +1,218 @@
   +  /**
   +   * A menu link plugin ID that should be used as the root.
   +   *
   +   * By default the "real" root (@code '' @encode) of a menu is used. But, when
   +   * only the descendants (subtree) of a certain menu link are needed, a custom
   +   * root can be specified.
   +   *
   +   * @var string
   +   */
   +  public $root = '';
   ...
   +   * @param string $root
   +   *   A menu link plugin ID, or @code '' @endcode to use the "real" root.
   ...
   +  public function setRoot($root) {
   

   I don't think specifying @code/@encode inline like this works for api.d.o. I also don't know what this means.

   I guess this second instance clarifies it a little. I'd say "or an empty string to use the real root" instead.

9. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
   @@ -0,0 +1,218 @@
   +   * @param string|NULL $operator
   +   *   The comparison operator, such as =, <, or >=. It also accepts more
   +   *   complex options such as IN, LIKE, or BETWEEN.
   ...
   +  public function addCondition($definition_field, $value, $operator = NULL) {
   

   Looks like the argument is optional and defaults to NULL. The docs should be updated to indicate that. And what happens when it's null?

10. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +   * @return array
    +   *   The names of the menus affected by the save operation (1 or 2).
    

    What does "1 or 2" mean here?

11. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +  /**
    +   * Saves menu links recursively.
    +   */
    +  protected function saveRecursive($id, &$children, &$links) {
    

    This protected method is missing parameter documentation.

12. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +   * @param MenuTreeParameters $parameters
    

    I think our standards indicate that MenuTreeParameters needs to be fully namespaced here, even if it's the current namespace or used in the file.

13. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +   *   The parameters to determine which menu links to be loaded into a tree.
    +   *   ::loadLinks() will set the absolute minimum depth, which is used
    +   *   ::doBuildTreeData().
    

    It's not clear which class these methods are on (the MenuTreeParameters parameter or this class). Should "::loadLinks()" simply be "this method"? Also, let's put the appropriate class name on ClassNameHere::doBuildTreeData().

14. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +   * @param array $tree
    +   *   The menu tree you wish to operate on.
    +   * @param array $definitions
    +   *   An array to accumulate definitions.
    ...
    +   * @param array $tree
    +   *   The menu link tree.
    +   * @param array &$definitions
    +   *   The collected definitions.
    

    These could use more detail. Where should I look to see what a "menu tree" array looks like? What is the structure of the array of accumulated definitions (which is updated by reference, it appears)? And why is it being passed by reference?

15. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +   *   See ::loadTreeData() for a sample query.
    

    Same as above. Let's add the class name here, and also add a proper @see at the bottom of the docblock.

16. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1403 @@
    +  /**
    +   * Helper function to determine serialized fields.
    +   */
    +  protected function serializedFields() {
    ...
    +    return $this->serializedFields;
    ...
    +  /**
    +   * Helper function to determine fields that are part of the plugin definition.
    +   */
    +  protected function definitionFields() {
    +    return $this->definitionFields;
    +  }
    ...
    +  /**
    +   * Defines the schema for the tree table.
    +   */
    +  protected static function schemaDefinition() {
    ...
    +    return $schema;
    

    These protected methods are missing return value documentation.

17. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,252 @@
    +   * @return array|FALSE
    +   *   Menu Link definition
    

    "The menu link definition, or FALSE if..."

    what definitions? Is it MenuLinkInterface plugin definitions? (Everywhere we talk about definitions we should clarify that; see multiple other places in the patch.)

18. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,252 @@
    +   * @return array
    +   *   An array of menu Link definitions.
    

    Weird capitalization. Also, what definitions? Is it MenuLinkInterface plugin definitions? (Everywhere we talk about definitions we should clarify that; see multiple other places in the patch.)

19. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,252 @@
    +   * Saves a plugin definition to the storage.
    ...
    +   * @return array
    +   *   The names of the menus affected by the save operation (1 or 2).
    ...
    +  public function save(array $definition);
    

    Wait a second, I've seen this "1 or 2" before. Is something else somewhere duplicating this documentation that should be @inheritdoc?

20. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,252 @@
    +   * @param int $max_relative_depth
    +   *   The maximum relative depth of the children relative to the passed parent.
    ...
    +  public function loadAllChildren($id, $max_relative_depth = NULL);
    ...
    +   * @param int $max_relative_depth
    +   *   The maximum depth of child menu links relative to the passed in.
    ...
    +  public function loadSubtreeData($id, $max_relative_depth = NULL);
    

    Looks like it is optional and defaults to NULL. And what happens when it's NULL?

    Also, nitpick: It looks like this might be 81 characters. (I've noticed this a couple other places as well but forgot to comment.) Double-check.

21. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,252 @@
    +   * @param string $menu_name
    +   *   (optional) The menu name to count by, defaults to NULL.
    ...
    +  public function countMenuLinks($menu_name = NULL);
    

    What happens if it's NULL? Also I'm having déja vu again... is this documentation repeated somewhere else?

22. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php
    @@ -0,0 +1,83 @@
    +   * @return array|NULL
    +   *   An override with following supported keys:
    +   *     - parent
    +   *     - weight
    +   *     - menu_name
    +   *     - expanded
    +   *     - hidden
    

    Or NULL if.... what?

23. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
    @@ -0,0 +1,147 @@
    +   * @return string|NULL
    +   *   Returns the route name, unless it is an internal link.
    

    When is it NULL? If it's an internal link?

24. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
    @@ -0,0 +1,147 @@
    +   * @param array $route_parameters
    +   *   The route parameters
    

    What route parameters? What is the structure of the array? Reference what the structure of this array is somewhere, or some other documentation for it. Also missing a period.

25. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessController.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Returns the access manager.
    +   *
    +   * @return \Drupal\Core\Access\AccessManager
    +   *   The route provider.
    +   */
    

    Looks like "The route provider" on the @return description is not correct.

Note - this issue includes a bug fix to PluginManagerPass because it was trying to add a method call addCachedDiscovery even through the MenuLinkManager does not implement \Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface

so, even though this looks unrelated, it's needed as part of the patch.

Small change to fix one comment

Added #15 to the sandbox branch

Thanks @victoru!

So, this patch adds:

• Three services: a menu link manager service, a menu cache service, and a menu tree storage service
• A menu link plugin interface, base class, and default implementation.
• An implementation for static configuration overrides of menu links.
• A menu link plugin manager
• An API for interacting with menu tree parameters (depth, parentage, etc.), as well as an interface for the storage of the menu tree data and a default implementation.
• A menu_link_content.module that allows administrators to create menu links. (Note: in a review of this patch yesterday, Dries asked that we provide a better user-facing label for this module, probably "Custom menu links" or similar. Need to implement that change.)
• Test coverage for menu_link_content.module. (Do we need unit tests for the other things?)

Is all of that correct?

Another question:

+++ b/core/lib/Drupal/Core/Plugin/PluginManagerPass.php
@@ -23,7 +23,9 @@ public function process(ContainerBuilder $container) {
-        $cache_clearer_definition->addMethodCall('addCachedDiscovery', array(new Reference($service_id)));
+        if (is_subclass_of($definition->getClass(), '\Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface')) {
+          $cache_clearer_definition->addMethodCall('addCachedDiscovery', array(new Reference($service_id)));
+        }

Why is this change necessary here? Edit: #13 totally answers this:

Note - this issue includes a bug fix to PluginManagerPass because it was trying to add a method call addCachedDiscovery even through the MenuLinkManager does not implement \Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface

so, even though this looks unrelated, it's needed as part of the patch.

We are making a lot of progress at the sprint - please check with me in IRC if you are looking at this.

Yep. kgoel and I are (with guidance from pwolanin) going through some of the cleanups xjm posted earlier.

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

Hm, this docblock actually looks entirely wrong? It's an interface.

Miscellaneous other review/questions:

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
   @@ -0,0 +1,168 @@
   +/**
   + * Defines an interface for creating menu links and retrieving menu link trees.
   + */
   
   +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    parent::postSave($storage, $update);
   +
   +    /** @var \Drupal\Core\Menu\MenuLinkManagerInterface $menu_link_manager */
   +    $menu_link_manager = \Drupal::service('plugin.manager.menu.link');
   +
   +    // The menu link can just be updated if there is already an menu link entry
   +    // on both entity and menu link plugin level.
   +    if ($update && $menu_link_manager->getDefinition($this->getPluginId())) {
   +      // When the entity is saved via a plugin instance, we should not call
   +      // the menu tree manager to update the definition a second time.
   +      if (!$this->insidePlugin) {
   +        $menu_link_manager->updateLink($this->getPluginId(), $this->getMenuDefinition(), FALSE);
   +      }
   +    }
   +    else {
   +      $menu_link_manager->createLink($this->getPluginId(), $this->getMenuDefinition());
   +    }
   +  }
   

   So, I had to read this method several times before it made sense. The updateLink() and createLink() methods on the plugin manager aren't actually creating a menu link item -- the CRUD operation that invoked this already did that. What they are doing, if I understood @effulgentsia's explanation, is "pushing" their new or updated definitions onto the plugin manager, integrating the content link plugin definitions with definitions discovered in yaml etc. and allowing them to be materialized together with those links. This way the whole discovery doesn't need to be re-done every time a content link is added. (Menu links don't use derivatives for their definitions because of the sheer quantity of definitions that are needed, which is orders of magnitude greater than e.g. custom block definitions.) Right?

   I wonder if there's some way we could improve both the documentation of this method and the class docblocks for the manager to explain this a little better, since I would have been completely at a loss if @effulgentsia hadn't explained it. Also, I think the method names are maybe misleading? Like it seems like they should be createLinkDefinition() updateLinkDefinition() etc., or something, to disambiguate it from actual CRUD operations done by the content menu link module or other implementations.

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
   @@ -0,0 +1,168 @@
   +  /**
   +   * Loads all parent link IDs of a given menu link.
   +   *
   +   * This method is very similar to getActiveTrailIds() but allows the link
   +   * to be specified rather than being discovered based on the menu name
   +   * and request. This method is mostly useful for testing.
   ...
   +  /**
   +   * Loads all child link IDs of a given menu link, regardless of visibility.
   +   *
   +   * This method is mostly useful for testing.
   ...
   +  public function getChildIds($id);
   ...
   +  /**
   +   * Resets any local definition cache. Used for testing.
   +   */
   +  public function resetDefinitions();
   
   +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
   @@ -0,0 +1,252 @@
   +  /**
   +   * Helper function for testing. Clears all definitions cached in memory.
   +   */
   +  public function resetDefinitions();
   
   +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * Force all overrides to be re-loaded from storage. Useful for testing.
   +   */
   +  public function reload();
   

   It seems odd for a public method to be explained as "useful for testing"? OTOH I guess a method needed for testing might have to be public... but it still seems odd for that to be the only reason the methods exist. Maybe I'm overthinking it.

3. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
   @@ -0,0 +1,161 @@
   +  /**
   +   * Encodes the ID by replacing dots with double underscores.
   +   *
   +   * This is done because config schema uses dots for its internal type
   +   * hierarchy.
   +   *
   +   * @param string $id
   +   *   The menu plugin ID.
   +   *
   +   * @return string
   +   *   The menu plugin ID with double underscore instead of dots.
   +   */
   +  protected static function encodeId($id) {
   +    return str_replace('.', '__', $id);
   +  }
   

   I don't see a corresponding decodeId() method? Why is that never needed? I guess because we never need to go from an override to the original link, since an override can't exist without an original link?

   Also, what happens when an ID actually contains a double underscore? Seems like a totally legitimate case that would make overrides blow up. Can/should we throw an exception if there's a double underscore since it would be incompatible with this?

   Given the issues that have cropped up with dots, it seems like this should have test coverage somewhere. Mostly the patch provides interfaces and plugin functionality that maybe doesn't make sense to test on its own, but I'd say at least an integration test is in order once the configuration system is actually exercised in step 4. (Need to check whether that specific test coverage is already there or not.)

   The exception for __ would want test coverage too, if it's appropriate.

4. +++ b/core/modules/menu_link_content/menu_link_content.info.yml
   @@ -0,0 +1,6 @@
   +name: 'Menu Link Content'
   

   Per discussion with Dries, let's change this to "Custom Menu Links".

5. +++ b/core/modules/menu_link_content/menu_link_content.info.yml
   @@ -0,0 +1,6 @@
   +description: 'Allows administrators to create custom links'
   

   Nitpick: This should end in a period.

6. +++ b/core/modules/menu_link_content/menu_link_content.module
   @@ -0,0 +1,17 @@
   +<?php
   +
   +/**
   + * @file
   + * Enables users to create menu link content.
   + */
   

   Minor, but I think the description from the .info.yml is better and could be reused here.

7. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   +   * Builds up the menu link plugin definition for this entity.
   ...
   +  protected function getMenuDefinition() {
   

   If it builds the menu link definition, why is it called getMenuDefinition()?

re: #12.18 the definition are not actually specific to menu link plugins - I think the storage could be potentially used for books as well.

re: #12.19, the similar doxygen is on a protected menthod on MenuTreeStorage

re: code/endcode, there are lots of example in core:

core/includes/bootstrap.inc:666: * @code t($text); @endcode, unless the text that the variable holds has been

re: code/endcode, there are lots of example in core:

core/includes/bootstrap.inc:666: * @code t($text); @endcode, unless the text that the variable holds has been

Yes, and look how the rendering for that is goofed up:
https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...

@code/@endcode are not just like <code> tags; they make a code block.

Here's an interdiff showing what @kgoel, @YesCT, and @pwolanin have improved so far in the sandbox.

A few nitpicky fixes. Meanwhile:

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -218,29 +226,25 @@ public function getInstance(array $options) {
      /**
       * Helper function to delete a specific instance.
   +   *
   +   * @throws
       */
   

   It throws... something! :)

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -188,30 +197,32 @@ public function deleteLink();
   +   * @return array|null
   +   *   An array with keys route_name and route_parameters, or NULL if there is
   +   *   no route.
   ...
   +   * @return array|null
   +   *   An array with keys route_name and route_parameters, or NULL.
   ...
   +   *   An array with keys route_name and route_parameters, or NULL if there is
   +   *   no route.
   

   Under what circumstances would there be no route?

RE: #23.1 regarding createLink() and updateLink()

these truly do create and update menu link plugins, and are used when the plugin implementation does not participate in discovery.

However, based on discussion renaming to addDefinition(), updateDefinition(), and removeDefinition()

We think all the things brought up in #5, #6, #12, #14, #22, #23 1., #25, #28 have been updated (in the sandbox). A patch (and an interdiff) coming here soon.
#23 2. Is fine the way it is. They need to be public, and the comments are accurate and useful about ... how they are useful. :)
#23 3. about the __ encode thing. fixed by also replacing __ with ___ (which works since that makes three turn into four and four turn into six etc.)

php -r "echo strtr('. lll __', array('.' => '__', '__' => '___'));"

#23 4-7 also are addressed.

Ian made draft change notice - we'll need to put most of the detail in the handbook, however.

updated issue summary to update the proposed statements in #18, with some corrections. it's correct in the issue summary now.

started change notice (need this for step 4) https://www.drupal.org/node/2302069

Here's an incremental diff since the 1st patch at #2 and the #1 patch relative to 8.x and the full patch for step #5

So the first half of my probably-final review; down through MenuLinkParameters. I've tried to indicate with parentheticals what comments needn't block an RTBC/commit (though feel free to clean them up if they're simple, particularly the docs nitpicks). For the stuff I haven't indicated that way, though, it would be good to post a reply on issue if appropriate.

I'll do the second half of the patch in a bit.

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
   @@ -0,0 +1,199 @@
   +  /**
   +   * The list of definition values where an override is allowed.
   +   *
   +   * The keys are definition names. The values are ignored.
   +   *
   +   * @var array
   +   */
   +  protected $overrideAllowed = array();
   
   +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
   @@ -0,0 +1,87 @@
   +    $overrides = array_intersect_key($new_definition_values, $this->overrideAllowed);
   

   This seems odd. If we only care about a flat list of definition names, why don't we return a flat list of definition names? Is it to avoid having to use in_array()? (Doesn't need to block commit; just curious).

   Edit: I found why; added to the snippet. Because $new_definition_values is keyed by those key names, and so that works for the intersect here. It seems a little magical/hacky but I'm not too concerned.

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
   @@ -0,0 +1,199 @@
   +  /**
   +   * Returns the weight of the menu link.
   +   *
   +   * @return int
   +   *   The weight of the menu link, 0 by default.
   +   */
   +  public function getWeight() {
   

   Shouldn't this be @inheritdoc? (At first I was going to ask why it wasn't on the interface, but it is.) :)

3. +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
   @@ -0,0 +1,199 @@
   +  public function getTitle() {
   +    // Subclasses may pull in the request or specific attributes as parameters.
   +    $options = array();
   +    if (!empty($this->pluginDefinition['title_context'])) {
   +      $options['context'] = $this->pluginDefinition['title_context'];
   +    }
   +    $args = array();
   +    if (isset($this->pluginDefinition['title_arguments']) && $title_arguments = $this->pluginDefinition['title_arguments']) {
   +      $args = (array) $title_arguments;
   +    }
   +    return $this->t($this->pluginDefinition['title'], $args, $options);
   
   +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -0,0 +1,408 @@
   +    // The static title for the menu link.
   +    'title' => '',
   +    'title_arguments' => array(),
   +    'title_context' => '',
   

   Where are title_context and title_arguments documented?

   Also, I don't entirely follow the inline comment. It doesn't seem related to the code that follows it.

   Is it saying that specific implementations might rely on the request or specific attributes to define the title? (Along the lines of the examples given in the interface docs, i.e., a link title might include dynamic information).

   Edit: I found where the rest of the definition is documented, inline in the manager $defaults. I think that's a pattern used elsewhere (documenting the definition inline in the defaults), though I think we might want to find a better way to document all of it.... which would be okay as a followup.

   However, even leaving it as is, there still is no documentation of title_context or title_arguments here, even though there is for the rest of the definition keys. :)

4. +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
   @@ -0,0 +1,199 @@
   +    throw new PluginException(sprintf("Menu link plugin with ID %s does not support deletion", $this->getPluginId()));
   
   +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -0,0 +1,408 @@
   +      throw new PluginException(sprintf("Menu link plugin with ID %s does not support deletion", $id));
   ...
   +      throw new PluginException(sprintf('The ID %s already exists as a plugin definition or is not valid', $id));
   
   +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +        throw new PluginException(sprintf('The link with ID %s or its children exceeded the maximum depth of %d', $link['id'], $this->maxDepth()));
   

   Not a big deal, but: any particular reason we are falling back to sprintf() here rather than String::format()? (On my mind because of #1825952: Turn on twig autoescape by default, not that it actually would make a difference if this string gets escaped or not because I don't think plugin IDs can contain anything that would get escaped to begin with.)

   Or maybe the answer is "We're deliberately not adding a dependency on String. But I'm not sure that makes sense since plenty of other Drupal/Core/ classes do (including ones in this patch). :)

5. +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
   @@ -0,0 +1,87 @@
   +/**
   + * Provides a default implementation for menu link plugins.
   + */
   +class MenuLinkDefault extends MenuLinkBase implements ContainerFactoryPluginInterface {
   

   I'm wondering if a better name for this implementation might be StaticMenuLink? Would be more parallel with StaticMenuLinkOverride, which is closely related (we even inject the service and provide a default list of allowed overrides.)

6. +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
   @@ -0,0 +1,87 @@
   +  public function updateLink(array $new_definition_values, $persist) {
   +    $overrides = array_intersect_key($new_definition_values, $this->overrideAllowed);
   +    if ($persist) {
   +      $this->staticOverride->saveOverride($this->getPluginId(), $overrides);
   +    }
   +    // Update the definition.
   +    $this->pluginDefinition = $overrides + $this->getPluginDefinition();
   +    return $this->pluginDefinition;
   +  }
   

   Maybe an inline comment above this array_intersect_key() would be good? "Filter the list of overrides to only those that are allowed."

   So this is where we have the behavior of silently ignoring disallowed overrides, and where we would throw an exception if we decided that was better DX. What's the usecase for letting calling code pass in disallowed overrides and silently ignoring them?

7. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -0,0 +1,231 @@
   +   * Updates the definition values for a menu link.
   +   *
   +   * Depending on the implementation details of the class, not all definition
   +   * values may be changed. For example, changes to the title of a static link
   +   * will be discarded.
   

   (Okay as a followup) Should this throw an exception if the link is not updateable, like we throw an exception if it's not deletable?

8. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -0,0 +1,231 @@
   +   * @throws \Drupal\Component\Plugin\Exception\PluginException
   +   *   If the link is not deletable.
   

   (Okay as a followup) Should we throw a less generic exception here than PluginException?

9. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
   @@ -0,0 +1,231 @@
   +   * To instantiate the form class, use an instance of the
   +   * \Drupal\Core\DependencyInjection\ClassResolverInterface, such as from the
   +   * class_resolver service. Then call the setMenuLinkInstance() method on the
   +   * form instance with the menu link plugin instance.
   

   (Okay as a followup) This would be easier to understand with sample code; is there an implementation we could add an @see for?

10. +++ b/core/lib/Drupal/Core/Menu/MenuLinkInterface.php
    @@ -0,0 +1,231 @@
    +  /**
    +   * Returns parameters for a delete link.
    +   *
    +   * @return array|null
    +   *   An array with keys route_name and route_parameters, or NULL if there is
    +   *   no route (e.g. when the link is not deletable).
    +   */
    +  public function getDeleteRoute();
    ...
    +  /**
    +   * Returns parameters for a custom edit link.
    +   *
    +   * Plugins should return a value here if they have a special edit form, or if
    +   * they need to define additional local tasks, local actions, etc. that are
    +   * visible from the edit form.
    +   *
    +   * @return array|null
    +   *   An array with keys route_name and route_parameters, or NULL if there is
    +   *   no route because there is no custom edit route for this instance.
    +   */
    +  public function getEditRoute();
    ...
    +  /**
    +   * Returns parameters for a translate link.
    +   *
    +   * @return array
    +   *   An array with keys route_name and route_parameters, or NULL if there is
    +   *   no route (e.g. when the link is not translatable).
    +   */
    +  public function getTranslateRoute();
    

    I think these should be "Returns route name and parameters", not just "parameters"?

    Also, the word "link" in the summary is ambiguous... I think we are talking about the link to delete a link, or the link to translate a link, etc.? If so maybe this would be more clear:

    Returns route information for the route to delete a link.

    Or something? What do you think?

11. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +    'expanded' => 0,
    +    'hidden' => 0,
    

    Why are these integers instead of boolean?

12. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +    // The plugin id. Set by the plugin system based on the top-level YAML key.
    

    (Docs standards nitpick) ID, not id.

13. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +   * Service providing overrides for static links
    

    (Docs standards nitpick) Missing period at the end of the line.

14. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +   * Constructs a \Drupal\Core\Menu\MenuLinkTree object.
    

    No, it constructs a MenuLinkManager object. Right?

15. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +   * By default we add defaults for the type to the definition. If a type has
    +   * additional processing logic they can do that by replacing or extending the
    +   * method.
    

    (Docs standards nitpick) "They" is ungrammatical; it should maybe instead be:

    If a type has additional processing logic, the logic can be added by replacing or extending the method.

16. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +    $definition['parent'] = (string) $definition['parent'];
    

    Why the string casting? To turn NULL into empty string, or? Comment wouldn't be amiss.

17. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +    // If this plugin was provided by a module that does not exist, remove the
    +    // plugin definition.
    +    foreach ($definitions as $plugin_id => $plugin_definition) {
    +      if (!empty($plugin_definition['provider']) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {
    +        unset($definitions[$plugin_id]);
    +      }
    +    }
    +    return $definitions;
    

    So, hm. Is there a reason we're silently ignoring invalid plugin definitions here?

18. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +      throw new PluginNotFoundException("$plugin_id could not be found.");
    

    (Okay as followup) I think this should probably use String::format() instead of printing the variable straight in the string. (Also see comment elsewhere about sprintf() use in an exception message.)

19. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
    @@ -0,0 +1,408 @@
    +   * Returns a pre-configured meu link plugin instance.
    

    (Docs standards nickpick) Typo: "meu".

    La vache dit meu!

20. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
    @@ -0,0 +1,183 @@
    + * Defines an interface for creating menu links and storing their definitions.
    + */
    +interface MenuLinkManagerInterface extends PluginManagerInterface {
    ...
    +   * This is used for plugins not found through discovery to remove definitions.
    ...
    +  public function removeDefinition($id, $persist = TRUE);
    ...
    +   * Use this function when you know there is no entry in the tree. This is
    +   * used for plugins not found through discovery to add new definitions.
    ...
    +  public function addDefinition($id, array $definition);
    ...
    +  public function updateDefinition($id, array $new_definition_values, $persist = TRUE);
    

    This is all much easier to understand. Should updateDefinition() have a similar note about being used for plugins not found through discovery?

    Also, I still think a paragraph on the interface docblock explaining that plugins can use these three methods as an alterative to participating in discovery would be good.

    Finally, maybe the interface summary should say "managing menu links and storing their definitions" rather than "creating"?

21. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
    @@ -0,0 +1,183 @@
    +  /**
    +   * Triggers discovery, save, and cleanup of static links.
    +   */
    +  public function rebuild();
    

    Is it worth mentioning here that it includes applying overrides? (Which it appears to in the main implementation.)

22. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
    @@ -0,0 +1,183 @@
    +   * @param array $route_parameters
    +   *   (optional) The route parameters. Defaults to an empty array.
    

    I thought I pointed this out before, but maybe not. Is it "The route parameters, if any"? I.e., not all routes have parameters.

    What would happen if I passed a route name for a route that needed parameters, but no parameters? (Maybe this isn't a relevant question.)

23. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
    @@ -0,0 +1,183 @@
    +   *   TRUE to also have the link instance itself persist the changed values to
    +   *   any additional storage by invoking MenuLinkInterface::updateDefinition() on
    +   *   the plugin that is being updated.
    

    (Docs standards nitpick) This is going over 80 characters.

24. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
    @@ -0,0 +1,183 @@
    +   * @throws \Drupal\Component\Plugin\Exception\PluginException
    +   *   Thrown if the $id is not a valid, existing, plugin ID or if the link
    +   *   cannot be reset.
    

    (Okay as a followup) Same question; do we want a less generic exception?

25. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManagerInterface.php
    @@ -0,0 +1,183 @@
    +  /**
    +   * Loads all parent link IDs of a given menu link.
    +   *
    +   * This method is very similar to getActiveTrailIds() but allows the link to
    +   * be specified rather than being discovered based on the menu name and
    +   * request. This method is mostly useful for testing.
    +   *
    +   * @param string $id
    +   *   The menu link plugin ID.
    +   *
    +   * @return array
    +   *   An ordered array of IDs representing the path to the root of the tree.
    +   *   The first element of the array will be equal to $id, unless $id is not
    +   *   valid, in which case the return value will be NULL.
    +   */
    +  public function getParentIds($id);
    +
    +  /**
    +   * Loads all child link IDs of a given menu link, regardless of visibility.
    +   *
    +   * This method is mostly useful for testing.
    +   *
    +   * @param string $id
    +   *   The menu link plugin ID.
    +   *
    +   * @return array
    +   *   An unordered array of IDs representing the IDs of all children, or NULL
    +   *   if the ID is invalid.
    +   */
    +  public function getChildIds($id);
    

    So, I can totally imagine myself using these methods in a module that does fancy things with menus, not just for testing. Or would I be abusing the API if I did?

26. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
    @@ -0,0 +1,222 @@
    +/**
    + * Provides a value object to model menu tree parameters.
    + *
    + * Menu tree parameters are used to determine the set of definitions to be
    + * loaded from \Drupal\Core\Menu\MenuTreeStorageInterface. Hence they determine
    + * the shape and content of the tree:
    + * - which parent IDs should be used to restrict the tree, i.e. only links with
    + *   a parent in the list will be included.
    + * - which menu links are omitted, i.e. minimum and maximum depth
    + *
    + * @todo Add getter methods and make all properties protected and define an
    + *   interface instead of using the concrete class to type hint.
    + *   https://www.drupal.org/node/2302041
    + */
    +class MenuTreeParameters {
    

    This docblock is hugely helpful. +1.

27. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
    @@ -0,0 +1,222 @@
    +  /**
    +   * An array of parent link IDs. This restricts the tree to only menu links
    +   * that are at the top level or have a parent ID in this list. If empty, the
    +   * whole menu tree is built.
    +   *
    +   * @var string[]
    +   */
    +  public $expandedParents = array();
    

    (Docs standards nitpick) This docblock is missing a one-line summary. I think we could probably just insert the blank line between the first sentence and the rest of the paragraph.

28. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
    @@ -0,0 +1,222 @@
    +  /**
    +   * An array of menu link plugin IDs, representing the trail from the currently
    +   * active menu link to the ("real") root of that menu link's menu.  This does
    +   * not affect the way the tree is built, it only is used to set the value of
    +   * the inActiveTrail property for each tree element.
    +   *
    +   * Defaults to the empty array.
    +   *
    +   * @var string[]
    +   */
    +  public $activeTrail = array();
    +
    +  /**
    +   * An associative array of custom query condition key/value pairs to restrict
    +   * the links loaded.
    +   *
    +   * Defaults to the empty array.
    +   *
    +   * @var array
    +   */
    +  public $conditions = array();
    

    (Docs standards nitpick) These also need one-line summaries, as above.

didn't need a fix: #35 1

fixed: #35 2,

#35 3: the array that defines menu links is defined in two places,
on the $defaults property on MenuLinkManager, and MenuTreeStorage. both of those places link to #2302085: Keep MenuTreeStorage field definition in sync with MenuLinkManager which is to figure out how to keep them in sync (but not necessarily how to make them documented and obvious.) Some suggestions were:
a) it would be nice if php allowed a const that was an array, then we could put it on the interface and reference that.
b) we could make a method that returns the array
c) .. I forget.

This is a pattern we use already in core, so if we make an issue to figure out how to make the bits documented, we should then apply *that* pattern to the other places this default property on classes is, like: LocalActionManager

I didn't change anything here.

#35 4. why using sprintf instead of String::format()...
I'm not sure, but I will say:
ag "throw .*Exception" core/modules | grep format | wc -l
40
ag "throw .*Exception" core/modules | grep sprintf | wc -l
7
ag "throw .*Exception" core/lib/Drupal | grep format | wc -l
57
ag "throw .*Exception" core/lib/Drupal | grep sprintf | wc -l
100

I didn't change anything here.

----
moving locations. back in a bit.

Thanks @YesCT.

#35 3: the array that defines menu links is defined in two places,
on the $defaults property on MenuLinkManager, and MenuTreeStorage. both of those places link to #2302085: Keep MenuTreeStorage field definition in sync with MenuLinkManager which is to figure out how to keep them in sync (but not necessarily how to make them documented and obvious.) Some suggestions were:
a) it would be nice if php allowed a const that was an array, then we could put it on the interface and reference that.
b) we could make a method that returns the array
c) .. I forget.

This is a pattern we use already in core, so if we make an issue to figure out how to make the bits documented, we should then apply *that* pattern to the other places this default property on classes is, like: LocalActionManager

I didn't change anything here.

My point is though that at a minimum we should add an inline comment explaining what title_context and title_arguments are, like there already is for the other definition keys, because nothing anywhere gives the developer a clue what their purpose is. The docmentation-in-defaults pattern exists elsewhere, but we are documenting some but not all of the definition keys. That's a docs gate concern.

Edit: also the point about the inline comment at the top of the hunk still stands, I think. The method's code needs to be better documented, as do the title definition keys used in it.

Phew. Just reviewed the storage code. Heavy stuff. I'll review the remaining two implementations in a bit.

1. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +/**
   + * Provides a tree storage using the database.
   + */
   +class MenuTreeStorage implements MenuTreeStorageInterface {
   

   (Docs nitpick) Not a big deal, but I guess this should probably be "Provides a menu tree storage using the database."

   Have I mentioned yet that I really like the way we've abstracted the storage?

   I read every line of this class but didn't do that much code-parsing or deep review of some of the protected methods, since I am sure this is mostly taken from our existing menu management API.

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +  /**
   +   * List of plugin definition fields.
   +   *
   +   * @todo Decide how to keep these field definitions in sync.
   +   *   https://www.drupal.org/node/2302085
   +   *
   +   * @var array
   +   */
   +  protected $definitionFields = array(
   +    'menu_name',
   +    'route_name',
   +    'route_parameters',
   +    'url',
   +    'title',
   +    'title_arguments',
   +    'title_context',
   +    'description',
   +    'parent',
   +    'weight',
   +    'options',
   +    'expanded',
   +    'hidden',
   +    'provider',
   +    'metadata',
   +    'class',
   +    'form_class',
   +    'id',
   +  );
   

   So how about, for now, we add an @see to MenuLinkManager::$defaults for an explanation of the keys?

3. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +    // Remove all such items. Starting from those with the greatest depth will
   +    // minimize the amount of re-parenting done by the menu link controller.
   +    if ($result) {
   +      $this->purgeMultiple($result);
   +    }
   

   I think maybe the second sentence of this inline comment might now belong inside purgeMultiple() (if it's even still relevant)? Can someone clarify? Because the code here certainly isn't doing what that sentence says by itself.

   Also, ixnay on the word "controller", I think. We've (mostly) made "controller" refer only to page/form controllers which doesn't make sense in this context. What is it that actually does the re-parenting?

4. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +  /**
   +   * Executes a select query while making sure the database table exists.
   +   *
   +   * @param \Drupal\Core\Database\Query\SelectInterface $query
   +   *   The select object to be executed.
   +   *
   +   * @return \Drupal\Core\Database\StatementInterface|null
   +   *   A prepared statement, or NULL if the query is not valid.
   +   *
   +   * @throws \Exception
   +   *   Thrown if the table could not be created or the database connection
   +   *   failed.
   +   */
   +  protected function safeExecuteSelect(SelectInterface $query) {
   +    try {
   +      return $query->execute();
   +    }
   +    catch (\Exception $e) {
   +      // If there was an exception, try to create the table.
   +      if ($this->ensureTableExists()) {
   +        return $query->execute();
   +      }
   +      // Some other failure that we can not recover from.
   +      throw $e;
   +    }
   +  }
   

   Huh. This seems like a weird method to have on this specific storage; it's super generic. Why do we need it? And if the answer to that question makes sense, should there be a followup to move this to something more generic in the DB API somewhere?

5. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +   * @return array
   +   *   The menu names affected by the save operation (1 or 2 names).
   
   +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
   @@ -0,0 +1,256 @@
   +   * @return array
   

   So the docs are better here now, but I still want to know why it's only 1-2 names. :)

6. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +    asort($fields['route_parameters']);
   ...
   +    asort($route_parameters);
   

   (Okay as followup) Why? Could use an inline comment. (Assuming the code works because HEAD and the combined patch). ;)

7. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +    // Cast booleans to int, if needed.
   +    $fields['hidden'] = (int) $fields['hidden'];
   +    $fields['expanded'] = (int) $fields['expanded'];
   

   Ah, so this might be related to my earlier question of why the defaults for these were ints instead of booleans.

   (Docs nitpick) "Boolean" should be capitalized (it's from a dude's name).

8. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +   * Moves the link's children using the query fields value and original values.
   +   *
   +   * @param array $fields
   +   *   The changed menu link.
   +   * @param array $original
   +   *   The original menu link.
   +   */
   +  protected function moveChildren($fields, $original) {
   

   Uh. Moves the children where?

9. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -0,0 +1,1426 @@
   +  protected function updateParentalStatus(array $link) {
   

   (No change needed) OT: I heard one of these today. ;)

10. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1426 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function loadMultiple(array $ids) {
    +    $query = $this->connection->select($this->table, $this->options);
    +    $query->fields($this->table, $this->definitionFields());
    +    $query->condition('id', $ids, 'IN');
    +    $loaded = $this->safeExecuteSelect($query)->fetchAllAssoc('id', \PDO::FETCH_ASSOC);
    +    foreach ($loaded as $id => $link) {
    +      $loaded[$id] = $this->prepareLink($link);
    +    }
    +    return $loaded;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function load($id) {
    +    if (isset($this->definitions[$id])) {
    +      return $this->definitions[$id];
    +    }
    +    $loaded = $this->loadMultiple(array($id));
    +    return isset($loaded[$id]) ? $loaded[$id] : FALSE;
    +  }
    +
    

    Why does load() but not loadMultiple() return early with definitions from $this->definitions?

    I also notice that we're not statically caching the loaded links like we do in a lot of entity storages. Probably it's a case of "Don't add static caches until you have proof you need them" but I thought it was worth pointing out.

11. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1426 @@
    +  /**
    +   * Loads all table fields, not just those that are in the plugin definition.
    +   *
    +   * @param string $id
    +   *   The menu link ID.
    +   *
    +   * @return array
    +   *   The loaded menu link definition or an empty array if not be found.
    +   */
    +  protected function loadFull($id) {
    +    $loaded = $this->loadFullMultiple(array($id));
    +    return isset($loaded[$id]) ? $loaded[$id] : array();
    +  }
    +
    +  /**
    +   * Loads multiple menu link definitions by ID.
    +   *
    +   * @param array $ids
    +   *   The IDs to load.
    +   *
    +   * @return array
    +   *   The loaded menu link definitions.
    +   */
    +  protected function loadFullMultiple(array $ids) {
    +    $query = $this->connection->select($this->table, $this->options);
    +    $query->fields($this->table);
    +    $query->condition('id', $ids, 'IN');
    +    $loaded = $this->safeExecuteSelect($query)->fetchAllAssoc('id', \PDO::FETCH_ASSOC);
    +    foreach ($loaded as &$link) {
    +      foreach ($this->serializedFields() as $name) {
    +        $link[$name] = unserialize($link[$name]);
    +      }
    +    }
    +    return $loaded;
    +  }
    

    I think the one-line summary here for loadFullMultiple() should probably have the same clarification loadFull() does (that it's all table fields, not just those in the definition)?

12. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1426 @@
    +    // Build the cache id; sort 'expanded' and 'conditions' to prevent duplicate
    

    (Docs standards nitpick) "ID", not "id".

13. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1426 @@
    +      // When specifying a custom root, we only want to find links whose
    +      // parent IDs match that of the root; that's how ignore the rest of the
    +      // tree. In other words: we exclude everything unreachable from the
    +      // custom root.
    

    (Docs standards nitpick) There is a missing word after the semicolon: "that's how ignore the rest of the tree."

14. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1426 @@
    +   * @throws \Drupal\Component\Plugin\Exception\PluginException
    +   *   If a database error occurs.
    ...
    +      throw new PluginException($e->getMessage(), NULL, $e);
    

    (Okay for followup) Same question about whether we should use a more specific exception type.

15. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
    @@ -0,0 +1,1426 @@
    +  /**
    +   * Determines serialized fields in the storage.
    +   *
    +   * @return array
    +   *   A list of fields that are serialized in the database.
    +   */
    +  protected function serializedFields() {
    +    // For now, build the list from the schema since it's in active development.
    +    if (empty($this->serializedFields)) {
    +      $schema = static::schemaDefinition();
    +      foreach ($schema['fields'] as $name => $field) {
    +        if (!empty($field['serialize'])) {
    +          $this->serializedFields[] = $name;
    +        }
    +      }
    +    }
    +    return $this->serializedFields;
    +  }
    

    What does "in active development" mean in this inline comment?

16. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +/**
    + * Defines an interface for the menu tree storage.
    + */
    +interface MenuTreeStorageInterface {
    

    So I believe I wrote this docblock so it's my fault (there wasn't one before), but I think we should explain a little more what a "menu tree storage" is here.

17. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +  /**
    +   * Loads multiple plugin definitions from the storage based on route.
    +   *
    +   * @param string $route_name
    +   *   The route name.
    +   * @param array $route_parameters
    +   *   (optional) The route parameters. Defaults to an empty array.
    +   * @param string $menu_name
    +   *   (optional) Restricts the found links to just those in the named menu.
    +   *
    +   * @return array
    +   *   An array of menu link definitions keyed by ID and ordered by depth.
    +   */
    +  public function loadByRoute($route_name, array $route_parameters = array(), $menu_name = NULL);
    

    Same question as before about the route parameters array.

18. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +   *   - tree: A fully built menu tree.
    ...
    +   *   - subtree: A fully built menu tree element or FALSE.
    

    What kind of a thing is a fully built menu tree? Array? Object implementing interface FooInterface?

19. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +   *   An associative array of IDs with keys equal to values that represents the
    +   *   path from the given ID  to the root of the tree. If $id is an ID that
    +   *   exists, the returned array will at least include it.  An empty array is
    +   *   returned if the ID does not exist in the storage.
    

    Whoa. An example array would be really helpful here.

20. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +   * Finds the height of a subtree rooted by of the given ID.
    

    (Docs standards nitpick) "rooted by of".

21. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +   *   The the ID of an item in the storage.
    

    (Docs standards nitpick) "The the".

22. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorageInterface.php
    @@ -0,0 +1,256 @@
    +   * Determines whether a specific menu named is used in the tree.
    

    (Docs standards nitpick) I think this should be "A specific menu name" (not "named")?

1. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
   @@ -0,0 +1,162 @@
   +  /**
   +   * The config name used to store the overrides.
   +   *
   +   * @var string
   +   */
   +  protected $configName = 'menu_link.static.overrides';
   

   (No change needed) So all the overrides get stored in a single config file, correct? I think this makes sense but it also might be good to have @alexpott spot-check this.

2. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
   @@ -0,0 +1,162 @@
   +    // Remove unexpected keys.
   +    $expected = array(
   +      'menu_name' => 1,
   +      'parent' => 1,
   +      'weight' => 1,
   +      'expanded' => 1,
   +      'hidden' => 1,
   +    );
   

   What's the reason for silently ignoring bogus override keys? (It's probably fine; just want to check whether we should be throwing an exception instead.)

3. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
   @@ -0,0 +1,162 @@
   +  /**
   +   * Encodes the ID by replacing dots with double underscores.
   +   *
   +   * This is done because config schema uses dots for its internal type
   +   * hierarchy. Double underscores are converted to triple underscores to
   +   * avoid accidental conflicts.
   +   *
   +   * @param string $id
   +   *   The menu plugin ID.
   +   *
   +   * @return string
   +   *   The menu plugin ID with double underscore instead of dots.
   +   */
   +  protected static function encodeId($id) {
   +    return strtr($id, array('.' => '__', '__' => '___'));
   +  }
   

   Oh, I get it now. This is clever.

   It probably wants explicit test coverage at some point (probably in step 4?). I guess we can add "Needs tests" on that issue with a note about it.

4. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverridesInterface.php
   @@ -0,0 +1,85 @@
   +   * Forces all overrides to be reloaded from config storage to compare the
   +   * override value with the value submitted during test form submission.
   

   (Docs nitpick) This should have a one-line summary.

5. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   + *   label = @Translation("Menu link content"),
   

   I think this should probably be changed to "Custom menu link" as well? (Correct?)

6. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   + *   fieldable = TRUE,
   

   (No change needed) Whoa. They're fieldable? Whoa. Neat.

7. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   + *     "canonical" = "menu_link_content.link_edit",
   + *     "edit-form" = "menu_link_content.link_edit",
   

   (No change needed) Interesting that the canonical link is the edit link. I guess it wouldn't really make sense to have a page to view a single menu link entity. ;)

8. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   +    // Don't bother saving title and description strings, since they are never
   +    // used.
   +    $definition['title'] = $this->getTitle();
   +    $definition['description'] = $this->getDescription();
   

   The inline comment confuses me. The two lines following seem to not agree with it? I mean, we're not "saving" anything here either way.

9. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,387 @@
   +    // The form widget doesn't work yet for core fields, so we skip the
   +    // for display and manually create form elements for the boolean fields.
   ...
   +    // @see https://drupal.org/node/2150511
   +    $fields['expanded'] = FieldDefinition::create('boolean')
   +      ->setLabel(t('Expanded'))
   +      ->setDescription(t('Flag for whether this link should be rendered as expanded in menus - expanded links always have their child links displayed, instead of only when the link is in the active trail (1 = expanded, 0 = not expanded).'))
   +      ->setSetting('default_value', 0)
   +      ->setDisplayOptions('view', array(
   +        'label' => 'hidden',
   +        'type' => 'boolean',
   +        'weight' => 0,
   +      ));
   

   (No change needed) OT: Huh. Surprised that this still isn't fixed. :(

10. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    @@ -0,0 +1,387 @@
    +    $fields['hidden'] = FieldDefinition::create('boolean')
    +      ->setLabel(t('Hidden'))
    +      ->setDescription(t('A flag for whether the link should be rendered in menus. (1 = a disabled menu item that may be shown on admin screens, -1 = a menu callback, 0 = a normal, visible link).'))
    +      ->setSetting('default_value', 0);
    

    (Okay as followup) Why magic integers? :(

11. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContentInterface.php
    @@ -0,0 +1,152 @@
    +   * Sets the route parameters of the custom menu link.
    +   *
    +   * @param array $route_parameters
    +   *   The route parameters, usually derived from the path entered by the
    +   *   administrator. For example, for a link to a node with route 'node.view'
    +   *   the route needs the node ID as a parameter:
    +   *   @code
    +   *     array('node' => 2)
    +   *   @endcode
    +   *
    +   * @return $this
    +   */
    +  public function setRouteParameters(array $route_parameters);
    

    (No change needed) Yes, +1 for these docs.

12. +++ b/core/modules/menu_link_content/src/MenuLinkContentAccessController.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Returns the access manager.
    +   *
    +   * @return \Drupal\Core\Access\AccessManager
    +   *   The access manager.
    +   */
    +  protected function accessManager() {
    +    if (!$this->accessManager) {
    +      $this->accessManager = \Drupal::service('access_manager');
    +    }
    +    return $this->accessManager;
    +  }
    

    (Okay as followup) Why isn't this injected?

13. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
    @@ -0,0 +1,247 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected $overrideAllowed = array(
    +    'menu_name' => 1,
    +    'parent' => 1,
    +    'weight' => 1,
    +    'expanded' => 1,
    +    'hidden' => 1,
    +    'title' => 1,
    +    'description' => 1,
    +    'route_name' => 1,
    +    'route_parameters' => 1,
    +    'url' => 1,
    +    'options' => 1,
    +  );
    

    (No change needed) Interesting. So this is presumably to allow contrib to do whatever with content links?

14. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
    @@ -0,0 +1,247 @@
    +   * @throws \Drupal\Component\Plugin\Exception\PluginException
    ...
    +        throw new PluginException("Invalid entity ID or uuid");
    

    (Okay for followup) Exception. Generic. :)

15. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
    @@ -0,0 +1,247 @@
    +   *   If the entity ID and uuid are both invalid or missing.
    

    (Docs nitpick) I guess "UUID" should probably be uppercase in text, same as ID.

16. +++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
    @@ -0,0 +1,398 @@
    +class MenuTreeStorageTest extends KernelTestBase {
    

    (No change needed) For the most part I just assumed the tests are fine rather than trying to execute tree logic in my brain. (Look at my faith in you!)

17. +++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
    @@ -0,0 +1,398 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Menu tree storage tests',
    +      'description' => 'Tests menu tree storage tests',
    +      'group' => 'Menu',
    +    );
    +  }
    

    So, this needs to go away now, and be updated for: https://www.drupal.org/node/2301125

And that's all! Once all of these things from #35-#39 are addressed, I will RTBC this. ("Addressed" here might mean: fixed in patch, followup filed, or simply documenting on issue "this is fine as-is").

I am adding all the follow up issues which I created for the todo's and all the follow up issues are child of https://www.drupal.org/node/2256497 [meta] Menu Links - New Plan for the Homestretch

#35 5 rename MenuLinkDefault to StaticMenuLink?
I feel like there could have been something about default being able to be used for more than just "static"...

@pwolanin clarified that MenuLinkDefault is better than StaticMenuLink since
a) if you don't provide a class it's the defualt
b) it's parallel with other link things: Drupal\Core\Menu\LocalActionDefault, Drupal\Core\Menu\LocalTaskDefault

#35 6 and 7, An exception when the intersect of the values to change and values allowed to change seems like a good idea #2302581: When intersect of menu link new definition values attempts to update those not allowed to be overridden throw an exception but might be too complicated.
Added the inline comment about what that is doing also. More discussion should be on 2302581. Might rescope it.

#35 17 #2302623: Do something instead of silently ignoring invalid plugin in MenuLinkManager (also adding another related)

We are tracking responses to xjm in a spreadsheet

See PDF version below

Huh. This seems like a weird method to have on this specific storage; it's super generic. Why do we need it? And if the answer to that question makes sense, should there be a followup to move this to something more generic in the DB API somewhere?

Well, this executes ensureTableExists() so it is not that generic. Added an issue for a trait: #2302635: Add a schema ensuring trait

Ah, so this might be related to my earlier question of why the defaults for these were ints instead of booleans.

Well, ANSI SQL does not really define a boolean type, so small integer is used.

(Docs nitpick) "Boolean" should be capitalized (it's from a dude's name).

I bet this is not done anywhere else.

Why does load() but not loadMultiple() return early with definitions from $this->definitions?

I also notice that we're not statically caching the loaded links like we do in a lot of entity storages. Probably it's a case of "Don't add static caches until you have proof you need them" but I thought it was worth pointing out.

I think it would be okay to cache them statically.

(Okay for followup) Same question about whether we should use a more specific exception type.

At least in the menu tree storage it is NOT a plugin so I would just not catch other exceptions TBH.

What does "in active development" mean in this inline comment?

Given that the schema will not change for MenuTreeStorage this method could just hardcode all serialized fields. This method
was quite helpful during the development of all that code.

(No change needed) So all the overrides get stored in a single config file, correct? I think this makes sense but it also might be good to have @alexpott spot-check this.

Yes it does. Note: static links are mostly admin ones, so there aren't that many overrides needed. On top of that this makes it possible to use a config schema.

(No change needed) Interesting that the canonical link is the edit link. I guess it wouldn't really make sense to have a page to view a single menu link entity. ;)

At least for now, maybe someone comes up with a more clever idea. Rest would probably though like a route without an '/edit' suffix.

The inline comment confuses me. The two lines following seem to not agree with it? I mean, we're not "saving" anything here either way.

This is actually wrong. For translatable titles/descriptions we do use them. Removing the comment.

(Okay as followup) Why magic integers? :(

I do agree that we should: #2302641: Use constants instead of magic ints for expanded/hidden in MenuLinkContent

(Okay as followup) Why isn't this injected?

Just changed it. Not even worth to wait on all the HTTP requests of drupal.org

Sweet, @dawehner already took care of almost everything. Taking care of some of the last few points.

This comment will be replaced with pretty tables of responses when we are done.

No pretty tables needed, just so long as we document the things that aren't obvious here. :)

(Docs nitpick) "Boolean" should be capitalized (it's from a dude's name).

I bet this is not done anywhere else.

[mandelbrot:drupal | Mon 02:50:13] $ grep -r "Boolean" *  | wc -l
     697

:)

From @pwolanin re: #35.22:

if the route didn't have the needed parameters, you get a fatal in theUrlGenerator or AccessManager (which calls the UrlGenerator in checkNamedRoute()) and will throw a MissingMandatoryParametersException. So, nothing to do here it's handled at the time the parameters are needed.

I pushed several more cleanups. Only a few things left that I'm not sure how to address, so I made them bright yellow in the spreadsheet.

I think was removed in a cross post accidentally.

For #35 9.
#2302849: Add a code example to documentation to MenuLinkInterface::getFormClass()

This should have all the requested fixes and the increment since the last patch

trivial conflict in core.services.yml

Add the points so far have been addressed. the google spreadsheet was helpful.
I looked at the changes made by others overnight and they look great too.

typo

Alright, all 104 (!) of my points of feedback have been addressed, from architectural questions to unresolved @todos in the code, to docs gate issues down to tiny typos. 104 thank-yous especially to @pwolanin, @YesCT, @kgoel, and @dawehner for their patience and help getting this patch ready for commit.

I think the overall architecture makes a ton of sense; menu link plugins are an excellent solution given the varied implementations that exist in core, and they also offer a lot of opportunity for contrib to do interesting things. I also think the abstracted, decoupled, materialized menu tree storage is a great solution that allows materializing links from multiple sources together in a consistent, easily queried way that also will support other storage backends.

Be sure to see the suggested commit message; many people worked on this who didn't post patches here!

Issue #2301239 by pwolanin, dawehner, Wim Leers, effulgentsia, joelpittet, larowlan, xjm, YesCT, kgoel, victoru, berdir, likin, and plach: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage.

Uh. :)

This looks really good. Some tiny comments.

1. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,385 @@
   +    $fields['id'] = FieldDefinition::create('integer')
   +      ->setLabel(t('Content menu link ID'))
   +      ->setDescription(t('The menu link ID.'))
   ...
   +    $fields['parent'] = FieldDefinition::create('string')
   +      ->setLabel(t('Parent menu link ID'))
   +      ->setDescription(t('The parent menu link ID.'));
   

   This is confusing because parent refers to the parent plugin id.

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,385 @@
   +    $fields['url'] = FieldDefinition::create('string')
   +      ->setLabel(t('External link url'))
   +      ->setDescription(t('The url of the link, in case you have an external link.'));
   

   This should be FieldDefinition::create('uri') for semantic correctness.

3. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,385 @@
   +    $fields['external'] = FieldDefinition::create('boolean')
   ...
   +      ->setSetting('default_value', 0);
   ...
   +    $fields['expanded'] = FieldDefinition::create('boolean')
   ...
   +      ->setSetting('default_value', 0)
   ...
   +    $fields['hidden'] = FieldDefinition::create('boolean')
   ...
   +      ->setSetting('default_value', 0);
   

   Minor nit - these are booleans let's set their default values to booleans.

4. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
   @@ -0,0 +1,249 @@
   +        // Make sure the current ID is in the list, which may include multiple
   +        // IDs added earlier in each plugin's constructor.
   +        static::$entityIdsToLoad[$entity_id] = $entity_id;
   

   Why are we so cautious here - if it is not set something has gone wrong in the constructor no? Or is it possible that the entity id has changed?

5. +++ b/core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php
   @@ -0,0 +1,249 @@
   +        $entities = $storage->loadMultiple(array_values(static::$entityIdsToLoad));
   +        $entity = isset($entities[$entity_id]) ? $entities[$entity_id] : NULL;
   ...
   +        $links = $storage->loadByProperties(array('uuid' => $uuid));
   +        $entity = reset($links);
   

   Very minor nit. Are we loading links or entities? Seems inconsistent.

6. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,385 @@
   +    return $this->get('url')->value ?: NULL;
   

   This means we're expecting a mix of empty strings and nulls in the table. This is potentially confusing. In my mind the only values that should be in this column are valid urls. An empty string is not that.

+++ b/core/tests/Drupal/Tests/Core/Menu/StaticMenuLinkOverridesTest.php
@@ -0,0 +1,220 @@
+use Drupal\Core\Config\Config;

Very very minor - not used.

Followup: #2303113: Explicitly document all instances of calling t($variable)

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -0,0 +1,385 @@
+    $fields['external'] = FieldDefinition::create('boolean')
...
+      ->setSetting('default_value', 0);
...
+    $fields['expanded'] = FieldDefinition::create('boolean')
...
+      ->setSetting('default_value', 0)
...
+    $fields['hidden'] = FieldDefinition::create('boolean')
...
+      ->setSetting('default_value', 0);

Minor nit - these are booleans let's set their default values to booleans.

Also see in the plugin definition defaults -- they are being defaulted to ints there as well.

Here are some other thoughts that occurred whilst reading the patch - not actionable within the scope of this patch AFAICS.

1. +++ b/core/lib/Drupal/Core/Menu/StaticMenuLinkOverrides.php
   @@ -0,0 +1,163 @@
   +  protected $configName = 'menu_link.static.overrides';
   

   This is a new config file - I was told in IRC that the schema file is coming in a later patch.

2. +++ b/core/lib/Drupal/Core/Plugin/PluginManagerPass.php
   @@ -23,7 +23,9 @@ public function process(ContainerBuilder $container) {
        foreach ($container->getDefinitions() as $service_id => $definition) {
          if (strpos($service_id, 'plugin.manager.') === 0 || $definition->hasTag('plugin_manager_cache_clear')) {
   -        $cache_clearer_definition->addMethodCall('addCachedDiscovery', array(new Reference($service_id)));
   +        if (is_subclass_of($definition->getClass(), '\Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface')) {
   +          $cache_clearer_definition->addMethodCall('addCachedDiscovery', array(new Reference($service_id)));
   +        }
          }
        }
   

   We should file a follow up to just loop through all the services and check the interface and remove the tag - less to know.

3. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -0,0 +1,385 @@
   + *     "canonical" = "menu_link_content.link_edit",
   

   Is this really the canonical representation of a menu link entity?

4. The patch only adds test coverage of MenuTreeStorage and StaticMenuLinkOverrides we need to make sure test coverage is improved in later patches.

5. $this->t($plugin['definition'])
   

   We need to make sure #2303113: Explicitly document all instances of calling t($variable) covers this instance too

@alexpott re: #61.6

The 'uri' field is created with a NOT NULL schema definition, so we will have empty strings in some cases, so I think the code is correct as-is to give us consistent return values from the method.

`url` varchar(2048) NOT NULL COMMENT 'The url of the link, in case you have an external link.',

re: #65.3 - the Translate tab appears next to the "canonical" route page. I can't think of a useful canonical view of the data other than the edit form that would also appear with Edit and Translate tabs visible on it.

@xjm re: #64. No, I don't think that makes sense.

They are saved to the DB as ints and loaded that way from the storage. So, it doesn't make no sense to default the definition to booleans. The field definition is a "boolean" and we handle the conversion to int when building up the plugin definition from the entity field values.

Here's a patch incorporating the requested changes other than those addressed in #66, plus incremental diff and the full part5 patch with all the changes (to show it's all still working together).

So this is fixed now with the referenced commit?

AFAIK, yes. It's indeed in the 8.x branch.

Next step: #2301273: MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests :)

 _________________________________________
/ Committed <a                            \
| href="http://cgit.drupalcode.org/drupal |
| /commit/?id=8bb62da">8bb62da</a> and    |
\ pushed to 8.x. Thanks!                  /
 -----------------------------------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

:D

There should be a lama version of cowsay.

Yay, amazing. Closed down #1966398: [PP-1] Refactor menu link properties to multilingual as duplicate, since the new menu_link_content module is multilingual.

Here's a PDF version to memorialize the responses we made to each point from xjm

Tagging the child issues retroactively.