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
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#23 | design_test_followup-23.patch | 952 bytes | ParisLiakos |
#22 | design_test_followup.patch | 941 bytes | tstoeckler |
#19 | interdiff.txt | 697 bytes | jibran |
#19 | 1987688-19.patch | 4.92 KB | jibran |
#14 | convert-design_test_category_page-to-controller-1987688-14.patch | 4.95 KB | CB |
Comments
Comment #1
CBComment #2
CBPatch attached.
Its worth noting that while this callback has a '$categories' parameter - it isn't actually used in the callback. I've left the code as-is though.
Comment #3
CBComment #5
CBKicking the bot off again as local tests were all green.
Comment #6
CB#2: convert-design_test_category_page-to-controller-1987688-2.patch queued for re-testing.
Comment #7
dawehnerJust some nitpicking...
No need for the interface if there is nothing to inject.
Comment #8
CBRemoved unused interface and rerolled against fresh 8.x (no changes)
Comment #9
larowlanNot needed, no DI happening here.
no need for 'implements ControllerInterface' when no DI
this can be removed, no DI going on here.
not needed
Comment #10
CBThanks larowlan, will update (hopefully) tonight.
Comment #11
dawehnerCrell agreed that it actually makes sense to add all of them now already as all of them will need injection on the long run.
Comment #12
larowlanSo in that case RTBC?
Comment #13
dawehner/me hides ... missing empty line , the rest is fine.
Comment #14
CBLine added. :)
Comment #15
CBComment #16
dawehnerGreat, thank you for your work!
Comment #17
alexpottVery minor nit...
This really should just be...
return menu_tree_output($tree);
Comment #18
alexpottSee #17
Comment #19
jibranFixed #17
Comment #20
tim.plunkettThis shouldn't actually work, I thought. Most other foreach loops need a RouteSubscriberEDIT: Nevermind, that only comes into play for access checks. Since these are all
_access: 'TRUE'
, it doesn't matter.Comment #21
alexpottCommitted d84dd29 and pushed to 8.x. Thanks!
Comment #22
tstoecklerThis is not compatible with the parent declaration.
It seems design_test is a bit broken in general, but I don't know in how far that is related to this patch. This fixes the fatal, at least.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedstraight reroll for
ControllerInterface
movingComment #24
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #25
alexpottI don't understand how tests are currently passing...
Well scanning the code it appears that the test module design_test is not being used.
Comment #26
alexpottAdded #2037569: Remove design_test module
Comment #27
alexpottI'm closing this issue in favour of #2037569: Remove design_test module - obviously we still need the followup fix from this issue but lets do that over there...
Adding tests is vital since the fact that they were missing is how we have ended up with a broken module.