Updated: Comment #0

Problem/Motivation

When converting a MENU_CALLBACK to new routing system, if the hook_menu entry is completely removed, all menus are removed from the page.

Steps to reproduce:

  1. Clone drupal 8 and checkout commit: a8d4542933c61484d1719394ae9843f25f325cde
  2. curl https://drupal.org/files/drupal8.test_page_test.1987868-19.patch | git apply -v --index
  3. Install Drupal 8
  4. drush en test_page_test -y
  5. Go to /test-page

Result: page has no home menu link at top
screenshot of bug
Expected result: page has home menu link
screenshot of desired behavior

Proposed resolution

menu_get_tree should not rely on menu_get_item() but primarily take the route object on the current request into account.
On the longrun we should remove the menu_get_item() call.

Remaining tasks

Review, RTBC, commit

#1987868: Convert test_page_test_page() to a new style controller
The issue was discovered in the above issue. The unique thing about tests with the test_page_test_page callback, is most of the menu_test module entries use the test page as their callback, and because they test the menu system, some of the tests go directly to the /test-page and look for menus on that page. That's most likely why this wasn't discovered earlier.

Files: 
CommentFileSizeAuthor
#18 menu_tree-2078583-18.patch1.5 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,527 pass(es).
[ View ]
#12 menu_tree-routing-2078583-12.patch4.84 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,778 pass(es).
[ View ]
#12 2078583-6-12.increment.txt2.85 KBpwolanin
#6 menu_tree-routing-2078583-6.patch3.24 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]
#6 2078583-3-6.increment.txt1.95 KBpwolanin
#3 menu_tree-routing-2078583-FAIL.patch993 bytesdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,379 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#3 menu_tree-routing-2078583-PASS.patch2.34 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,574 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#2 menu_tree-routing-2078583-2.patch1.37 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,598 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 shortcut_0.png178.67 KBdawehner
screenshot of desired behavior21.78 KBdisasm
screenshot of bug22.8 KBdisasm

Comments

dawehner’s picture

StatusFileSize
new178.67 KB

Let's post the old backtrace.

dawehner’s picture

Status:Active» Needs review
Issue tags:+WSCCI
StatusFileSize
new1.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,598 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Here is a fix for it, working on proper tests now.

dawehner’s picture

StatusFileSize
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 58,574 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new993 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,379 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Here is also a test.

dawehner’s picture

Issue summary:View changes

Adding work-around

pwolanin’s picture

Are you sure that this is not set on a legacy page?

$request->attributes->has(RouteObjectInterface::ROUTE_OBJECT)

Might be better to check for '_legacy'?

Status:Needs review» Needs work

The last submitted patch, menu_tree-routing-2078583-PASS.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.95 KB
new3.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]

Switches the check and fixes the Views test

dawehner’s picture

+++ b/core/includes/menu.inc
@@ -1274,8 +1275,19 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
+  if ($request->attributes->has('_legacy')) {
+    // @todo Remove once the old router system got removed.
+    $item = menu_get_item($active_path);
+  }

Good idea!

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

That is ready to fly, we have test coverage and a proper fix.

Linking an similar issue: #2081963: Shortcut add/remove link on titles don't work on route only pages

disasm’s picture

Perfect solution! Thanks @dawehner and @pwolanim for getting this done so quickly. RTBC++.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/includes/menu.inc
@@ -1274,8 +1275,19 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
+  elseif ($route = $request->attributes->get(RouteObjectInterface::ROUTE_NAME)) {
+    $item['href'] = $request->attributes->get('_system_path');
+    // @todo What should be done on a 404/403 page?
+    $item['access'] = TRUE;
+  }

Uh, woah there now. We can't switch one critical bug for another critical security bug. We need that resolved before commit.

pwolanin’s picture

This isn't a security bug - it's just faking the access check and allowing the menu to render. Crell was in favor of removing this check anyhow - it's debatable as to whether it's useful.

Currently there is a regression of sorts in core that we cannot tell if the current page is a 403 or 404. That's being worked on in #2068471: Normalize Controller/View-listener behavior with a Page object so We should probably mention that issue in the todo - basically this is a fix to get things mostly right.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.85 KB
new4.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,778 pass(es).
[ View ]

Improved the variable names and code comments to make it more clear what's happening.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

It is a bit confusing that we don't cast just when we need it but I won't fight this.

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI

The last submitted patch, menu_tree-routing-2078583-12.patch, failed testing.

pwolanin’s picture

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

#12: menu_tree-routing-2078583-12.patch queued for re-testing.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

ok, the earlier test fail seems to have been something sporadic, since it was unrelated to the patch.

catch’s picture

Status:Reviewed & tested by the community» Needs review

This looks fine, but could we reverse the check so the legacy is the fallback? Will make profiling more accurate the more routes are converted.

dawehner’s picture

StatusFileSize
new1.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,527 pass(es).
[ View ]

Yeah this was the route in the earlier patch as well. Additional this is easier to read.

You seem to have this already committed?

catch’s picture

Status:Needs review» Fixed

Yes I must have committed before I spotted the order. Committed/pushed the follow-up as well.

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

Anonymous’s picture

Issue summary:View changes

update