Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #1971384: [META] Convert page callbacks to controllers
Will cover:
- menu_overview_page
- menu_parent_options_js
- menu_menu_add
- menu_menu_edit
- menu_link_add
- entity_get_form (used for admin/structure/menu/item/%menu_link/edit)
Comment | File | Size | Author |
---|---|---|---|
#70 | menu-1984702-70.patch | 38.3 KB | tim.plunkett |
#68 | menu-1984702-68-same.patch | 38.63 KB | tim.plunkett |
#68 | menu-1984702-68-no-route.patch | 38.3 KB | tim.plunkett |
#68 | interdiff.txt | 514 bytes | tim.plunkett |
#66 | menu-1984702-66.patch | 38.63 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettYay.
Comment #3
dawehnerCan't we get just the request object injected from the controller?
Do we actually really want to do that? This feels like a potential security issue.
Comment #4
tim.plunkettBlocks #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()
Comment #5
dawehnerTrying to fix the failures.
Comment #6
dawehnerDrupal has some interesting bits of code :)
It seems to make sense to move this menu_edit_menu_name_exists() as it's just use in a single place (the controller).
Sadly the breadbrumb test still fail, but I couldn't figure out yet why they actually fail.
Comment #8
dawehnerThis patch basically adds '/edit' to all paths, as routing doesn't work without it.
Does someone know of a central issue where this is discussed?
Comment #10
tim.plunkettSee #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK, I think we should go the other direction.
Comment #11
tim.plunkettI hate breadcrumbs, breadcrumbs suck.
Comment #12
tim.plunkettContextual links assume you want to go to MENU_DEFAULT_LOCAL_TASK. This will be a problem.
Comment #15
tim.plunkettOkay, I opened #2006636: menu_contextual_links() will always return a link to the MENU_DEFAULT_LOCAL_TASK, never the parent.
Comment #16
tim.plunkettThis incorporates that patch. Should pass menu tests just fine, but will likely break dozens of other tests.
Comment #18
tim.plunkettThat went in!
Comment #19
dawehnerAt least the factory should be injected. Beside from that it seems perfect so far.
Comment #21
dawehnerUps.
Comment #23
dawehner#21: menu-1984702-21.patch queued for re-testing.
Comment #24
tim.plunkettModernized the patch.
This now depends on #2008356: Provide a _entity_list route default to replace Controller\EntityListController and mimic _entity_form. It will fail, I'll just retest when that's in.
Comment #26
tim.plunkettRerolling around #1999398: Use Symfony Request for menu module.
Will still fail.
Comment #28
tim.plunkettThat went in.
Comment #29
dawehnerI guess we should inject the entity loading as well.
Comment #30
tim.plunkettThat and the rest of the form too.
Comment #32
tim.plunkettRerolled after #2011018: Reconcile entity forms and confirm forms
Comment #34
tim.plunkettBad variable name.
Comment #35
dawehnerGreat!
Comment #36
alexpottI think we should remove this static now we're OO :)
Comment #37
tim.plunkettAbsolutely.
Comment #38
Crell CreditAttribution: Crell commentedWFM.
Comment #39
alexpottNeeds a reroll...
Comment #40
tim.plunkettRerolling. Found some fun bugs in the process.
Comment #41
tim.plunkettThis contains #2027183: hook_menu() title callback is ignored on routes.
Posting a do-not-test patch without that part.
Comment #42
tim.plunkettBlergh.
Comment #43
YesCT CreditAttribution: YesCT commentedThis issue is taking out the unneeded $operation in the constructor of the MenuFormController that was put in in #1945226-143: Add language selector on menus
Related to:
#2027335: If EntityFormController implements EntityControllerInterface it can ignore $operation in createInstance()/__construct()
Comment #44
tim.plunkettThat went in, rerolled.
Comment #46
tim.plunkett#44: menu-1984702-44.patch queued for re-testing.
Comment #47
larowlanshould this be MenuLinkStorageControllerInterface now?
Should we add a todo to remove these once those two are part of a menu manager service? And add the follow up issue.
Should we add a todo to remove when we've resolved title/title callback issue?
out of scope:oh yuck, that surely belongs in a menu service too
Why is this removed?
Comment #48
tim.plunkettI opened the follow-up #2028249: Add a MenuManager service
But I disagree with linking that one inline, it could be a whole issue just to identify all the places to mark up.
I did add an @todo for the drupal_set_title().
Fixed the typehint, and renamed the variable in anticipation of #1976158: Rename entity storage/list/form/render "controllers" to handlers
That was removed because /edit is no longer a valid URL.
Comment #49
larowlanunless bot says otherwise
Comment #50
alexpottNeeds a reroll..
Comment #51
tim.plunkettThe getOperations issue added an access controller right where I was adding form controllers in the entity_info, so it conflicted. No changes.
Comment #52
dawehnerThanks for the rerole.
Comment #53
alexpottNeeds a reroll
Comment #54
tim.plunkettRerolled around #2025991: Introduce hook_entity_prepare_form() to generalize hook_node_prepare()
Comment #55
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #56
tim.plunkettRerolled for loadMultiple. Leaving RTBC.
Comment #57
tim.plunkettRerolled for #849624: wrong permission for admin/structure/menu/parents.
Just update the routing.yml to match.
Comment #58
tim.plunkettYes, thank you d.o, I definitely wanted to kill those tags...
Comment #59
tim.plunkettWell this was interesting. Major bugs in our new local tasks/actions.
Comment #61
dawehnerBefore we can even start with using parameters we need #2031487: When replacing the upcasted values in the request attributes array, retain the original raw value too to get in, because otherwise it will be full of upcasted stuff,
which will just produce a lot of brokenness all over the place.
Comment #62
tim.plunkettYes, I agree that is very much needed. My workarounds here are interesting but overkill.
Comment #63
dawehnerSo what about convert them not yet to the new local action api? This lets the patch stay way more focused.
Comment #64
tim.plunkettFine by me.
Comment #65
dawehnerThank you!
Bam! Potentially you could drop the second query if the first already returned something useful ...
Comment #66
tim.plunkettGood idea!
I also restored a test that was deleted, I don't know how that happened.
Also switched from using _permission and custom access checkers to the entity access checkers.
Comment #67
dawehnerNice! The issue is a classical example of what happens if you convert way too much at the same time.
Comment #68
tim.plunkettThis reuploads #66, but because it fails for me locally, trying a fix.
If they both pass, I'll be sad.
Comment #69
alexpottI'm sad on you're behalf... as I wouldn't know which patch is rtbc :)
Comment #70
tim.plunkettReuploading #66 again by itself, the interdiff from #68 doesn't do anything and I have no idea why I did that.
Comment #71
alexpottCommitted c5f81fb and pushed to 8.x. Thanks!
I think we need a change notice here because we've moved some function that menu manipulating modules in contrib might rely on.
Comment #72
andypostFollow-up clean-up #2012916: Implement access controller for the menu and menu link entity
Comment #73
tim.plunkettI'm told that no one will volunteer to write a change notice if the issue is assigned, and there's a sprint coming up.
Feel free to ping me if any questions arise.
Comment #74
star-szrTagging to write up the change record. Maybe we can start with before/after lists of functions and where they've moved to.
Comment #75
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
It sounds like there's possibly two separate tasks here: adding the callbacks to https://drupal.org/node/2119699, and possibly a separate change record to document other menu.module API changes.
Comment #76
Gábor HojtsyLooking at this patch, I'm not sure why this needs a change notice for "menu module API changes". There are no **API functions** affected by this patch, which is shown clear by the fact that only the page callback related stuff (routing.yml, page callback file, controller) is affected.
I see the idea behind https://drupal.org/node/2119699, but to adapt that to all page controller patches, we would need to reopen several dozens of issues for change notices. We did not document one-on-one how other page callbacks were moved to classes either, that change notice only mentions one of them, although it appears it was intended to broadly cover the whole thing.
Comment #77
xjm@Gábor Hojtsy, we don't need to reopen them -- just go down the list on the WSCCI meta, which I'm planning to ask some new contributors to work on in upcoming weeks. :) But if #71 is incorrect, and there are no menu module API changes, let's just add to the page callback list then and close this?
Comment #78
Gábor HojtsyOk, so we are not tracking that. That may be counter-productive if we want to get it done (we have a hard time getting done what we track). However, that is not related to *this* issue, so no need to discuss it here.
Added the page callback changes from this patch to https://drupal.org/node/2119699 and since there is no API change to document here otherwise AFAIS, closing.
Comment #79
webchickHm. I'm not really down with https://drupal.org/node/2119699 the way it is worded. We definitely do not want to waste anyone's time (let alone dozens of anyone's time) going through all of the WSCCI conversions and finding out what functions changed to what classes/methods, IMO. There is next to no reason that a module would ever call, say, menu_overview_page() directly. (And if there is, they can always look up the path in the routing file and figure out where it went.)
However, the title "Page callbacks converted to controllers" definitely deserves merit as a change notice; just the body should explain to module developers how to do this, using a D7 vs. D8 code example. This is indeed not a problem for this issue (or any other individual conversion issue) to solve, but is rather for #1971384: [META] Convert page callbacks to controllers.
Comment #80
Gábor HojtsyQuestion is if that would be a change notice or not. The WSCCI converison guide (several pages) exists to guide developers though that process: https://drupal.org/node/1953342
Comment #81
Crell CreditAttribution: Crell commentedThere's already a massive change notice for the routing system. (https://drupal.org/node/1800686) Probably too massive. And then the docs are in progress, and more complete than any change notice should be. Add a link to the docs to the existing routing change notice and call it a day. Listing individual callback->controller renames is pointless.
To expect people to learn how to upgrade their modules from D7 to D8 just from change notices is a fool's errand. Change notices are simply not suited to the level of change we're talking about here.