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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
Issue tags: +sprint
plach’s picture

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).

plach’s picture

Issue tags: +sprint

crosspost

Crell’s picture

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.

tim.plunkett’s picture

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.

plach’s picture

Status: Active » Needs work
Issue tags: +sprint

crosspost party :)

plach’s picture

plach’s picture

Status: Needs work » Needs review

sigh

Anonymous’s picture

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

YesCT’s picture

@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.

YesCT’s picture

Status: Needs review » Needs work

wrong patch! hang on.

YesCT’s picture

should be the right files this time.

YesCT’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

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

tim.plunkett’s picture

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

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.

Anonymous’s picture

FileSize
2.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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.1 KB

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.

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.99 KB
494 bytes
Gábor Hojtsy’s picture

@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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
829 bytes
3.79 KB

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

Gábor Hojtsy’s picture

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

YesCT’s picture

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.

Anonymous’s picture

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?

Gábor Hojtsy’s picture

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

Kristen Pol’s picture

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.

dawehner’s picture

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.

Dries’s picture

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.

Gábor Hojtsy’s picture

Issue tags: -sprint

Remove from sprint.

Gábor Hojtsy’s picture

Remove sprint tag.