Problem

Breadcrumbs check access to breadcrumb items, but they never run the param upcasting on the items. This makes most of the patch at #1952394: Add configuration translation user interface module in core broken for most routes, where older menu items are involved. Major because it makes pages whitescreen that you add below menu items that are otherwise in the old menu system.

Proposal

The solution is just to use the mock menu item access checking the same way as it was used elsewhere in the menu.inc code already. So to avoid copy-paste, I'm just introducing a simple helper function for that. No code changes in the first hunk and using the same access check code in the other hunk to ensure new route based paths on top of old route based paths work. This was suggested and reviewed live by @timplunkett and @Crell.

Testing

The patch at #1952394: Add configuration translation user interface module in core is just about to include a test for entity pages where it fails due to this bug, so we can cross-check with that. I'm not sure it makes sense to add a test to this one because the other patch will demonstrate the fail, and making a test for this would include old menu items with new routes in a test module and we are very actively working to remove the coexistence of them.

Files: 
CommentFileSizeAuthor
#26 access-check-2004780-26.patch3.79 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,032 pass(es).
[ View ]
#26 interdiff.txt829 bytestim.plunkett
#23 interdiff.txt494 bytestim.plunkett
#23 access-check-2004780-23.patch2.99 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,040 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#20 unify-mocked-menu-item-route-access-checking-2004780-20.patch3.1 KBRyan Weal
FAILED: [[SimpleTest]]: [MySQL] 57,257 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#18 interdiff.txt2.47 KBRyan Weal
#17 unify-mocked-menu-item-route-access-checking-2004780-17.patch2.99 KBRyan Weal
FAILED: [[SimpleTest]]: [MySQL] 55,810 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#12 unify-mocked-menu-item-route-access-checking-2004780-9.patch2.98 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 54,750 pass(es), 1,722 fail(s), and 22,903 exception(s).
[ View ]
#12 interdiff-7-9.txt1.29 KBYesCT
#10 test_all_lists-2004710-7.patch3.45 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#10 interdiff-5-7.txt2 KBYesCT
#7 unify-mocked-menu-item-route-access-checking-2004780-7.patch2.72 KBplach
FAILED: [[SimpleTest]]: [MySQL] 55,625 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
unify-mocked-menu-item-route-access-checking.patch2.72 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 55,773 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+sprint

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

Code looks good to me, I was able to spot only a couple of code style issues.

+++ b/core/includes/menu.inc
@@ -949,6 +938,30 @@ function _menu_link_translate(&$item, $translate = FALSE) {
+ *  Router for the given menu item.

Wrong indentation.

+++ b/core/includes/menu.inc
@@ -949,6 +938,30 @@ function _menu_link_translate(&$item, $translate = FALSE) {
+  // acccess checker.

Typo (was already there).

Issue tags:+sprint

crosspost

Status:Needs work» Needs review

This seems straightforward enough to me, but I'll let Tim actually RTBC it since he wrote the original code being moved around.

Status:Needs review» Active
Issue tags:-sprint

+1 for no explicit tests. The code being moved into menu_item_route_access() already is covered in several different places, and the code path to trigger this bug will ideally go away after all conversions are complete.

Status:Active» Needs work
Issue tags:+sprint

crosspost party :)

StatusFileSize
new2.72 KB
FAILED: [[SimpleTest]]: [MySQL] 55,625 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

New patch fixing #2.

Status:Needs work» Needs review

sigh

This patch solves the problems identified in #1998600: Block config translation fails.

StatusFileSize
new2 KB
new3.45 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

@param and type casting.. which made it make sense to have the thing in the use.
Also made some comment a sentence.

The moving around of the code makes sense to me.
If these changes look ok to others, the docs core gate part is RTBC from me.

Status:Needs review» Needs work

wrong patch! hang on.

StatusFileSize
new1.29 KB
new2.98 KB
FAILED: [[SimpleTest]]: [MySQL] 54,750 pass(es), 1,722 fail(s), and 22,903 exception(s).
[ View ]

should be the right files this time.

Status:Needs work» Needs review

I would prefer not taking item by reference and returning a Boolean, and doing the assignment in the calling code.

Actually, then it could just take a route and an href, no need for the whole router item.

Status:Needs review» Needs work

The last submitted patch, unify-mocked-menu-item-route-access-checking-2004780-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,810 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Made some revisions based on feedback from @tim.plunkett in #14 and #15.

I see that previous tests failed but the testbot page will not load the details so I'm not sure what tests failed.

StatusFileSize
new2.47 KB

Interdiff at the request of @YesCT!

Status:Needs review» Needs work

The last submitted patch, unify-mocked-menu-item-route-access-checking-2004780-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.1 KB
FAILED: [[SimpleTest]]: [MySQL] 57,257 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Let's reference the right Symfony component!

Status:Needs review» Needs work

The last submitted patch, unify-mocked-menu-item-route-access-checking-2004780-20.patch, failed testing.

The code still attempts to catch NotFoundHttpException although that is not anymore use-d.Bad.

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] 56,040 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new494 bytes

@tim.plunkett: that seems to be the same as #17 now?

Status:Needs review» Needs work

The last submitted patch, access-check-2004780-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new829 bytes
new3.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,032 pass(es).
[ View ]

I found the issue, our MockRouteProvider was not returning any routes.

Wow, looks good to me. Should be RTBC if passes AFAIS.

I looked at the code in #26 and it looks great standards wise.
I also tried it manually, with the config translation module.
without the patch admin/structure/contact/manage/feedback/translate white screens
with the patch it shows the translation table correctly

without the patch admin/config/content/formats/basic_html/translate white screens
with the patch it shows the translation table correctly

.. so this is looking good. But there is a problem with the bots. We'll check back later.

Tested #26 and it seems OK. We still get an "access denied" message on the block config page but it no longer does WSOD there, so I'm assuming that bug to be in the config_translation module. The other entity-based translatable elements seem to work as expected.

Still waiting for testbot. Maybe someone can ping a bot manager?

Yeah the block bug may be the same as the filter bug with the config data. Not related to this one.

I tested #26 for the taxonomy vocabulary translate page and it works great! :) Thanks @tim.plunkett :)

The code looks clean to me... but I don't know anything about Symfony ;) RTBC in my book but I won't change the status.

Status:Needs review» Reviewed & tested by the community

The code looks great for me. On the longrun, when we have a proper strategy for menu links in general we might need some helper class
and based on that proper unit testing but this seems to be work for the future.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

Status:Fixed» Closed (fixed)

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

Issue tags:-sprint

Remove from sprint.

Remove sprint tag.