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.
Follow-up from #1971384: [META] Convert page callbacks to controllers.
73 instances of 31 callbacks. Something to do on the plane!
Comment | File | Size | Author |
---|---|---|---|
#44 | page-callback-2091691-44.patch | 141.85 KB | tim.plunkett |
#44 | interdiff.txt | 1.03 KB | tim.plunkett |
#41 | page-callback-2091691-41.patch | 142.87 KB | tim.plunkett |
#42 | interdiff.txt | 16.31 KB | tim.plunkett |
#39 | page-callback-2091691-39.patch | 141.23 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettHere's a start.
Comment #3
tim.plunkettHere are some more.
Comment #5
tim.plunkettMore work, still some left.
Comment #7
tim.plunkettComment #9
tim.plunkett63 down, 11 to go.
Comment #11
tim.plunkettOkay, this is all of them, but will still have failures.
Comment #13
tim.plunkettRemoved:
testModuleInvokeAllDuringLoadFunction()
testMenuLoadArgumentsInheritance()
Neither of these are relevant anymore, they do not work with routes at all.
Comment #17
tim.plunkettI think I found the last couple fails.
Comment #18
dawehnerWait, are you sure about that change?
So is there a reason why we don't have a title callback on the route?
This seems to be out of scope?
nice!!
We can't convert this to a title callback on the route?
WOW
Comment #19
dawehnerIt would be great to add a todo that entities should not deal with paths but rather set the route names.
Comment #20
tim.plunkett18.1
See drupal_set_page_content(), the fallback uses 'main', and our route based one was written wrong.
18.2
I've opened #2102391: Provide a _title_callback replacement for entity_page_label() for entity_page_label()
18.3
We had test coverage for this (see the fails in #15) so I converted it
18.5
I don't actually touch those lines, just indenting them...
19
That will be fixed by #1810350: Remove menu_* entity keys and replace them with entity links, so I'm not too worried.
Comment #22
dawehnerWell at least the patch you have pointed just adds even more paths to the entity definitionm, anyway.
Comment #23
tim.plunkettUgh, stupid fixes needed in menu.inc.
Also, git bisect + simpletest is depressing, but it works.
Comment #25
tim.plunkettI went down the rabbit hole there, but @dawehner had a good suggestion.
Comment #27
tim.plunkettComment #29
tim.plunkettComment #30
tim.plunkettOkay, while that passed, I think its not ideal.
I went that way because I couldn't get the local tasks to work, but I think I figured it out.
Also switching from $entity to $_entity as we do everywhere else when it's an internal param.
Comment #31
tim.plunkettFor anyone following along, here's the interdiff from the last reviewed patch (#17)
Comment #35
tim.plunkettUgh, I broke d.o. Missed one change.
Comment #36
dawehnerThis change is kind of odd as I kind of expect that both usecases are valid.
Comment #37
tim.plunkettOkay, switching them back.
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedso we added an if, lets add the indentation too
win!
i guess this is not blocking
i cant see this one added as route. removed deliberately?
awesomeness
Comment #39
tim.plunkett1) The diff is now going to look insane, be warned :)
3) I don't think it should be.
4) The usage of that callback was removed back in #1862398: [meta] Replace drupal_http_request() with Guzzle, so I just removed the callback
No interdiff because its just the indentation fix.
Comment #40
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, well this identation change made that patch unreviewable:P
i am glad i reviewed it on its previous form
lets get this in quickly, unless bot disagress
Comment #41
tim.plunkettActually, I find that incredibly hard to understand, so I'm splitting it out.
The interdiff is against #37.
Actually, due to #2103877: Patch uploads are failing I can't upload it. will as soon as I can
Comment #42
tim.plunkett29/30 uploads give me a 500. yay.
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commenteda lot better indeed. Thanks!
Comment #44
tim.plunkettIn IRC, @alexpott pointed out that until #2084463: Convert contextual links to a plugin system similar to local tasks/actions happens, we can't make those changes to content_translation_menu(). Restored that.
Here is a "reverse review" of the patch, highlighting some oddities. Most of this is at the top of the patch, the end of the patch is pretty standard.
These are the only functional changes here. The rest is just moving the GIANT hunk of legacy code into a helper method, because it was very confusing to me which was legacy and which wasn't.
The top hunk is fixing a bug where the hook_menu_local_tasks/hook_menu_local_tasks_alter weren't being called for plugin-based tasks. It was trying to bail out early when a menu_router item wasn't found, but that's not enough anymore.
The second hunk was due to huge WTFs with the alters, since *sometimes* menu_get_item() will return the hook_menu() parent of a new routing.yml path. In cleaning that up, we now only pass along the route name.
As explained above, this is a bug fix to make it match drupal_set_page_content(). The test for this behavior was only testing page callbacks, and now that its testing routes we found this mistake.
These two, and all hunks like them, are more of the same changes we've made elsewhere. This is the MENU_DEFAULT_LOCAL_TASK path, which is never presented as a link in the D7 UI, but is used in test code. They would fall through to the real path in D7, but will 404 in D8.
All of this is needed because content_translation needs to be all things to all entity types. The config_translation module is having some of the same pains, which I was just recently helped with. This is not normal, unless your module is trying to be a generic layer on top of a dozen things, you won't have to write stuff like this
These tests are removed because we're removing support for magic menu loaders (%node => node_load), you need a ParamConverter
MENU_CALLBACKs don't need the leftover hook_menu definitions when converted
Just switching from doing 1 thing in a loop to 2 things
Comment #46
tim.plunkett#44: page-callback-2091691-44.patch queued for re-testing.
Comment #47
tim.plunkettBase table or view not found: 1146 Table 'drupaltestbotmysql.simpletest264828menu_links' doesn't exist
Testbot fluke.
Comment #48
webchickSo I had a huge review written out which some combination of Dreditor and Firefox then lost, but it's OK because all of the points I was going to bring up (and then some) were already pre-emptively addressed in #44.
Just one question:
What's left to complete this so we can remove these 100+ lines of inscrutable code? :)
Committed and pushed to 8.x. Thanks!!!! :D
Comment #49
tim.plunkettThank you!
I think #2102125: Big Local Task Conversion is the major blocker for that, but possibly also #2102653: Convert test form callbacks to new form controller
Comment #50
tim.plunkett