Update menu link access checks for new router

Bumping to major since this blocks the conversion of menu links to the new router. See #1848648: [meta] Convert menu system to new routing system for related issues.

This also depends on the #access key from menu_get_item(). That'll need to check both systems as well.

Ok, so here is a first patch that updates drupal_valid_path() to look at both routing systems. Added a helper function _drupal_valid_path_new_router() for that.

Depends on #1793520: Add access control mechanism for new router system.

I also included a special key for hook_menu() that we can use for conversion until menu links are sorted out.

  $items['user/register'] = array(
    'title' => 'Create new account',
    'type' => MENU_LOCAL_TASK,
    'router_converted' => TRUE,
  );

Please review the interdiff.

The current patch has potentially two performance issues, I'm assuming the first will be more severe than the second:

- every access check requires an individual database query to find the router item associated with the link, we've previously had that attached to menu links so it didn't require a separate lookup.

- every access check requires mocking the request object.

Since this patch will be the first opportunity to actually test this with the new router system, I'm adding the 'needs profiling' tag.

It'd be good to compare before/after with a large-ish menu tree (admin/config with a lot of modules enabled for example) - we might need to fake it with some dummy router items/links to have enough access checks to be comparable.

FYI devel_generate can generate menu links. Might need updating for D8.

This came up in irc and I remembered why I thought the issue title was strange.

menu_valid_item() is one place this gets checked, but for menu link access when they're rendered, it's

http://api.drupal.org/api/drupal/core%21includes%21menu.inc/function/_me...

and

http://api.drupal.org/api/drupal/core%21includes%21menu.inc/function/_me... (which loads menu objects ready for the access check).

Since this is going to be similar, I think we should do the two places at the same time, but could be persuaded otherwise if there's a really good reason to split.

If you need a different access check then your point of attack is the _menu_link_translate call in _menu_tree_check_access. You need to replace _menu_link_translate with something else. You will have a world of pain with links containing % signs. Otherwise, not so much. Obviously, the _menu_check_access call also needs to be replaced in this _menu_link_translate replacement .

I'm currently busy with getting rest.module into shape, so unfortunately I don't have time to continue with this patch. The posted patch is really far from ready, so setting to needs work.

I am not entirely sure if I understood what we need to do here. So for starters I tried to port the patch from #3 (or merely just the interdiff) to the new routing system.

To document what I think it should possibly do I added a Test for it. :)

Further advice is very welcome.

One of the user.module hunks was failing, so here's a free re-roll, courtesy of porting pants module to D8.

I'm still not sure how to make a menu item both have its route defined in a YML file, and also appear as a clickable link under admin/foo* though. This patch doesn't seem to help.

Hm. Nevermind, I was able to hack around it with the same hack user/register is currently doing, of defining its access callback twice: once in YAML and once in hook_menu(). I don't think we can ship this way though, so I wonder if this needs to be critical.

@webchick this issue is in the summary of #1848648: [meta] Convert menu system to new routing system which is marked as critical, although it's definitely critical in its own right as well.

@Crell that sounds like a good plan with the current menu link system, although I have no idea how it'll interact with menu links as entities.

@catch, the interaction with menu links as entities should be pretty minor, we can load all the needed routes (at once) and reconstruct the current structure in attachLoad().

This one will definitely screw the menu links as entities patch because there are a lot of places where we do db queries for getting menu links instead of using api functions (like menu_link_load())..

Even more, because our current access checks are closely tied to upcasting, I would say that this is also blocked by #1798214: Upcast request arguments/attributes to full objects and maybe #1906810: Require type hints for automatic entity upcasting too.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -324,6 +324,31 @@ public function loadModuleAdminTasks() {
   /**
+   * Implements \Drupal\Core\Entity\EntityStorageControllerInterface::load().
+   */
+  public function load(array $ids = NULL) {
+    $entities = parent::load($ids);
+
+    // For all links that have an associated route, load the route object now
+    // and save it on the object. That way we avoid a select N+1 problem later.
+    $routes = array();
+    foreach ($entities as $id => $entity) {
+      if ($entity->route) {
+        $routes[$id] = $entity->route;
+      }
+    }
+
+    if ($routes) {
+      $route_objects = drupal_container()->get('router.route_provider')->getRoutesByNames($routes);
+      foreach ($routes as $entity_id => $route) {
+        $entities[$entity_id]->setRouteObject($route_objects[$route]);
+      }
+    }
+
+    return $entities;

This should go into attachLoad().

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.php
@@ -232,6 +234,20 @@ class MenuLink extends Entity implements \ArrayAccess, ContentEntityInterface {
   /**
+   * The name of the route associated with this Link, if any.
+   *
+   * @var string
+   */
+  public $route;

No, no, no.. this is mixing the route property of a router item with a menu link property, and I thought the goal was to untangle them :/

Or.. did you actually want to add the 'route' column to the {menu_links} instead of {router}?

From testbot apache error log:

PHP Fatal error: Call to a member function setRouteObject() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php on line 84

#27: 1845402-menu-links-access.patch queued for re-testing.

From testbot checkout watchdog log:

Notice: Undefined variable: id in Drupal\menu_link\MenuLinkStorageController->attachLoad() (line 76 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php).

Notice: Undefined index: in Drupal\menu_link\MenuLinkStorageController->attachLoad() (line 84 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php).

Along with the PHP fatal listed above.

Just for completeness sake ... drupalLogin() is returning 3 passes, 3 fails. I'm also seeing warnings in the tesbot log itself (though those appear somewhat more randomly):

Warning: simplexml_import_dom(): Invalid Nodetype to import in DrupalWebTestCase->parse() (line 1594 of /var/lib/drupaltestbot/sites/all/modules/simpletest/drupal_web_test_case.php). x2

Let's see.

+++ b/core/modules/menu_link/menu_link.installundefined
@@ -199,6 +199,11 @@ function menu_link_schema() {
+      'route_name' => array(
+        'description' => 'The machine name of a defined Symfony Route this menu item represents.',
...
+        'length' => 255,

+++ b/core/modules/system/system.installundefined
@@ -2095,6 +2095,19 @@ function system_update_8048() {
+function system_update_8050() {
+  $spec = array(
...
+    'type' => 'varchar',
+    'length' => 255,

Why they are in different modules? Also not sure that 255 is enough for route as urls could be x4 longer

Possibly caused by #1239370: Module updates of new modules in core are not executed

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -117,7 +117,7 @@ function menu_link_delete_multiple(array $mlids, $force = FALSE, $prevent_repare
-  $menu_link->save();
+  return $menu_link->save();

Do not need to touch deprecated functions!

+++ b/core/modules/user/user.moduleundefined
@@ -879,22 +879,15 @@ function user_menu() {
   $items['user/autocomplete'] = array(
     'title' => 'User autocomplete',
     // _menu_router_build() denies access to paths without a page callback.
...
+    'route_name' => 'user_autocomplete',
...
   $items['user/autocomplete/anonymous'] = array(
     'title' => 'User autocomplete including anonymous',
     // _menu_router_build() denies access to paths without a page callback.
...
+    'route_name' => 'user_autocomplete_anonymous',

Maybe better to replace it with @todo and link?

This entries looks useless

+++ b/core/modules/user/user.moduleundefined
@@ -922,13 +915,7 @@ function user_menu() {
   $items['user/register'] = array(
     'title' => 'Create new account',
     'type' => MENU_LOCAL_TASK,
...
+    'route_name' => 'user_register',

Needs @todo for #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)

Why they are in different modules?

Yeah you are probably right on that point.

Also not sure that 255 is enough for route as urls could be x4 longer

That's the router name, not the path, so for example "edit_metadata", see edit.routing.yml

Maybe better to replace it with @todo and link?

This entries looks useless

There is already a @todo in the beginning of user_menu().

Needs @todo for #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)

Not sure whether it really makes sense to add the todo, because, you would have to add one to all menu items already.

Fixed some other pieces.

I'm wondering whether we want to have explicit tests for this kind of functionality?

This has been a bad comment from mine.

Create an explicit test for a hook_menu entry with a connected route, that has an access check and test, for example, that _menu_translate() returns what's expected.

I'll take a stab at that test tomorrow.

Discussed this with @dawehner and we agreed that the expected functionality is already tested by the 'user/register' route (which has to stay in hook_menu() because it's a MENU_LOCAL_TASK) and there's really no point in testing _menu_link_translate() since it will be going away soon-ish anyway.

I found a couple of other doxygen issues and also that we don't need hook_menu() entries anymore for user autocomplete routes, since this patch fixes the @todo that was keeping them in place!

Therefore, I think this is good enough for now and allows the conversion of MENU_CALLBACK router items to be crowdsourced this weekend :)

Merged HEAD.

This is what happens when we work with and without branches :( We were missing the fix from #38.

Finally!

It's not clear whether catch's performance concern in #5 was addressed?

Tossing it at him. :)

msonnabaum said he'd be profiling this today. The query-per-route was my main concern and that's fixed, I think smaller regressions we can make up if we do #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees (since current menu link caching in 7/8 is not that efficient).

Assuming profiling comes back OK I'm happy for someone else to commit this (and I know people are keen to have it in tomorrow for conversions - I won't be around for most of tomorrow so don't want to hold it up).

#51: drupal-1845402-51.patch queued for re-testing.

I profiled this patch on new d8 install, hitting user/register 30 times each. That seemed to be a decent place to trigger the new code path, but please correct me if I'm wrong here.

I'm actually showing a slight improvement in wall time with the patch. I dug into it a bit and it looks like there's some savings from not calling things like _menu_check_access, which in turn makes a bunch of config calls, etc.

I could dig further, but it doesn't seem worth it. This patch feels very low risk performance-wise to me.

Ok, looks like the poor patch survived both testbot and msonnabaum :D so going to go ahead and get this in to help people attending the Drupal Global Sprint Weekend crank on conversion patches! :)

I love love love that this patch gets rid of totally awkward stuff like the NOT_USED value. It's a little unfortunate you need to manually specify the route name for visual links, but I can't think of another way now that path is no longer unique between routes.

Committed and pushed to 8.x. Thanks!

This'll need an update to the routing system change notice. We might be getting close to be able to stop updating that now. :P