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 | drupal8.overlay-module.1978932-23.patch | 3.68 KB | kbentham |
#21 | drupal8.overlay-module.1978932-21.patch | 58.37 KB | kbentham |
#21 | interdiff.txt | 809 bytes | kbentham |
#17 | drupal8.overlay-module.1978932-17.patch | 3.75 KB | kbentham |
#17 | interdiff.txt | 1.17 KB | kbentham |
Comments
Comment #1
xjmSince this is an AJAX callback rather than a full page callback, we should wait on #1944472: Add generic content handler for returning dialogs. It seems to already return a Response object.
Comment #2
xjmSince this is an AJAX callback rather than a full page callback, we should wait on #1944472: Add generic content handler for returning dialogs.
Comment #3
jibran#1944472: Add generic content handler for returning dialogs is in.
Comment #4
laurentchardin CreditAttribution: laurentchardin commentedFirst try at it during drupalcon sprint.
Questions:
- is the OverlayController naming ok since it is only used for ajax calls ? Was thinking about using OverlayAjaxController instead.
Left to be done :
- probably find a way to get rid of the hardcoded overlay_render_region() call (through the injection of a service ?)
Comment #5
laurentchardin CreditAttribution: laurentchardin commentedOnly adding some ending newlines.
Comment #6
laurentchardin CreditAttribution: laurentchardin commentedAssigning to me for the moment.
Comment #7
laurentchardin CreditAttribution: laurentchardin commentedJust figured out that http://drupal.org/node/1978938 was also adding the controller... Shouldn't those issue be merged ?
Comment #8
dawehner... No need for the ControllerInterface, constructor and __construct method,
Comment #9
laurentchardin CreditAttribution: laurentchardin commentedI had a talk with Crell, that says he prefers to keep them for consistency.
Comment #11
laurentchardin CreditAttribution: laurentchardin commentedOk... Forgot that when you returning the full response object, you should use _controller in your routing definition.
Comment #12
dawehnerWe still have to figure out how to proper test that manually, which we haven't been sure, sadly.
I guess we can skip this.
should be @param string ...
needs @return
Comment #13
laurentchardin CreditAttribution: laurentchardin commentedDefinitely could use some guidance to identify use cases when this is called so we could build some test cases.
Comment #14
dawehnerAdd tags
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedno need for this. but even if you keep it, it should be @todo
same
Comment #16
Crell CreditAttribution: Crell commentedThe comment changes from #15 should be easily Novice-able; I think the patch is ready otherwise.
Comment #17
kbentham CreditAttribution: kbentham commentedRerolled the patch and I also fixed the todo syntax.
Comment #18
Crell CreditAttribution: Crell commentedThanks! Bot can object if it wants.
Comment #19
tstoecklerThis looks like a merge conflict. In the patches above, 'overlay-ajax/%' is simply being removed. Was 'overlay/dismiss-message' removed already in the meantime?
Setting back to needs review for confirmation.
Comment #20
Crell CreditAttribution: Crell commentedHm. Yeah, it does look like it. Bah. What conflicted?
Anyway, kbentham can you reroll again? It looks like we can just remove the whole hook_menu() and be done with it.
Comment #21
kbentham CreditAttribution: kbentham commentedI have rerolled the patch without the hook_menu.
Comment #22
dawehnerYou maybe have made a diff against the wrong branch? This seems to include quite some of the recent changes of core.
Comment #23
kbentham CreditAttribution: kbentham commentedI believe this is the correct patch.
Comment #24
kbentham CreditAttribution: kbentham commentedI believe this is the correct patch.
Comment #25
Crell CreditAttribution: Crell commentedThanks. Let's try this again...
Comment #26
alexpottI've committed this because removing all routes from hook_menu is a valid goal in itself but we need to get the followup created to complete the
@todo add a DI for managing the overlay_render_region() call.
Committed f37f7a1 and pushed to 8.x. Thanks!
Comment #27
tstoecklerOpened #2074125: Improve OverlayController as a follow-up, originally, but broadened scope a bit there. Would welcome reviews nonetheless. :-)
Comment #28
tstoecklerAlso opened #2074151: Inject overlay_render_region() into OverlayController