Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up from #2106709: Remove legacy router backward compatibility layer.
We removed the code that consults the {menu_router} table for page callbacks, but the table remains.
Its full removal might depend on finishing local tasks/actions, contextual links, and theme callbacks, but we might as well start.
Blockers
- #2084463: Convert contextual links to a plugin system similar to local tasks/actions
- #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system
- #2108349: Minimize the hook_menu() entries by remove references to 'page callback', file as well as 'access callback'
- #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
- #2177031: Remove menu_get_item() and every use of it.
- #2177041: Remove all implementations of hook_menu
- #2177045: Replace all implementations of hook_menu_alter with appropriate uses of the new routing system
- More to fill in....
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff.txt | 2.65 KB | dawehner |
#39 | menu_router-2107533-39.patch | 122.41 KB | dawehner |
#34 | menu_router-2107533-34.patch | 119.95 KB | pwolanin |
#34 | increment.txt | 5.31 KB | pwolanin |
#33 | menu_router-2107533-33.patch | 117.44 KB | tim.plunkett |
Comments
Comment #1
dawehnerLet's collection the blocking issues. It is kind of pointless to assign an issue to yourself if you can't work on it in like a month (common estimation from my experience).
Comment #2
tim.plunkettYeah the first two blockers are contextual and theme callback, as you added.
This also means we have to kill every usage of menu_get_item() :(
Comment #3
dawehnerLet's be honest ... we need to kill menu.inc first before we can get rid of
{menu_router}
.Some other potential blockers might be the book module.
Comment #4
xjmComment #4.0
xjm...
Comment #5
dawehnerLet's at least already try to reduce the menu_router table.
Comment #6
webchickIt may be a long shot, but it would be SO NICE to have this done by alpha4. (next Friday)
Comment #7
tim.plunkettThis is a meta, not a real actionable issue, and I don't think all of the sub issues in the issue summary will be done by then...
Comment #8
catchYes there's no chance of this being done by alpha 4. Removing tag and marking postponed so it's explicit.
Comment #8.0
catch.
Comment #9
xjmComment #10
tim.plunkettComment #11
tim.plunkettComment #12
tim.plunkettMoving this back to an actionable postponed issue, so we only have one meta.
Comment #13
xjmComment #14
dawehnerSimilar patch: #1316692: Convert hook_admin_paths() into declarative properties on routes
Comment #15
pwolanin CreditAttribution: pwolanin commentedComment #16
pwolanin CreditAttribution: pwolanin commentedComment #17
pwolanin CreditAttribution: pwolanin commentedComment #18
webchick#2177031: Remove menu_get_item() and every use of it. was just committed. I *believe* that now unpostpones this?
Comment #19
dawehner#2177041: Remove all implementations of hook_menu is certainly one issue we need and I doubt that we don't need many more.
Comment #20
xjm@dawehner, do you mean that there are missing beta blockers that still need to be filed? It would be helpful to get an outline if so in order to help find resources for them.
Comment #21
tim.plunkettHere's a patch that depends on the hook_menu removal. I've attached a combined one for now.
Comment #22
tim.plunkettLet's see how the combined one does.
EDIT: Stupid mistake. Moving this to a patch testing issue until hook_menu is in.
Comment #23
dawehnerYeah clearly we cannot just rip it out yet. One issue we also need is the book one, which decouples the menu_router and the book subsystem.
This is the reason why I just think that we cannot rip it out immediately and we need to break up the remaining parts into pieces.
Comment #24
tim.plunkettActually, book is just heavily tied to menu links, not the router.
I got this to pass, here's the full combined and split one again.
This patch's diffstat: 45 files changed, 104 insertions(+), 2107 deletions(-)
Comment #25
BerdirRe-uploading the do-not-test patch.
Comment #26
tim.plunkettPer discussion with @dawehner, I reinstated most of the doTestDescriptionMenuItems() test, and I finally removed the menu_item_route_access() helper function.
Also, since the callback and description parts of _menu_item_localize depended on the menu_router, I removed those.
We have the critical #2193777: Menu link translations need to work with content translation, shipped translations need to be compatible to follow-up with that, I'm just removing dead code.
I'm happy with this as is, but assigning myself for fixing any test failures, rerolls, or addressing reviews.
Comment #28
tim.plunkettHeh, #2044367: Replace calls to lock() with \Drupal::lock() changed code I was deleting.
Comment #29
BerdirBased on tests that I fixed in the hook_menu() removal issue, that dead code might not be as dead as you'd think. Let's see, there could be fails in the language ui negotiation something test, which indirectly verifies translatability of menu link descriptions...
Comment #30
tim.plunkettIt's pretty dead. Whether or not it was needed I cannot say, but it was not used. As evidenced by the passing patch.
Comment #31
dawehnerNice work!
It was not tested, it does not mean that we did not relied on it.
As a former translator I would expect the description to be translated on admin/index
I wonder whether we need to include the cache clearing into the lock statement?
Do we need this change?
We should open a follow up in case we want to keep this functionality.
This is an important piece of functionality. We want to ensure that the menu link_title has the proper language context. Note: This tests menu_links_defaults_rebuild() or how it is called.
Isn't that a totally valid test? We should maybe replace menu_get_router()
Actually we rebuild the router. ... kinda pointless comment to be honest.
Is there a reason why we did not needed the _menu_admin flag before?
This also feels a bit out of scope.
<3
Comment #32
tim.plunkett1) The description on /admin/index, or on any of the pages using SystemManager::getAdminBlock() like /admin, is not affected by _menu_item_localize().
It is pulled out of $link['localized_options']['attributes']['title'];, menu links don't have a 'description'.
2) This is just moved code, I'm not changing this around.
3) We need this change because I'm removing user_is_anonymous():
I'm removing that because I'm killing that global.
5) I can add back the test, but menu_router_rebuild() no longer exists...
6) I have no idea how to rewrite this test now that menu_get_router() is gone
7) Removed
8) This was a sloppy conversion by whoever swapped out user_is_anonymous(). I can add a test for this.
9) See 4.
Comment #33
tim.plunkettAddressing 5,6,7.
Comment #34
pwolanin CreditAttribution: pwolanin commentedCleanup of _menu_item_localize() to try to make it do something useful
Comment #35
pwolanin CreditAttribution: pwolanin commentedrelated: #2206235: Optimized access check for node menu links is broken.
Comment #36
tim.plunkettThat change looks good to me, +1
Comment #37
tim.plunkettThis is big enough to be a pain to reroll, but it also shouldn't stop too much from going in (hopefully no one else is changing menu_router code!).
Will retest next, but it'd be great to get another (or final?) review on this.
This could probably get a dedicated change notice for the few functions that are removed and as an overall announcement, but mostly we just need to update existing change records.
Comment #38
tim.plunkettNevermind, we can just add https://drupal.org/node/2203305 to the list of change records to update/expand.
Comment #39
dawehnerWe can remove a few more constants. I hope I don't kill kittens with doing that here.
I just don't get the avoid commit conflicts tag. All patches should be kinda treated equally.
Comment #40
dawehnerMissing interdiff.
Comment #41
webchickHELL YES!
Committed and pushed to 8.x. YAYYYYY. :) Great work, folks!!
Comment #42
ianthomas_ukWe need new or updated change records for at least some of the removed functions:
user_is_anonymous()
user_is_logged_in() (procedural replacement \Drupal::currentUser()->isAuthenticated())
user_register_access()
user_uid_only_optional_to_arg()
user_uid_optional_load()
user_uid_optional_to_arg()
menu_get_ancestors()
menu_unserialize()
menu_tail_to_arg()
menu_tail_load()
there are many more
If there is already a change record for these functions, then the function name should be added to it explictly so that it shows up in searches when people get fatal errors from old code.
https://drupal.org/node/2044515 needs updating (user_is_anonymous, user_register_access)
https://drupal.org/node/2031999 may be relevant
https://drupal.org/node/2203305 was mentioned in #38
Comment #43
Berdir_load() and _to_arg() are menu load callbacks, those aren't called directly.
The user_ functions were mostly used as access callbacks, but there's probably some code that was calling user_is_anonymous()/user_is_logged_in() directly, could be added to the AccountInterface change record?
menu_get_ancestors() was an internal helper function, if there really was code somewhere that relied on it, it likely needs to be completely rewritten for 8.x and you won't even get as far as calling this :) menu_unserialize() even more so.
https://drupal.org/node/2044515 only mentions those functions in the old code, that does not need to be updated :)
Comment #44
Crell CreditAttribution: Crell commentedWe talked about this in the WSCCI meeting today. We don't believe this warrants a separate change notice. The *entire* routing system is different. There's already a change notice that says "everything you knew about the old routing system is gone, here's how it works now, don't expect it to work." Documenting individual functions as missing now is a waste of time better spent on for-reals documentation.
Comment #45
dawehnerYeah in case we would add all this tiny change records here, the confusion would be worse than the potential benifits from it.
Comment #46
xjmComment #47
kfritscheI stumbled across a old change record - https://drupal.org/node/1561492 - which is now not valid anymore, as menu_router_rebuild() is removed by this issue.
What should happen with this change record then? unpublish? update? or just link to the big routing change record?
Comment #48
Crell CreditAttribution: Crell commentedIMO unpublish.
Comment #49
xjmDid both. :) However, there's no reference to menu_rebuild() on https://drupal.org/node/1800686. We probably need to add it to a list of functions removed there?
Comment #50
xjmSomehow the priority didn't get set back to critical. :)