Validate default menu link definitions

It would be kind of better to add this exception somewhere around

      else {
        if (empty($link['route_name']) && empty($link['link_path'])) {
          watchdog('error', 'Menu_link %machine_name does neither provide a route_name nor a link_path, so it got skipped.', array('%machine_name' => $machine_name));
          continue;
        }
        $menu_link = $menu_link_storage->createFromDefaultLink($link);
      }
      if (!empty($link['parent'])) {
        $children[$link['parent']][$machine_name] = $machine_name;
        $menu_link->parent = $link['parent'];
        if (empty($link['menu_name'])) {
          // Unset the default menu name so it's populated from the parent.
          unset($menu_link->menu_name);
        }
      }
      else {
        // A top level link - we need them to root our tree.
        $top_links[$machine_name] = $machine_name;
        $menu_link->plid = 0;
      }

in
menu_link_rebuild_defaults()

What about providing an actual named exception for non-existing parent and catch that one different, so we at least show some message?

Too tired... updated the issue title and summary to clarify that the current behavior results in weird and invalid menu links being saved and notices being thrown (in HEAD) and changing to a bug report. With the validation/exception throwing as the suggested solution for

Result is the same but clarifies that this fixes an actual problem and isn't just a nice to have improvement :)

The link title is a similar case, you can right now save a menu link without a link title (for example, because you accidentally used 'title' when converting from hook_menu()), and it will happily save a menu link with a link title. I would assume that we can make the title required now, as we don't have callbacks and stuff like that anymore, so we should possibly validate and throw an exception for that too?

Adding some tags

Previously menu_router_rebuild() have been critical, as you could not rebuilt anything manually via the UI. This is not longer the case given that the menu_router is not the router anymore.

+++ b/core/includes/menu.inc
@@ -2553,6 +2553,16 @@ function menu_link_get_defaults() {
+
+    // Check for required array keys.
+  if (!isset($link['link_title'])) {
+    throw new \Exception(sprintf('Menu link %s does not have a "link_title" key in its definition.', $machine_name));
+  }
+

We could also check for an accident use of 'title' instead of 'link_title'. have you also thought about checking that the route name is valid?

+++ b/core/includes/menu.inc
@@ -2554,11 +2555,44 @@ function _menu_link_save_recursive($controller, $machine_name, &$children, &$lin
+      throw new \Exception(sprintf('Menu link %s does not have a "link_title" key in its definition.', $machine_name));
...
+      throw new \Exception(sprintf('Menu link %s must provide either a "route_name" or a "link_path" key in its definition.', $machine_name));
...
+        throw new \Exception(sprintf('Menu link %s specifies route name %s, but no route with that machine name exists.', $machine_name, $link['route_name']), 0, $e);

No typed exception for any of these? :(

Also shouldn't we be able to test this now?

They also don't appear in the watchdog?

Raising to major, for the reason #2198571: menu.inc must not unset MenuLink::menu_name was re-opened. Apparently providing a wrong parent now leads to a PDO exception and this is happening a lot now because we renamed the menu links so all contrib modules that are placing admin links are now broken.

Found the problem, the two foreach's with by reference were messing with the links.

20: drupal_2198619_20.patch queued for re-testing.

why are we using sprintf?

+      throw new \Exception(sprintf('link_path %s for menu link %s must be absolute.', $link['link_path'], $machine_name));

Also, there is code now that's incorrect that unsets menu name in certain cases during the save. I don't know that this patch makes sense in general.

I don't think validating the actual route is worth the code in term of time and memory.

We should certainly validate the menu links in case we are in a development enviroment, much like we should do route definition validation in case we have that mode.

should possibly postpone on this? #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module

Lets postpone this issue until: #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module

Unblocked I think.

Maybe this helps to trigger some initiative in xano :P

Plain reroll

Sorry, but that's a completely incorrect re-roll In #34 - this patch needs to be rewritten since the menu link save code is no longer in menu.inc.

In fact, I think the basic problem is fixed in MenuTreStorage::findParent()

It validates the parent there - if it doesn't exist, the link goes to the top level with an empty parent.

I'm pretty sure we handle invalid route name at render time.