Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

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.

xjm’s picture

Title: Convert overlay_ajax_render_region to a Controller » Convert 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.

jibran’s picture

laurentchardin’s picture

Status: Active » Needs work
FileSize
3.22 KB

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

laurentchardin’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

Only adding some ending newlines.

laurentchardin’s picture

Assigned: Unassigned » laurentchardin

Assigning to me for the moment.

laurentchardin’s picture

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

dawehner’s picture

+++ 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,

laurentchardin’s picture

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.

laurentchardin’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

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

dawehner’s picture

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

laurentchardin’s picture

FileSize
3.04 KB

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

dawehner’s picture

Issue tags: +Needs manual testing

Add tags

ParisLiakos’s picture

+++ 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

Crell’s picture

Issue tags: +Novice

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

kbentham’s picture

Rerolled the patch and I also fixed the todo syntax.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Bot can object if it wants.

tstoeckler’s picture

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.

Crell’s picture

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.

kbentham’s picture

Status: Needs work » Needs review
FileSize
809 bytes
58.37 KB

I have rerolled the patch without the hook_menu.

dawehner’s picture

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.

kbentham’s picture

Status: Needs work » Needs review
FileSize
3.68 KB

I believe this is the correct patch.

kbentham’s picture

I believe this is the correct patch.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Let's try this again...

alexpott’s picture

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!

tstoeckler’s picture

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

tstoeckler’s picture

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