Updated: Comment #0
Problem/Motivation
We have several legacy services in place to support using hook_menu() for routing. We're just about to not have any more need for that, so let's start removing it.
Proposed resolution
Remove legacy_router
Rename router.dynamic to router, remove the existing ChainRouter
Attempt to remove legacy_access_subscriber, legacy_url_matcher, legacy_generator
Ensure _drupal_menu_item and USES_ROUTE no longer appear
Remaining tasks
Wait for the last 2 page callbacks to die.
User interface changes
N/A
API changes
Using hook_menu() for routing will no longer work.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#14 | remove-legacy-router-2106709-14.patch | 26.07 KB | tim.plunkett |
#14 | interdiff.txt | 8.45 KB | tim.plunkett |
#8 | remove-legacy-router-2106709-8.patch | 17.61 KB | tim.plunkett |
#7 | remove-chain-router.patch | 18.18 KB | tim.plunkett |
#5 | KILL_LEGACY_ROUTER-2106709-5.patch | 18.05 KB | tim.plunkett |
Comments
Comment #1
Crell CreditAttribution: Crell commentedI'm inclined to leave the ChainRouter layer in place for now. It's overhead for only one router is minimal, and we've discussed using it for an optimized router for common routes (which its, actually, its primary use case).
Otherwise, KILL!!!
Comment #2
tim.plunkettLet er rip
Comment #4
cosmicdreams CreditAttribution: cosmicdreams commentedMissing some bits in common.inc. Search the docblocks as well.
Comment #5
tim.plunkettI don't care about removing the string "page callback" from everywhere yet. This is a functional issue.
I added back too many a hunk when I was restoring ChainRouter.
Comment #7
tim.plunkettI'm going to debug this locally, but in the meantime here is the patch I originally wrote, before reading #1.
Comment #8
tim.plunkettUploading this one as well, let both finish please.
Both should pass, but we can decide which to use.
Removing the actual concept of a 'page callback' and all of its code is for another issue, since we still have legacy local tasks, actions, and contextual links to finish.
Comment #9
webchickThis is a patch I'd love to get in before alpha4.
Comment #10
Crell CreditAttribution: Crell commentedPer comment #1, I favor patch #8. That is, keep ChainRouter in place for easier flexibility and extensibility. Even if we find later that it has an unwanted performance overhead (which I highly doubt), it could be taken out of the container configuration trivially with no API changes.
Thus, I am RTBCing #8.
TIM, YOU'RE AWESOME!
Comment #11
Crell CreditAttribution: Crell commentedI DID change the status, I swear I did...
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedis getting the docblocks right a gate? Because this patch removes the old stuff, but I'm not sure if all the docblocks have been updated for this change.
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedis getting the docblocks right a gate? Because this patch removes the old stuff, but I'm not sure if all the docblocks have been updated for this change.
Comment #14
tim.plunkettComment #15
mpark CreditAttribution: mpark commentedSorry about OTC, but where could I dowload 8.x-alpha4?
Comment #16
webchickYou can't yet. :) It comes out end of next week, hopefully.
Comment #17
mpark CreditAttribution: mpark commentedThanks, I hope also! :-)
Comment #18
BerdirIf we do want to commit this like this, then we should at least have a follow-up issue to clean up the documentation, there are still 59 (not counting the one in CHANGELOG.txt) mentiones of "page callback" left. Lot's of those are pages.inc @file docblocks, but still.
Also, this still seems to exist:
But that constant shouldn't exist anymore according to the issue summary?
Comment #19
tim.plunkettI'm working through the menu_router_build() stuff in #2107039: Make remaining hook_menu declarations match route paths.
There are
isset($item['page callback'])
checks still needed for local tasks, local actions, contextual links, and theme callback. Yet the value is no longer used.Comment #20
catchSo as I understand it, we have both docs references to page callback, and things like theme callback that rely on it being set, but not actually the value.
In that case I'm OK committing this as is to remove it, and another, critical follow-up to update docs once the code references are gone. Does that sound OK?
While this means the old router won't be consulted, we still have a tonne of code writing to it etc. don't we? i.e. I don't see anything removing the table from schema here.
Comment #21
tim.plunkettOpened #2107533: Remove {menu_router} to address #20, I think #2046367: Document the internals of the router covers the rest.
Comment #22
catchCommitted/pushed to 8.x, thanks!
This could probably use a g.d.o/core announcement since it's finally going to break hook_menu() for routes.
Comment #23
xjmWe probably need to do a few things change notice-wise here:
Check to see if the main routing change notice or any others need an update.Update https://drupal.org/node/1800686 if needed.Comment #24
xjmOh, and I said this in IRC, but I'll make sure this goes in tomorrow's "This week in D8". Yay! :)
Comment #25
xjm@catch retitled https://drupal.org/node/1800686 sensibly so I think that covers #1 for now. Can someone check for #2 and then we can close this? Edit: This at least is a lie, so please read closely:
Also, the
hook_menu()
API documentation is egregiously misleading. While I look forward to the fairies and ponies from #2046367: Document the internals of the router and #2107533: Remove {menu_router}, can we please file a small docs patch that adds a short paragraph indicating that much of the hook is deprecated and use the new routing system? It doesn't need to be perfect; it just needs to point out that most of the wall of text following it is a lie.Comment #26
hass CreditAttribution: hass commentedI had a very bad DX last week when the settings menu item has disappeared in my module and than I found later that I was out of sync with hook_menu. Once I got them in sync with the route.yml the menu item re-appeared. https://drupal.org/node/1800686 does not list the cases for 'description' and the other open @todo's.
Can someone fill this article with the missing issue links, please? I'm fairly lost in finding them myself.
Comment #27
jhodgdonDid the patch for this issue not update the hook_menu docs? Wouldn't that be part of the docs gate for a patch that totally changes what hook_menu() is for, or better yet several iterations ago when you had to start using routing.yml files in the first place? This practice of ripping things in/out of core without updating the docs is not helpful. Just saying... :((((((
Comment #28
catchThat's mainly why I didn't hold the patch up. This doesn't completely remove hook_menu() or change what's required there, nor does it completely remove the router - it just removes one part of the backwards compatibility layer for it. That the docs are out of sync is bad, but it's not this issue's fault.
Comment #29
effulgentsia CreditAttribution: effulgentsia commented#2044969-36: Break the old router for contrib contains some updates to hook_menu() docs. Perhaps we can start by extracting that into a docs-only patch? Should we open a new issue for that?
Comment #30
jhodgdonThere is already another issue:
#2046367: Document the internals of the router
Comment #31
Crell CreditAttribution: Crell commentedShouldn't this issue get properly reclassified as a meta issue, with appropriate title? If not, we need a new meta it sounds like...
Comment #32
tim.plunkettComment #33
sunComment #34
xjmSee #23 and #25. Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #36
dawehner#2107533: Remove {menu_router} together with [#1800686] should really cover it properly.
Comment #37
ianthomas_ukRestoring title/priority
Comment #38
xjmComment #39
jessebeach CreditAttribution: jessebeach commentedThis issue just doesn't want to give up the needs change record tag!
Removed 'Needs change record' again.