This is partially a Views issue as well since it also improperly prepares its routes when default tasks are involved.

There is specific code in menu_router_build() to process menu items referencing routes, but it is run before the drupal_alter() call.

CommentFileSizeAuthor
#14 vdc-2011006-14-FAIL.patch3.53 KBtim.plunkett
#14 vdc-2011006-14-FAIL-without-menuinc.patch8.24 KBtim.plunkett
#14 vdc-2011006-14-PASS.patch10.94 KBtim.plunkett
#12 route-vdc-2011006-12.patch7.4 KBtim.plunkett
#12 interdiff.txt2.29 KBtim.plunkett
#8 menu-items-2011006-8.patch4.57 KBtim.plunkett
#8 interdiff.txt985 bytestim.plunkett
#1 menu-routes-2011006-1.patch4.56 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
menu-routes-2011006-1.patch4.56 KB

This blocks #1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations and #1851086: Replace admin/people with a View

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.phpundefined
@@ -84,6 +84,16 @@ public function collectRoutes(RouteCollection $collection) {
+      if ($bit == '%views_arg' || empty($bits)) {
+        $bits[] = $bit;
+      }

Does this also cover the case that views_arg is skipped to make it as performant as possible?

+++ b/core/includes/menu.incundefined
@@ -2716,25 +2716,6 @@ function menu_router_build($save = FALSE) {
-          $new_path = _menu_router_translate_route($router_item['route_name']);
...
-          $router_items[$new_path] = $router_item;
-          $path = $new_path;

What happens with this bits?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Not sure what you mean about views_args. That's lifted from executeHookMenu().

+++ b/core/includes/menu.incundefined
@@ -2742,6 +2723,26 @@ function menu_router_build($save = FALSE) {
+  foreach ($callbacks as $path => &$router_item) {
...
+      $new_path = _menu_router_translate_route($router_item['route_name']);
+
+      unset($callbacks[$path]);
+      $callbacks[$new_path] = $router_item;

This covers that now by using &$router_item by reference.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, menu-routes-2011006-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review

#1: menu-routes-2011006-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, menu-routes-2011006-1.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Hmm, at the moment this completely kills my installation. Apply this patch and try to install drupal...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt985 bytes
menu-items-2011006-8.patch4.57 KB

My silly reference was causing an infinite loop.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Don't we need tests for that as well?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: +Needs tests

Yes. Some module using hook_menu_alter().

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Seems like views could do with a test too?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt2.29 KB
route-vdc-2011006-12.patch7.4 KB

Views forms are also broken, for the same reason.
This moves the default tab check into a helper method.

The interdiff ignores whitespace.

Still needs tests.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Menu items for routes provided in hook_menu_alter() are not processed » Default local tasks provided by Views are broken
Component: routing system » views.module

I can no longer reproduce whatever made me change menu.inc.
I'm going to refocus on writing a test for the views portion.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: -Needs tests
FileSize
vdc-2011006-14-PASS.patch10.94 KB
vdc-2011006-14-FAIL-without-menuinc.patch8.24 KB
vdc-2011006-14-FAIL.patch3.53 KB

Actually, the menu.inc change is still needed.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Related: #2012502: Tab options form cannot be submitted

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Nice!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Major » Critical

@tim.plunkett clarified that this is our actual blocker for overriding routes (I couldn't find either of them).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed edb0095 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.