Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit

I love this idea. It's the only sane way to finally decouple menu links from old router items.

We should also try to invoke this hook from the new menu_link module, and move _menu_navigation_links_rebuild() and friends to the same module while we're at it.

I am wondering whether we should replace a hook_menu_link() (however we call it) with a .menu_link.yml file and build something similar as routes, but this is just an idea.

The advantage is that people have a more common way to build things related with routing/menu (which people can't distinct that easy as we think). If you need non-static menu links, you just go with a event subscriber and act on certain events.

At least we should put the hook_menu_link into a separate include file, so it is not loaded on every request. This is one of these decisions which are hard to make, especially because every solution as advantages/disadvantages.

We're eliminating manual include files everywhere else. Let's not re-introduce it here.

I'm flexible on moving the remnants of hook_menu() to a new hook, but that's an API change so unless it fulfills https://drupal.org/node/2045345 I doubt that we could justify it.

Let's see. (I agree.)

We're eliminating manual include files everywhere else. Let's not re-introduce it here.

I dont' talk about manual include files but rather about a hook_hook_info entry.

I'm finding it hard to have an opinion on this one.

Is the main reason to rename the hook to break it for un-ported modules? Or to make it clearer that it's only defining menu links?

There's no 'exported' menu links format - they're content entities rather than config entities, and that format would need to reflect relationships between them etc. which aren't in hook_menu() atm but get added via processing or the UI, so that's not really an option I don't think - for this we should just keep more or less the same thing as we have now and revisit again for D9.

I'm leaning towards a won't fix on this (at least as far as renaming hooks goes) unless it's really necessary for #2044969: Break the old router for contrib, but I don't think so.

The rest (deprecating the APIs for contrib) seems like it's handled in that issue, so we should probably centralize discussion over there, and close this as a duplicate.

I'm very much in agreement with @pwolanin here, changing the hook name would make it very visible when porting a module to D8 that you need to check out the new routing system *now*, not in a few months when we'll hopefully be able to remove the old one and your code will break.

It also makes it consistent with the new module and entity type that we have in D8, which is 'menu_link', not 'menu' (that's a different entity type).

I guess the answer to @catch in #12 is: pretty much both :) Also, Peter has a plan to make the parent relationship explicit in hook_menu_link() definitions, which I also think is a nice goal. Of course, that relationship will not be taken into consideration on menu rebuilds if the menu link was customized in the UI.

To sum up, I think this is a very good change to make and we should do it as soon as possible.

There are other ways to accomplish that that don't invalidate documentation and break APIs post-API freeze, though. For example:

if (!in_array($module, $core_modules)) {
  if (in_array($key, $list_of_deprecated_properties)) {
    throw new Exception("Bad.");
  }
}

What exactly is 'that'? :)

+1
Totally agree with amateescu and pwolanin

Adding #2051889: Add route parameters property to menu links as a dependency to get something going. The link generator issue is also quite important: #2047619: Add a link generator service for route-based links

I like hook_default_menu_links(), since that seems to map to the end result better. I do remember, vividly, the confusion I had around all the various magic that hook_menu() contained when I started writing modules for Drupal. It took time to realize I was dealing with both the definition of menu items, menu routes, and menus themselves, in a way.

Renaming the function would make it much clearer as to what the hook is actually doing, IMO.

updated resultion

For reference, this will help also with completing #1810350-50: Remove menu_* entity keys and replace them with entity links.

+++ b/core/modules/system/system.module
@@ -955,6 +957,204 @@ function system_menu() {
+  $items['admin/config/regional'] = array(
+    'link_path' => 'admin/config/regional',
+    'link_title' => 'Regional and language',
+    'description' => 'Regional settings, localization and translation.',
+    'weight' => -5,
+  );
...
+++ b/core/modules/system/system.module
@@ -955,6 +957,204 @@ function system_menu() {
+  $items['admin/config/search'] = array(
+    'link_title' => 'Search and metadata',
+    'link_path' => 'admin/config/search',
+    'parent' => 'admin/config',
+    'description' => 'Local site search, metadata and SEO.',
+    'weight' => -10,
+  );

1. Why does the first not have a 'parent', but the second does?
2. Since the keys no longer have any direct relation to URLs, should we change them to use some other delimiter than "/" (the issue summary currently has "_")?
3. Should we default 'parent' to the key name up to the last instance of the chosen delimiter? If not, should we then make our key names even shorter, like 'system.search' instead of 'admin_config_search'?

Oh, and adding the "API change" tag to this issue. This still needs approval from a maintainer. #25 does not look to me like actual approval. FWIW, I agree with the change, because it looks like we need to still make API changes to hook_menu() anyway, such as changing the keys (link machine names) from paths to something else, and adding an explicit 'parent' key for each link. Since these are new changes that haven't already been communicated (as opposed to the ones that have, such as removing 'page callback'), then it's a post-API-freeze BC break either way, so we might as well pick a hook name we like, and I think hook_default_menu_links() has some DX/clarity advantages over hook_menu(), per #23.

Also, FWIW, I like #4's suggestion of moving this into a non-CMI YAML file, as we have for routing, instead of a hook, but #12 rejected that.

One thing that we may want to think about is that we want to get rid of hooks in Drupal 9. Converting this to a different hook in D8 and then converting it to another format in D9 while keeping the actual functionality the same will hurt BC. Instead, converting it to a format that will likely stay in D9 (yml, plugins?) now will less likely break BC then. Let's not do two 'lesser' API changes in two Drupal versions instead of a big one in one Drupal version.

+1 to converting to an yaml file. This would probably also need an Event ala RouteSubscriber for dynamic menus, but not sure on that.

If that's too big of an api change, can we at least add it to hook_hook_info()? hook_menu() is often one of the largest hooks and 95% of the time it's just clutter.

Let's see how much this fixes.

Note: this code is just involved in {menu_router}, so running this code does not really matter as the menu_name should not be used in the {menu_router} table.

Rerolled.

Just some work everywhere.

Most or at least many of the issues are caused by menu_link_get_preferred() working just on menu links but not on things like local tasks anymore.

Well, we should have the foundations for it, but maybe it would be kind of hard to share the information between the title bit and the breadcrumb bit, but yeah the breadcrumb patch is nearly ready anyway.

Yeah the inheriting of options in the menu tree is just wrong. Add the title/title callback every would do it.

So we basically need on issue which converts all titles.

As discussed with @pwolanin at the extended sprints in Prague, I opened #2094481: Support default content entities to try and solve the definition and import of default content entities on a more generic level.

I am wondering where this issue got up to... I'd dearly like to see this get into D8.

Specifically, as a module developer I fully support:

* breaking hook_menu so people are forced to get up to speed on the D8 separation between menu items and routes... a distinction that makes a lot of sense. There are already good blog posts on this topic so D7 developers should be able to pick this up easily.

* the 'parent' key makes so much sense — an implicit structure is inherent in menu items and this is easy to comprehend.

* as it is currently, the D8 hook_menu items are keyed by the route's URL path, which I think is confusing since the items directly reference the route anyway.

Does it have approval from a core maintainer as yet?

Let's first ensure that the installation works again.

This is a new version, which fixes a couple of the failures.

One thing we might should do in the meantime is to get titles on all the routes and not rely on hook_menu() anymore.

This is currently blocked by #2143933: Add _title and _title_callback to all routes.

This now converts all the entries which just uses hook_menu() and not hook_menu_alter() ... let's first see the test failures.

Drupal installs fine, so we don't get a massive amount, especially now that the title issue is solved, yeah!

Introduced an alter hook and converted the views implementation to it.

Just out of curiosity, why a hook? This seems very similar to hook_image_default_styles in D7, which we converted to config entities in D8.

Hm. I guess cos they're content entities, not config entities. Still, this same question (or slight variations of) have been asked multiple times in this thread, and it'd probably be good for the issue summary to offer more of an explanation why this is seen as the best route forward. I do think it's a bit of a DX fail to define one half of the menu link in YAML and the other half in PHP, but I guess this is what catch asked for in #12?

Just out of curiosity, why a hook? This seems very similar to hook_image_default_styles in D7, which we converted to config entities in D8.

I think we should switch to a really similar model like routing, so have static yml files but also allows dynamic/altered items.
The main reason why this patch haven't made that is because the hard part of the patch is getting it green and using hooks certainly requires the least amount of changes. I would suggest to get it green and then switch to a better format and discuss that in the meantime?

The failure was that you should not register menu links for non-static routes (routes with a required parameter), as you cannot generate a URL for it.

Yeah, I guess my main issue is trying not to introduce more API changes at this point in the cycle, so trying to get the format right the first time. It sounds like though like we might just have to live with shipping with this as a hook in D8 and revisit other options in D9.

One note on the patch itself:

+++ b/core/modules/views_ui/views_ui.module
@@ -36,6 +36,31 @@ function views_ui_menu() {
+  // Top-level Views module pages (not tied to a particular View).
+  $links['admin.structure.views'] = array(
+    'link_title' => 'Views',
+    'parent' => 'admin.structure',
+    'description' => 'Manage customized lists of content.',
+    'route_name' => 'views_ui.list',
+  );
+
+  // A page in the Reports section to show usage of plugins in all views.
+  $links['admin.reports.views-plugins'] = array(
+    'link_title' => 'Views plugins',
+    'parent' => 'admin.reports',
+    'description' => 'Overview of plugins used in all views.',
+    'route_name' => 'views_ui.reports_plugins',
+  );

*Having* to specify the parent is a bit unfortunate. :\ It'd be nice if it could do something automagical based on the dot notation (e.g. "admin.structure.XXX" gets its parent set behind the scenes to "admin.structure"), but could manually override it in case a route name didn't want to conform for some reason.

@Pwolanin, did you see http://previousnext.com.au/blog/potential-default-content-solution-drupa..., happy to discuss

I doubt that we can require people to have hal, serialize and rest enabled just to use menu links for real.

#876580: drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus) kind of contains the same fixes as needed here ...
This also fixes all kind of issues all over the place.

Should we continue to support the "reset" feature? I'm tempted to rip it out and see if anyone misses it.

As long we provide all current "default links", contrib could provide that UI for us.

Should we try to delete links if the route they point to goes away? If we are treating these are pure content, maybe it would be better not to mess with "content"?

I doubt that we can require people to have hal, serialize and rest enabled just to use menu links for real.

agreed

Really nice cleanups here :)

1. +++ b/core/includes/menu.inc
   @@ -1133,21 +1078,12 @@ function menu_tree_output($tree) {
   +    $element['link'] = $data['link']->build();
   
   +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
   @@ -638,26 +614,29 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
   +  /**
   +   * Builds and returns the renderable array for this menu link.
   +   *
   +   * @return array
   +   *   A renderable array representing the content of the link.
   +   */
   +  public function build() {
   

   This is the job of a entity ViewBuilder, and we have an issue to implement one for menu links: #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now

2. +++ b/core/includes/menu.inc
   @@ -2350,10 +2286,10 @@ function menu_set_active_trail($new_trail = NULL) {
   +       _menu_link_translate($preferred_link);
   
   @@ -2380,7 +2315,7 @@ function menu_set_active_trail($new_trail = NULL) {
   +            _menu_link_translate($link);
   

   Isn't this _menu_link_prepare() now?

3. +++ b/core/includes/menu.inc
   @@ -2689,116 +2603,125 @@ function menu_get_router() {
   +  // @todo - remove links based on their machine names when module are
   +  // uninstalled, or mark them disabled if the route names are missing?
   

   I'd vote for disabled.

Where does _menu_link_prepare() exist?

In the patch.

In 2 places 'prepared' is used instead of 'prepare'. Shouldn't the names match the function name?

   drupal_alter('prepared_menu_link', $item);
   function hook_prepared_menu_link_alter(\Drupal\menu_link\Entity\MenuLink &$menu_link, $map) {

This needs updating in user.module.

 function user_translated_menu_link_alter(MenuLink &$menu_link) {

And shouldn't \Drupal::moduleHandler()->alter('prepare_menu_link', $item) be used instead of drupal_alter since the latter is deprecated?

No offense, but I have the feeling that these kind of unrelated changes will SO much not help to get this patch green on the longrun.

I'm with @dawehner, there is a ton of stuff here that might be a good change, but seems out of scope. This patch is big enough already, can we remove some of this?
Especially the #below/below part, no idea what's happening there.

This is a straight reroll.

Just posting some work, which removes some of the changes to move the main point of the patch forward.

This fixes a couple of the failures.

If it blocks the beta, it by definition blocks the release.

I agree with webchick that the hook isn't ideal, but I also agree that making it anything else we'd want a 'default content entities' API which is out of scope at the moment.

However, potentially we could provide a 'default entities API' in a future 8.x release, use this to create default menu links, and have the hook left in as a bc layer.

@catch: given #98 above saying Removes all localizing for now since they needs to come back later as proper translatable fields., that means we should consider #1842858: [PP-1] Convert menu links to the new Entity Field API (already critical) and #1966398: [PP-1] Refactor menu link properties to multilingual (only major yet) as a critical as well. Which may make #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions a critical given the supposed performance problems of duplicating tables for multilingual. Would be good to roll this all the way to the issues required as consequences of decisions made here.

I'd be OK with those three being critical.

FWIW, I fully agree with #116. And there's some great cleanup here that will help #1842858: [PP-1] Convert menu links to the new Entity Field API a lot!

More fixes.

I took a look at the documentation in this patch. Found a couple of minor things:

a)

+/**
+ * Builds menu links for the items returned from hook_default_meanu_links().
+ */
+function menu_default_links_rebuild() {

hook has typo in name in doc block "meanu_links" instead of "menu_links".

b) Within function drupal_valid_path():

+  /** @var $route_provider \Drupal\Core\Routing\RouteProviderInterface */
+  $route_provider = \Drupal::service('router.route_provider');

We don't actually want /** @tag */ comments within function bodies, at all.

c) Shouldn't hook_menu() be going away in this patch, and all the existing hook_menu() implementations be removed? Otherwise, I wouldn't be sure the tests are valid... Oh, I see, there is a two-phase approach. Maybe a patch that also removes hook_menu() would at least be useful for testing purposes? But... the hook_menu() docs page on api.drupal.org says that menu_router_build() invokes it, and also system_get_module_admin_tasks()... I don't think that first invocation has been removed from the patch?

d) In MenuLink.php:

+    // @FIXME Decide whether we want to keep the reset functionality.

Please use @todo, which is more standard in the Drupal project.

e) Is there documentation of the new hook somewhere? I'm not seeing it in the patch.

I didn't set this to "needs work" in case that would be annoying if this is really just an "in-progress" patch looking for feedback, but I think these issues need to be addressed... and I didn't read through the 128 comments here to see what's going on, just the summary. So sorry if I am duplicating other comments. Update the summary. :)

There are more than a dozen usages of this, it is included in the proposed PSR-5, and someone should open an issue about adding it to our doc standard. But not worth discussing further in this issue.

Adding Performance tag as this gets rid of a very slow code path. See #2150833: Performance regression in menu access checks. Lets try to clear the blocker #2100577: Phase 1 - Decouple book module from menu inc and proceed to a speedy commit.

Yes please open a new issue if you want to update the docs standards; until then, it's not compliant. Use tag "coding-standards" and component "documentation" and start title with [policy]. And then go read #1617948: [policy for now] New standard for documenting form/render elements and properties where people complained about adding /** */ comments within function bodies, and thereby blocked otherwise quite reasonable proposals.

Opened the policy issue: #2152037: [policy] Use inline @var documentation for more specific class documentation

Fixed the following failures:

• Drupal\book\Tests\BookTest
• Drupal\language\Tests\LanguageUILanguageNegotiationTest
• Drupal\system\Tests\System\FrontPageTest (should be already fixed)

.

There are still no docs for this hook, which makes the patch really hard to evaluate. Writing hook docs is a great exercise, as it makes you think about whether the hook makes any sense. :)

This fixes the toolbar tests as well as the frontpage tests but and fixes the exception in the MenuRouterTest which causes 5 new failure in the actual test.

Aggregator categories have recently been removed from core, so that sounds like a legitimate fail :)

Awesome to see this green!

1. +++ b/core/includes/menu.inc
   @@ -759,7 +759,9 @@ function _menu_translate(&$router_item, $map, $to_arg = FALSE) {
   +   // Avoid notices until we remove this code.
   

   Should we add a link to an issue here? (and the indentation is off a bit)

2. +++ b/core/includes/menu.inc
   @@ -1133,19 +1085,14 @@ function menu_tree_output($tree) {
   +    // @todo Use route name and parameters to generate the link path, unless
   +    //    it is external.
   +    $element['#href'] = $data['link']['link_path'];
   

   Here or followup?

3. +++ b/core/includes/menu.inc
   @@ -2487,6 +2423,8 @@ function menu_link_get_preferred($path = NULL, $selected_menu = NULL) {
   +    // @todo How do we fetch the title of tabs and other non menu links.
   

   Still relevant?

4. +++ b/core/includes/menu.inc
   @@ -2689,105 +2614,141 @@ function menu_get_router() {
   +  // Remove process links names so we can find stragglers.
   

   Remove processed links ...

5. +++ b/core/includes/menu.inc
   @@ -2689,105 +2614,141 @@ function menu_get_router() {
   +      // Note, we set this as 'system', so that we can be sure to distinguish all
   

   "we set the 'module' field as 'system'"

6. +++ b/core/includes/menu.inc
   @@ -2689,105 +2614,141 @@ function menu_get_router() {
   +          $link['link_title'] = 'THIS IS A BUG';
   

   lol :) Throwing an exception is to invasive here? Maybe it deserves at least a watchdog entry.

7. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Plugin/Derivative/ConfigTranslationContextualLinks.php
   @@ -58,7 +58,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
   -      /** @var \Drupal\config_translation\ConfigMapperInterface $mapper */
   +      /** @type \Drupal\config_translation\ConfigMapperInterface $mapper */
   

   Unrelated change.

8. +++ b/core/modules/contact/contact.module
   @@ -85,6 +85,26 @@ function contact_menu() {
   +    'type' => MENU_SUGGESTED_ITEM,
   

   Since we're moving/updating all menu link definitions, how about doing away with MENU_SUGGESTED_ITEM and just use 'hidden' straight up?

9. +++ b/core/modules/entity/entity.module
   @@ -96,6 +96,36 @@ function entity_menu() {
   +  $links['admin.structure.display-modes'] = array(
   

   Any reason for using hyphens instead of underscores?

10. +++ b/core/modules/menu/lib/Drupal/menu/Form/MenuDeleteForm.php
    @@ -106,7 +106,7 @@ public function submit(array $form, array &$form_state) {
         // @todo Convert this to an EFQ once we figure out 'ORDER BY m.number_parts'.
    -    $result = $this->connection->query("SELECT mlid FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.router_path = m.path WHERE ml.menu_name = :menu AND ml.module = 'system' ORDER BY m.number_parts ASC", array(':menu' => $this->entity->id()), array('fetch' => \PDO::FETCH_ASSOC))->fetchCol();
    +    $result = $this->connection->query("SELECT mlid FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.link_path = m.path WHERE ml.menu_name = :menu AND ml.module = 'system' ORDER BY m.number_parts ASC", array(':menu' => $this->entity->id()), array('fetch' => \PDO::FETCH_ASSOC))->fetchCol();
    

    Since the purpose of this patch is completely decouple {menu_links} from {menu_router}, can we remove this JOIN?

11. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -368,13 +375,15 @@ public function setRouteObject(Route $route) {
    +    // @FIXME Decide whether we want to keep the reset functionality.
    

    Do we have a decision yet? :)

12. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -384,27 +393,23 @@ public function reset() {
    +  protected function buildFromOriginalItem(array $item) {
    

    How about buildFromArray()? Or we can just move this code to a EntityInterface::preCreate() implementation.

13. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -384,27 +393,23 @@ public function reset() {
    -      ->getStorageController('menu_link')->create($item);
    +       ->getStorageController('menu_link')->create($item);
    

    Unneeded change.

14. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -597,7 +594,7 @@ public static function findRouteNameParameters($link_path) {
    +  protected function setParents(MenuLink $parent) {
    

    We type hint with interfaces, not classes.

15. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -638,26 +635,29 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
    +  /**
    +   * Builds and returns the renderable array for this menu link.
    +   *
    +   * @return array
    +   *   A renderable array representing the content of the link.
    +   */
    +  public function build() {
    

    This should say that it's temporary and it's going to be moved to a proper view builder in #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now.

16. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -100,7 +100,7 @@ public static function createInstance(ContainerInterface $container, $entity_typ
    +    $query->leftJoin('menu_router', 'm', 'base.link_path = m.path');
    

    How come this is still necessary?

17. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LinksTest.php
    @@ -183,7 +183,7 @@ function testMenuLinkReparenting($module = 'menu_test') {
    +   * Test automatic reparenting of menu links derived from hook_default_men_links.
    

    Typo and exceeds 80 chars.

18. +++ b/core/modules/system/system.admin.inc
    @@ -9,6 +9,7 @@
    +use Symfony\Cmf\Component\Routing\RouteObjectInterface;
    

    Doesn't seem to be used.

19. +++ b/core/modules/system/system.install
    @@ -2293,6 +2293,23 @@ function system_update_8060() {
    + * Add machine_name column to the menu_links table.
    

    .. and remove the 'router_path' field.

20. +++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Controller/MenuTestController.php
    @@ -19,6 +19,11 @@ public function menuTestCallback() {
    +  public function titleCallback(array $_title_arguments = array(), $_title = '') {
    

    Missing doxygen.

21. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TaxonomyController.php
    @@ -57,6 +57,19 @@ public function termTitle(TermInterface $taxonomy_term) {
    +   *   The term label.
    

    The vocabulary label.

Also it looks like the @return for menu_link_translate() got removed in the patch, inadvertently.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
@@ -380,10 +380,12 @@ public function reset() {
+    static::preCreate($storage_controller, $original);

preCreate() is already invoked by the storage controller :)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageControllerInterface.php
@@ -34,20 +34,6 @@ public function setPreventReparenting($value = FALSE);
-  public function loadUpdatedCustomized(array $router_paths);

Nice!

#2095283: Remove non-storage logic from the storage controllers has been committed, which conflicts a bit with the storage controller changes here.

Just a simple rerole.

How come this is still necessary?

Other queries like loadModuleAdminTasks need the join to be there, so let's try to keep the patch size as small as possible and improve later.

Now that we know who exactly still needs that join, I'm perfectly fine with keeping it in this patch :)

Just a rerole to see the test failures.

Let's try to revert some specific changes to get it back to green again.

Let's try to revert some specific changes to get it back to green again.

That is it.

With all of the patches flying on this issue, and comments saying "reverting to make tests pass" etc., it is hard to know whether the patches that are posted are really meant to be reviewed as potential candidates for committing. I'd like to review the API docs in the patch, for instance, but I don't want to waste time looking at them if the patch is still being worked on.

So when you think you have a real viable patch, can you post a comment that lets potential reviewers you think it's viable? Until then I'm assuming you're still working on it and using patch posting as a collaboration tool or a tool to see what still needs to be done (which is fine, but for those not working actively on the patch it is kind of pointless to look at each patch).

Thanks!

You know, peter from time to time just goes to far with his patches, so this patch lets the patch stay in scope.

So this patch is ready to reviewed.

OK, thanks! I took a look at the patch (primarily at the API docs, as you might expect). I found a few things to fix:

a)

 * @return
  *   Returns the map of path arguments with objects loaded as defined in the
  *   $item['load_functions'].
  *   $item['access'] becomes TRUE if the item is accessible, FALSE otherwise.
- *   $item['href'] is generated from link_path, possibly by to_arg functions.
- *   $item['title'] is generated from link_title, and may be localized.
- *   $item['options'] is unserialized; it is also changed within the call here
- *   to $item['localized_options'] by _menu_item_localize().
+ *   $item['href'] is generated from link_path
+ *   $item['access'] becomes TRUE if the item is accessible, FALSE otherwise.
+ *   $item['title'] is generated from link_title.
+ *   $item['route_parameters'] is unserialized if needed.
+ *   $item['options'] is unserialized and copied to $item['localized_options'].
  */
-function _menu_link_translate(&$item, $translate = FALSE) {
+function _menu_link_translate(&$item) {

This ends up with two $item['access'] and the $item['href'] should end in . -- actually this should be formatted as a list anyway, shouldn't it?

And actually, the return statement has been removed from the end of this function body. So the @return needs to be taken out entirely. It seems like the description of what happens to $item is useful, but there is not a return value for this function.

b)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -25,18 +25,17 @@
 class MenuLinkStorageController extends DatabaseStorageController implements MenuLinkStorageControllerInterface {
 
   /**
-   * Indicates whether the delete operation should re-parent children items.
-   *
-   * @var bool
+   * Contains all {menu_router} fields without weight.
+   * @var array
    */
-  protected $preventReparenting = FALSE;
+  protected static $routerItemFields;

Should be a blank line between description and @var. Also if this is an array of some specific type of object, you can do @var \Name\Of\The\Class[]

c) There are still some @todo items in the code, like MENU_SUGGESTED_ITEM. Do they need to be resolved now?

d)

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
...
+   * Test automatic reparenting of menu links derived from hook_default_men_links.
    */
   function testMenuLinkRouterReparenting() {

Test -> Tests and the hook name has a typo, should be _menu_ not _men_.

e) hook_default_menu_links() and the _alter() hook both need sample function bodies. Also the base hook and alter hook should have @see references to each other.

f) In hook_default_menu_links:

+ *   An array of default menu links. Each link has a key corresponding to a
+ *   unique machine name. The corresponding array value is an
+ *   associative array that may contain the following key-value pairs:
+ *   - "link_title": Required. The untranslated title of the menu item.
+ *     with path component substitution as described above.
...

- Second sentence should say "Each link has a key that is the machine name, which must be unique."
- Do not put "" or '' or anything else around the key names in the list. And "Required" should be (required). See https://drupal.org/node/1354#lists
- I think the second line of "link_title" should go away? There is nothing about path component substitution "above"?
- "route_name" says it is an untranslated title, but I think it's the machine name of the route really?
- "route_parameters" says the same thing, and it definitely isn't?
- "link_path" doesn't really say what it is.
OK I stopped looking. This doc section is not OK. Please fix. :)

-

Should be a blank line between description and @var. Also if this is an array of some specific type of object, you can do @var \Name\Of\The\Class[]

Fixed. I don't think it is really helpful for scalar datatypes like string[] or int[] or even mixed[] ?

There are still some @todo items in the code, like MENU_SUGGESTED_ITEM. Do they need to be resolved now?

Yes, they need to be resolved at some point, though I really think we should take the iterative strategy and fix small in scope changes, given that adding 50kb to a patch defers the result potentially for at least 2 weeks.

e) hook_default_menu_links() and the _alter() hook both need sample function bodies. Also the base hook and alter hook should have @see references to each other.

Added one example.

OK I stopped looking. This doc section is not OK. Please fix. :)

I hope you will like this more.

This was just stupid, ... $item might be an object.

Just two simple things.

This patch is looking better since the last time I looked at it!

A few notes:

a)

+ * @param array $item
+ *   The passed in and changed menu item with the following elements:
+ *   - href: The actual path to the link, is generated from link_path
+ *   - access: Becomes TRUE if the item is accessible, FALSE otherwise.
+ *   - title: The title of the link, is generated from link_title.
+ *   - route_name: The route name of the menu link.
+ *   - route_parameters: The unserialized route parameters.
+ *   - options: Is unserialized and copied to $item['localized_options'].
  */
-function _menu_link_translate(&$item, $translate = FALSE) {
+function _menu_link_translate(&$item) {

In the href item, what is "link_path"? I don't see it in the list. Maybe this should say: "The path to the link, which comes from the defined route." -- or ... not sure what it means. In any case it should end in ".". Same applies to the title item here. ... This is all a bit confusing to me. Some of the items in the list are inputs, I think, and some are added to the array. It seems like this should be made clearer. Maybe it should be two sections -- something like:
The following elements need to be present in $item:
(list, and indicate here if they are also updated in the function)
The following elements are added to or overridden in $item:
(list)

b)

-function menu_item_route_access(Route $route, $href, &$map) {
-  $request = RequestHelper::duplicate(\Drupal::request(), '/' . $href);
-  $request->attributes->set('_system_path', $href);
+function menu_item_route_access(Route $route, $href, &$map, Request $request = NULL) {

This function has a new argument $request but I don't see a new @param added to the documentation above it.

c)

+ * Recursive helper to save menu links.
  */
+function _menu_link_save_recursive($controller, $machine_name, &$children, &$links) {

Should probably say something like "Saves menu links recursively for whatever_uses_it()." to conform to our docs standards of starting with a verb and referring to the user of the function.

d)

+/**
+ * Find all links returned from hook_default_menu_links().
+ */
+function menu_get_default_links() {

Find -> Finds

e)

+  /**
+   * {@inheritdoc}
+   *
+   * This function should only be called for link data from
+   * hook_default_menu_links().
+   */
+  public function createFromDefaultLink(array $item) {

@inheritdoc does not work in the API module if you put other stuff in the documentation. Our docs standard currently is that you either inherit the doc or you don't. You can't both inherit it and add stuff to it.

f) In the docs for hook_default_menu_links():

+ *   - link_title: (required) The untranslated title of the menu item.
+ *     with path component substitution as described above.

This is not described above. There is nothing above this.

g) Same docs block:

+ *   - route_parameters: (required) The route parameters to build the path.

I do not think this is required. Most of the implementations and the sample function body do not include it.

h) same block:

+ *   - link_path: (optional) If you have an external link use link_path instead
+ *     of providing a route_name.

Well above it says route_name is required. Here it says it isn't.

i) same block:

 *   - parent": (optional) The machine name of a link to define hierarchy.

Stray " -- also this isn't clear. What hierarchy is defined? How about just saying "The machine name of the link that is this link's menu parent" or something?

j) same block:

+ *   - weight: An integer that determines the relative position of items in

Should probably say (optional)

k) same block:

+ *   - type: (optional) A bitmask of flags describing properties of the menu item.
+ *     Many shortcut bitmasks are provided as constants in menu.inc:

It seems like only the two flags of normal/suggested apply actually now?

l) same block:

+ *   - options: An array of options to be passed to l() when generating a link
+ *     from this menu item.

This should say (optional)

m)

+ *   - menu_name: (optional) Set this to a custom menu if you don't want your
+ *     item to be placed in the default Tools menu.

How about saying "The machine name of a menu to put the link in, if not the default Tools menu."

n) How come the menu links table schema is in menu_link.install and the update function for it is in system.install? That seems wrong. If menu_link is not enabled, won't that system.install update function fail?

This is not described above. There is nothing above this.

We indeed don't do that fancyness anymore.

Well above it says route_name is required. Here it says it isn't.

You are absolute right!

Stray " -- also this isn't clear. What hierarchy is defined? How about just saying "The machine name of the link that is this link's menu parent" or something?

This sounds nice!

It seems like only the two flags of normal/suggested apply actually now?

You are right, I changed the text a bit.

n) How come the menu links table schema is in menu_link.install and the update function for it is in system.install? That seems wrong. If menu_link is not enabled, won't that system.install update function fail?

Well, menu links are part of system module in d7, so if you have system in d7 you need to update the table of menu links. menu links in d7
are bound to the system schema, so that is kind of the place to update it. Additional the other update functions for menu links happen on there.

interdiff?

Here is one.

Thanks for the interdiff, although it wasn't complete? Anyway, a few comments:

a)

+ *   - route_parameters: The unserialized route parameters of the men link.

typo: men -> menu

b) I think my comment in #196-a still stands, in regards to the menu_link_translate() param docs:

This is all a bit confusing to me. Some of the items in the list are inputs, I think, and some are added to the array. It seems like this should be made clearer.

c) #196 (e) still applies.

d) In the hook docs:

+ *   - route_name: (required) The route name to be used to build the path.
+ *   - route_parameters: (optional) The route parameters to build the path.
+ *   - link_path: (optional) If you have an external link use link_path instead
+ *     of providing a route_name.

The route_name (required) and docs for link_path still contradict each other. Either route_name or link_path is required, but saying route_name is required is misleading.

e) in hook docs:

+ *   - parent": (optional) The machine name of the link that is this link's menu

Remove the "

f) #196 (l) still applies.

g) I still do not understand the justification for having the update function and the schema in different modules. If the Menu Links module is not installed, its schema/database tables will not exist, and the update function will not work. Don't they need to be together? And if someone updates from 7 to 8 and does not have the menu links module enabled at the time... what happens then if they install the module and it tries to create the tables and they already exist? The whole thing I think needs a bit more thought.

a)

Fixed

b) I think my comment in #196-a still stands, in regards to the menu_link_translate() param docs:

This is all a bit confusing to me. Some of the items in the list are inputs, I think, and some are added to the array. It seems like this should be made clearer.

Splitted up the input and changed elements.

c)

Fixed

The route_name (required) and docs for link_path still contradict each other. Either route_name or link_path is required, but saying route_name is required is misleading.

Right.

Remove the "

Good catch.

This should say (optional)

Fixed.

g) I still do not understand the justification for having the update function and the schema in different modules. If the Menu Links module is not installed, its schema/database tables will not exist, and the update function will not work. Don't they need to be together? And if someone updates from 7 to 8 and does not have the menu links module enabled at the time... what happens then if they install the module and it tries to create the tables and they already exist? The whole thing I think needs a bit more thought.

If you update from d7 to d8 you don't have the menu module installed, just the system module, which provides these tables back in d7. Afaik these updates will be migrations on the longrun.

I am sorry but I failed again to provide a proper interdiff.

Reviewed this patch again and I understand that some parts of the review from #159 need to be handled in followups in order to keep the patch size manageable. Good thing we already have followups for most of them.

I hope @jhodgdon is happy with the latest changes and I only found a few small nitpicks, so here's a small update and a much deserved RTBC :)