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:
- Clone drupal 8 and checkout commit: a8d4542933c61484d1719394ae9843f25f325cde
- curl https://drupal.org/files/drupal8.test_page_test.1987868-19.patch | git apply -v --index
- Install Drupal 8
- drush en test_page_test -y
- Go to /test-page
Result: page has no home menu link at top
Expected result: page has home menu link
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
Related Issues
#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.
Comment | File | Size | Author |
---|---|---|---|
#18 | menu_tree-2078583-18.patch | 1.5 KB | dawehner |
#12 | menu_tree-routing-2078583-12.patch | 4.84 KB | pwolanin |
#12 | 2078583-6-12.increment.txt | 2.85 KB | pwolanin |
#6 | menu_tree-routing-2078583-6.patch | 3.24 KB | pwolanin |
#6 | 2078583-3-6.increment.txt | 1.95 KB | pwolanin |
Comments
Comment #1
dawehnerLet's post the old backtrace.
Comment #2
dawehnerHere is a fix for it, working on proper tests now.
Comment #3
dawehnerHere is also a test.
Comment #3.0
dawehnerAdding work-around
Comment #4
pwolanin CreditAttribution: pwolanin commentedAre you sure that this is not set on a legacy page?
Might be better to check for '_legacy'?
Comment #6
pwolanin CreditAttribution: pwolanin commentedSwitches the check and fixes the Views test
Comment #7
dawehnerGood idea!
Comment #8
dawehnerThat 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
Comment #9
disasm CreditAttribution: disasm commentedPerfect solution! Thanks @dawehner and @pwolanim for getting this done so quickly. RTBC++.
Comment #10
webchickUh, woah there now. We can't switch one critical bug for another critical security bug. We need that resolved before commit.
Comment #11
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #12
pwolanin CreditAttribution: pwolanin commentedImproved the variable names and code comments to make it more clear what's happening.
Comment #13
dawehnerIt is a bit confusing that we don't cast just when we need it but I won't fight this.
Comment #15
pwolanin CreditAttribution: pwolanin commented#12: menu_tree-routing-2078583-12.patch queued for re-testing.
Comment #16
pwolanin CreditAttribution: pwolanin commentedok, the earlier test fail seems to have been something sporadic, since it was unrelated to the patch.
Comment #17
catchThis looks fine, but could we reverse the check so the legacy is the fallback? Will make profiling more accurate the more routes are converted.
Comment #18
dawehnerYeah this was the route in the earlier patch as well. Additional this is easier to read.
You seem to have this already committed?
Comment #19
catchYes I must have committed before I spotted the order. Committed/pushed the follow-up as well.
Comment #20.0
(not verified) CreditAttribution: commentedupdate