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.
Comment | File | Size | Author |
---|---|---|---|
#26 | access-check-2004780-26.patch | 3.79 KB | tim.plunkett |
#26 | interdiff.txt | 829 bytes | tim.plunkett |
#23 | interdiff.txt | 494 bytes | tim.plunkett |
#23 | access-check-2004780-23.patch | 2.99 KB | tim.plunkett |
#20 | unify-mocked-menu-item-route-access-checking-2004780-20.patch | 3.1 KB | Anonymous (not verified) |
Comments
Comment #1
Gábor HojtsyComment #2
plachCode looks good to me, I was able to spot only a couple of code style issues.
Wrong indentation.
Typo (was already there).
Comment #3
plachcrosspost
Comment #4
Crell CreditAttribution: Crell commentedThis seems straightforward enough to me, but I'll let Tim actually RTBC it since he wrote the original code being moved around.
Comment #5
tim.plunkett+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.
Comment #6
plachcrosspost party :)
Comment #7
plachNew patch fixing #2.
Comment #8
plachsigh
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch solves the problems identified in #1998600: Block config translation fails.
Comment #10
YesCT CreditAttribution: YesCT commented@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.
Comment #11
YesCT CreditAttribution: YesCT commentedwrong patch! hang on.
Comment #12
YesCT CreditAttribution: YesCT commentedshould be the right files this time.
Comment #13
YesCT CreditAttribution: YesCT commentedComment #14
tim.plunkettI would prefer not taking item by reference and returning a Boolean, and doing the assignment in the calling code.
Comment #15
tim.plunkettActually, then it could just take a route and an href, no need for the whole router item.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedMade 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.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedInterdiff at the request of @YesCT!
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedLet's reference the right Symfony component!
Comment #22
Gábor HojtsyThe code still attempts to catch NotFoundHttpException although that is not anymore use-d.Bad.
Comment #23
tim.plunkettComment #24
Gábor Hojtsy@tim.plunkett: that seems to be the same as #17 now?
Comment #26
tim.plunkettI found the issue, our MockRouteProvider was not returning any routes.
Comment #27
Gábor HojtsyWow, looks good to me. Should be RTBC if passes AFAIS.
Comment #30
YesCT CreditAttribution: YesCT commentedI 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.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedTested #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?
Comment #32
Gábor HojtsyYeah the block bug may be the same as the filter bug with the config data. Not related to this one.
Comment #33
Kristen PolI 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.
Comment #34
dawehnerThe 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.
Comment #35
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #37
Gábor HojtsyRemove from sprint.
Comment #38
Gábor HojtsyRemove sprint tag.