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.

Files: 
CommentFileSizeAuthor
#23 drupal8.overlay-module.1978932-23.patch3.68 KBkbentham
PASSED: [[SimpleTest]]: [MySQL] 58,159 pass(es).
[ View ]
#21 drupal8.overlay-module.1978932-21.patch58.37 KBkbentham
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es).
[ View ]
#21 interdiff.txt809 byteskbentham
#17 drupal8.overlay-module.1978932-17.patch3.75 KBkbentham
PASSED: [[SimpleTest]]: [MySQL] 58,336 pass(es).
[ View ]
#17 interdiff.txt1.17 KBkbentham
#13 drupal-1978932-13.patch3.04 KBlaurentchardin
PASSED: [[SimpleTest]]: [MySQL] 56,164 pass(es).
[ View ]
#11 drupal-1978932-11.patch3.19 KBlaurentchardin
PASSED: [[SimpleTest]]: [MySQL] 56,297 pass(es).
[ View ]
#5 drupal-1978932-5.patch3.04 KBlaurentchardin
FAILED: [[SimpleTest]]: [MySQL] 55,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#4 drupal-1978932.patch3.22 KBlaurentchardin
FAILED: [[SimpleTest]]: [MySQL] 55,939 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Postponed

Since 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.

Title:Convert overlay_ajax_render_region to a ControllerConvert overlay_ajax_render_region() to a Controller

Since this is an AJAX callback rather than a full page callback, we should wait on #1944472: Add generic content handler for returning dialogs.

Status:Active» Needs work
StatusFileSize
new3.22 KB
FAILED: [[SimpleTest]]: [MySQL] 55,939 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

First 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 ?)

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Only adding some ending newlines.

Assigned:Unassigned» laurentchardin

Assigning to me for the moment.

Just figured out that http://drupal.org/node/1978938 was also adding the controller... Shouldn't those issue be merged ?

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,49 @@
+class OverlayController implements ControllerInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }
+
+  /**
+   * Constructs a OverlayController object.
+   */
+  public function __construct() {
+  }

... No need for the ControllerInterface, constructor and __construct method,

I had a talk with Crell, that says he prefers to keep them for consistency.

Status:Needs review» Needs work

The last submitted patch, drupal-1978932-5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
PASSED: [[SimpleTest]]: [MySQL] 56,297 pass(es).
[ View ]

Ok... Forgot that when you returning the full response object, you should use _controller in your routing definition.

We still have to figure out how to proper test that manually, which we haven't been sure, sadly.

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,49 @@
+   * This function is intended to be called via Ajax.

I guess we can skip this.

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,49 @@
+   * @param $region

should be @param string ...

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,49 @@
+  public function regionRender($region) {

needs @return

StatusFileSize
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 56,164 pass(es).
[ View ]

Definitely could use some guidance to identify use cases when this is called so we could build some test cases.

Issue tags:+Needs manual testing

Add tags

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,49 @@
+ * TODO: keeping the controllerInterface since we should be injecting
+ * something to take care of the overlay_render_region() call.

no need for this. but even if you keep it, it should be @todo

+++ b/core/modules/overlay/lib/Drupal/overlay/Controller/OverlayController.phpundefined
@@ -0,0 +1,49 @@
+   * TODO: add a DI for managing the overlay_render_region() call.

same

Issue tags:+Novice

The comment changes from #15 should be easily Novice-able; I think the patch is ready otherwise.

StatusFileSize
new1.17 KB
new3.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,336 pass(es).
[ View ]

Rerolled the patch and I also fixed the todo syntax.

Status:Needs review» Reviewed & tested by the community

Thanks! Bot can object if it wants.

Status:Reviewed & tested by the community» Needs review

+++ b/core/modules/overlay/overlay.module
@@ -28,11 +28,11 @@ function overlay_help($path, $arg) {
-  $items['overlay-ajax/%'] = array(
...
+  $items['overlay/dismiss-message'] = array(

This 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.

Status:Needs review» Needs work

Hm. 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.

Status:Needs work» Needs review
StatusFileSize
new809 bytes
new58.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,155 pass(es).
[ View ]

I have rerolled the patch without the hook_menu.

Status:Needs review» Needs work

You maybe have made a diff against the wrong branch? This seems to include quite some of the recent changes of core.

Status:Needs work» Needs review
StatusFileSize
new3.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,159 pass(es).
[ View ]

I believe this is the correct patch.

I believe this is the correct patch.

Status:Needs review» Reviewed & tested by the community

Thanks. Let's try this again...

Status:Reviewed & tested by the community» Fixed

I'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!

Opened #2074125: Improve OverlayController as a follow-up, originally, but broadened scope a bit there. Would welcome reviews nonetheless. :-)

Automatically closed -- issue fixed for 2 weeks with no activity.