In #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases, we added generic AJAX commands for controlling dialogs. In that issue, we discovered that having a single menu callback (or router _content callback) generate both AJAX and non-AJAX content introduced callbacks that were doing multiple things based on the request headers. One of the primary selling points of the new router system is the ability to send back different content based on the specific type of content requested *at the same URL*, using different _content handlers.

We should utilize the new router system for cleanly separating _content handlers that provide actual content, and those that utilize the original content handlers and return the content in some different way (such as in a dialog). In order to facilitate simple "dialogification" of existing paths, we should add a generic _content handler that can wrap around any existing handler. This will make simple situations that use dialogs possible with the following:

- Adding a second router item at the path to be displayed in the dialog.
- Add the "use-ajax" class in the link to the URL to be opened in the dialog.
- Attach the drupal.ajax library so that the use-ajax link is detected.

Preferably, we could combine use-ajax and the drupal.ajax library requirements together, but how that should be implemented isn't yet decided.

The config.module was converted in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases to use the new AJAX commands, but would benifit from using separate router items as described above. We may convert that module as part of this issue as a demonstration, or separate it out into another issue.

Remaining tasks

Postponed on this

Related

CommentFileSizeAuthor
#68 dialog-route-1944472.68.patch29.61 KBlarowlan
#65 dialog-route-1944472.65.interdiff.txt8.2 KBlarowlan
#65 dialog-route-1944472.65.patch29.61 KBlarowlan
#61 config-diff.png41.38 KBeffulgentsia
#56 dialog-route-1944472.56.interdiff.txt798 byteslarowlan
#56 dialog-route-1944472.56.patch30.69 KBlarowlan
#54 dialog-route-1944472.54.interdiff.txt1.71 KBlarowlan
#54 dialog-route-1944472.54.patch30.69 KBlarowlan
#52 dialog-route-1944472.52.interdiff.txt681 byteslarowlan
#52 dialog-route-1944472.52.patch34.01 KBlarowlan
#46 dialog-route-1944472.46.patch30.84 KBeffulgentsia
#46 interdiff.txt6.45 KBeffulgentsia
#44 dialog-route-1944472.44.interdiff.txt6.81 KBlarowlan
#44 dialog-route-1944472.44.patch34.07 KBlarowlan
#41 dialog-route-1944472.41.interdiff.txt1.45 KBlarowlan
#41 dialog-route-1944472.41.patch29.44 KBlarowlan
#39 dialog-route-1944472.39.interdiff.txt632 byteslarowlan
#39 dialog-route-1944472.39.patch29.48 KBlarowlan
#35 dialog-route-1944472.35.interdiff.txt3.61 KBlarowlan
#35 dialog-route-1944472.35.patch29.45 KBlarowlan
#34 Screen Shot 2013-03-27 at 7.27.24 AM.png55.57 KBlarowlan
#34 Screen Shot 2013-03-27 at 7.27.35 AM.png49.78 KBlarowlan
#34 dialog-route-1944472.34.interdiff.txt12 KBlarowlan
#34 dialog-route-1944472.34.patch27.69 KBlarowlan
#31 Screen Shot 2013-03-26 at 7.49.59 PM.png28.43 KBlarowlan
#31 Screen Shot 2013-03-26 at 7.59.25 PM.png41.36 KBlarowlan
#31 Screen Shot 2013-03-26 at 7.59.35 PM.png21.73 KBlarowlan
#31 dialog-route-1944472.31.interdiff.txt4.11 KBlarowlan
#31 dialog-route-1944472.31.patch17.64 KBlarowlan
#13 dialog-route-1944472.13.interdiff.txt12.85 KBlarowlan
#13 dialog-route-1944472.13.patch16.17 KBlarowlan
#10 dialog-route-1944472.9.patch7.57 KBlarowlan
#4 config-dialog.1.patch7.52 KBlarowlan
#1 ajax-dialog-1870764.92.wip_.interdiff.txt7.52 KBquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Category: bug » task
FileSize
7.52 KB

@larowlan posted this patch in #1870764-94: Add an ajax command which makes it easy to use the dialog API in complex cases to demonstrate the concept. It's not a working patch, more a proof of concept on how we could convert config.module's "Show differences" button.

larowlan’s picture

Assigned: Unassigned » larowlan
Issue tags: +WSCCI-conversion

I'll keep kicking this along, been up to my eyeballs in symfony routing trying to make this work

quicksketch’s picture

Thanks @larowlan, I'll give this as much attention as I can, hoping I can be a help on some front. I can help be your "reroll support". ;)

larowlan’s picture

Status: Active » Needs review
FileSize
7.52 KB

Original patch was so close - the ajax one had to use _controller instead of _content.

Status: Needs review » Needs work

The last submitted patch, config-dialog.1.patch, failed testing.

larowlan’s picture

Will re roll tomorrow

Crell’s picture

Related: #1938980: Fix Ajax system; the last remnants of the old API must be swept away and #1945024: Remove subrequests from central controllers and use the controller resolver directly.. Note that currently the Ajax-controller-detection code is wrong, so the first issue there is probably a blocker for this one.

Otherwise, ++ on generalizing the ability to Ajax load an arbitrary page body. That's exactly the sort of coolness we want to enable.

jibran’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

Issue tags: +modal dialog

Tagging

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -modal dialog

So this is the patch from #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases that should apply cleanly now that is in.
However, we want a more generic approach that provides a dedicated controller that just forwards to the fallback version and wraps it in a dialog command.

larowlan’s picture

And this time with the patch

Crell’s picture

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -0,0 +1,114 @@
+  /**
+   * The target storage.
+   * @var \Drupal\Core\Config\StorageInterface;

I think technically we need a line break between the description and @var line. Same for $sourceStorage.

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -0,0 +1,114 @@
+   * Page callback: Shows diff of specificed configuration file.

We don't really have a standard for prefixing docblocks of controller methods yet, but "Page callback" is not it. :-) Probably we can just rip that part off and leave the rest.

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -0,0 +1,114 @@
+    $formatter = new \DrupalDiffFormatter();

Probably off topic here, but... why is DrupalDiffFormatter in the global namespace???

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -0,0 +1,114 @@
+   * Page callback: Shows diff of specificed configuration file.

Ibid.

I really like what we're able to do here with the routing, though. :-) Is there a way to genericize that behavior, perhaps?

larowlan’s picture

Status: Needs review » Needs work

I really like what we're able to do here with the routing, though. :-) Is there a way to genericize that behavior, perhaps?

Yeah that is the ultimate goal here, we need a \Drupal\Ajax\DialogController and \Drupal\Ajax\ModalDialogController then mark the routes to use that in your yml, setup the 'use-ajax' class on the link and you're done.

I think technically we need a line break between the description and @var line. Same for $sourceStorage.

Yep, missed that - thanks

Probably off topic here, but... why is DrupalDiffFormatter in the global namespace???

My thoughts exactly -definite wtf.

larowlan’s picture

Tagging for nod_ and JS team.
This is the generic implementation.
In order to make a link load its contents in a dialog you need to a) use the new routing system and b) make sure your link has the following attributes

        'attributes' => array(
          'class' => array('use-ajax'),
          'data-accepts' => 'application/vnd.drupal-modal'
        ),

If you don't want a modal, use 'application/vnd.drupal-dialog'

This just works. You cannot begin to comprehend how fully awesome the new routing system is until you see stuff like this.
At the moment all that I've converted is the config module's diff dialog.

Next step is to convert the ajax_test module's dialogs.

Then we need to wait for #1938980: Fix Ajax system; the last remnants of the old API must be swept away and rework the bit in the RouteProcessorSubscriber to use a Route Enhancer instead.

larowlan’s picture

Status: Needs work » Needs review

Lets see if it breaks anything else

larowlan’s picture

Issue tags: +JavaScript

Lost tags

larowlan’s picture

Issue tags: +Needs tests

Need to add tests for the new accepts behaviour

mkadin’s picture

This looks quite good to me and falls in line with what already exists for Ajax. The idea of hitting node/1 with a custom mime type and getting back a modal has me drooling over D8! Looks like #1938980: Fix Ajax system; the last remnants of the old API must be swept away is RTBC, so maybe we should wait until that's in so we can refactor and then write tests.

As a side note, I wonder if there's a way to do something a bit more general so that some of the code that's repeated between Ajax, Modal, and Dialog controllers can be consolidated...I don't think it's particularly important for the moment, but if we start to add more of these mime-type based controllers...it might make our lives easier. Or if it was a generalized class, it could make it easy for modules to add their own custom controller for a custom mime type. Just a thought.

larowlan’s picture

Turns out this works for existing menu links (hook_menu) too.
I just created an article node, then a second article node, added a link with that class and data-accepts in the second node pointing to the first node and it loaded node/1 in a modal!!!!

larowlan’s picture

larowlan’s picture

Things to consider: we should be able to control modal size from data- attributes

effulgentsia’s picture

we should be able to control modal size from data- attributes

Agreed. How about if a data- attribute could hold default dialogOptions that Drupal.ajax.prototype.commands.openDialog() extends with response.dialogOptions instead of using only response.dialogOptions?

quicksketch’s picture

Things to consider: we should be able to control modal size from data- attributes

I think in most cases the size of the dialog would be determined by the content being returned. If you were opening the same URL from multiple places, it would probably be the same size in all of them (maybe not though). I think I'm basically saying that we should keep the settings server-side rather than making our generic handler provide all kinds of options that are based on client-side specifications. If we go down that road I won't be offended, but I'd like to see client-side specifications be a secondary option rather than the de facto approach.

Speaking of, this code seems a bit too rigid:

+    // @todo Move this to a RouteEnhancer when http://drupal.org/node/1938980
+    // lands.
+    elseif ($this->negotiation->getContentType($request) == 'drupal_dialog') {
+      if (!$request->attributes->has('_content') && $request->attributes->has('_controller')) {
+        // Pass the controller on as content.
+        $request->attributes->set('_content', $request->attributes->get('_controller'));
+      }
+      $request->attributes->set('_controller', '\Drupal\Core\Ajax\DialogController::dialog');
+    }
+    // @todo Move this to a RouteEnhancer when http://drupal.org/node/1938980
+    // lands.
+    elseif ($this->negotiation->getContentType($request) == 'drupal_modal') {
+      if (!$request->attributes->has('_content') && $request->attributes->has('_controller')) {
+        // Pass the controller on as content.
+        $request->attributes->set('_content', $request->attributes->get('_controller'));
+      }
+      $request->attributes->set('_controller', '\Drupal\Core\Ajax\DialogController::modal');
+    }

It basically looks to me like the controller is forcibly set based on the request content type. Does this mean that a module couldn't (or shouldn't) set up a different route/content handler specifically for dialogs? Is that still an option with this approach, and what would it look like?

As a guess, if you wanted to have a customized dialog, would you you use data-accepts="application/vnd.drupal-ajax" and return a set of custom AJAX commands? Or would you still use "application/vnd.drupal-dialog"?

effulgentsia’s picture

This looks great. Just some minor nits.

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -0,0 +1,107 @@
+        if (isset($output['title'])) {
+          $title = $output['title']['#value'];
+          unset($output['title']);
+        }

This can be improved with #1830588: [META] remove drupal_set_title() and drupal_get_title(), but we shouldn't hold up this issue on that.

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -0,0 +1,107 @@
+      $options = array('width' => '700');

Per #21, let's not have this here.

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -0,0 +1,107 @@
+      $response->addCommand(new OpenDialogCommand('#drupal-modal', $title, $content, $options));

#drupal-modal doesn't make sense as the selector for non-modals. I suppose if we think it's useful to have a generic DOM element for non-modals, we can simply rename it to 'drupal-dialog'. However, the nature of non-modals is you can have multiple open at a time, so that's why I originally thought that for non-modals we need to require the specific use case (response or link data- attribute) to explicitly specify the selector.

effulgentsia’s picture

It basically looks to me like the controller is forcibly set based on the request content type. Does this mean that a module couldn't (or shouldn't) set up a different route/content handler specifically for dialogs? Is that still an option with this approach, and what would it look like?

It's not forcibly set, it's only defaulted if the route doesn't have an explicit _controller (look again at the if statement). So, you can have a drupal_dialog (application/vnd.drupal-dialog) route explicitly define a _controller in the routing file (in addition to the _content) if you want a specific one for that route.

effulgentsia’s picture

Oh, and if a module wanted to define a different default _controller to use for all routes that don't have an explicit one (i.e., swap out Drupal\Core\Ajax\DialogController with something else entirely), it could do that by defining a higher priority RouteProcessorSubscriber that would run before this one.

effulgentsia’s picture

I think in most cases the size of the dialog would be determined by the content being returned.

Good point. First of all, I hope in #1918744: Define some dialog CSS classes for common dialog sizes, we move away from pixel sizes, and define things like "small", "medium", and "large". But whether the link should have precedence or the server response should have precedence is a little tricky. To set it from the server, but not hard-code it in the generic controller, we could add some info to the route. For example:

config_diff:
  pattern: '/admin/config/development/sync/diff/{config_file}'
  defaults:
    _content: '\Drupal\config\Controller\ConfigController::diff'
    _content_size: 'large'

That only handles size though. Alternatively, instead of _content_size, we could define something like _dialog_options?

mkadin’s picture

RE: module's defining their own module controllers...they don't need to use the mime-type at all...they can just use the Ajax commands to open and close dialogs as they wish. The purpose of the mime-type routing is to take content that wasn't pre-determined to be in a modal to be modalizable.

I disagree (but won't freak out if it goes the other way) with the server-side implementation for sizing. Because this mime-type approach gives us the ability to put any content into a modal, it makes sense to me to handle its presentation after the fact. For instance, if I want to pull up nodes in a modal by linking to /node/{nid} with the custom mime-type, I don't think core should predetermine (in the node module's YAML routing file) the size of that modal.

Crell’s picture

Side note: The screencast in #19 makes me drool.

If a contrib module wants to change the default-controller logic, it can rip out the existing enhancers and add its own if it wants to, via a Compiler Pass. There's a lot of flexibility there. Depending on how things go I could be persuaded to move controller selection from a series of enhancers to a series of objects that a single enhancer uses, much the way we are currently handling parameter conversion. I don't know if that makes sense or not, but I'm open to discussing it later if it would be more performant/deterministic.

I don't have a strong opinion on server-side vs. client-side sizing, since I don't understand the problem space as well as I'd like. Couldn't we have some site-wide default, overridden by a per-route default, overridden by a client-side specification? (That seems to be the Drupally answer, anyway.)

quicksketch’s picture

On dialog sizes, I posted to #1918744: Define some dialog CSS classes for common dialog sizes basically saying we shouldn't have any sizes at all other than what's defined in CSS, assuming we can figure out the technical details of that approach. A requirement of that working though is giving each dialog a class that it can be targeted by. If we can figure out a good generic way of handling that here too, that would be great.

falcon03’s picture

Question from a novice point of view: is this a blocker for converting Overlay to use the new dialog API to solve
#890288: Improve Overlay accessibility by using modal dialogs

larowlan’s picture

So this patch supports

data-dialog-options='{"width": 700, "target": "some-target"}'

To allow the front-end to pass in options.
Not sure if there is an XSS threat vector here, looked into the ajax.dialog.js, looks like the selector ends up getting straight into $() but has to be prefaced with a # so I think that is safe.
Some screenshots

The markup showing three dialogs of varying sizes, targets and modal state
Screen Shot 2013-03-26 at 7.49.59 PM.png
Screenshot of two of the non-modals open with different widths
Screen Shot 2013-03-26 at 7.59.25 PM.png
Screenshot of the modal at a nominated width
Screen Shot 2013-03-26 at 7.59.35 PM.png

So to-do here is tests.

effulgentsia’s picture

is this a blocker for converting Overlay to use the new dialog API

I don't think so. Just like the config diff dialog that's in HEAD works fine without this, I think Overlay related work can proceed without this as well. Possibly, once this lands, some Overlay integration code can be cleaned up to leverage it, but even if that's the case, it represents a small fraction of the total work needed in rethinking Overlay based on dialogs. I only recommended postponing #1842036: [META] Convert all confirm forms to use modal dialog on this, because that issue is practically nothing but marking URLs/links that should appear in dialogs, and this issue changes the way that's done.

falcon03’s picture

@effulgentsia#32: thank you very much for this information. I was wondering if this was a blocker for the overlay conversion while I was looking at #1842036: [META] Convert all confirm forms to use modal dialog.

larowlan’s picture

So this refactors the tests to use this method.
Also some minor coding standards cleanups in DialogTest and documents WebTestBase::drupalGetAJAX as it took me ages to work the headers out.

So this gives us
*automatic route enhancement to dialog/modal based on data-accepts attribute for links (with test coverage)
*pass through of dialog options via data-dialog-options attribute for links (with test coverage)

We're now stuck waiting for #1938980: Fix Ajax system; the last remnants of the old API must be swept away

Shots of config module using it at different widths
Screen Shot 2013-03-27 at 7.27.24 AM.png

Screen Shot 2013-03-27 at 7.27.35 AM.png

larowlan’s picture

So now that route enhancers are in, switched this to use those instead of the route subscriber.

Lets see what breaks.

This is ready for serious reviews/consideration now the blocker is out of the way.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs tests, -WSCCI-conversion

The last submitted patch, dialog-route-1944472.35.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

#35: dialog-route-1944472.35.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs tests, +WSCCI-conversion

The last submitted patch, dialog-route-1944472.35.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
29.48 KB
632 bytes

Missed the return

Status: Needs review » Needs work

The last submitted patch, dialog-route-1944472.39.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
29.44 KB
1.45 KB

We need to handle legacy routes too (for now).
Also had controller instead of _controller.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Routing/Enhancer/DialogEnhancer.php
@@ -0,0 +1,63 @@
+      if (empty($defaults['_content']) && !empty($defaults['_controller'])) {

Only for the AjaxEnhancer do we currently have this nonsensical if statement. For the other enhancers, we only set _controller for routes that *don't* already have one and *do* have _content (or _form). AjaxEnhancer does it this backwards way to support legacy routes, but why do we need to support legacy routes for the new dialog controller? I'd be fine with saying you need to convert routes to the new routing system in order to benefit from the generic dialog controller.

larowlan’s picture

Status: Needs review » Needs work

Works for me - gives folks an incentive :)
Means I have to move the test module over to make it work but no big deal.

larowlan’s picture

Status: Needs work » Needs review
FileSize
34.07 KB
6.81 KB

Removes legacy route support.

larowlan’s picture

Ok, back to green, ready for serious reviews.

effulgentsia’s picture

Issue tags: -Needs tests
FileSize
6.45 KB
30.84 KB

This removes the ViewSubscriber changes that are being handled in a separate issue (#1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply).

And this makes a couple other cleanups related to #42.

The tests here look good, so removing the "Needs tests" tag.

I haven't finished reviewing everything, so am not quite ready to RTBC it yet, but reviews or RTBC from others welcome.

Crell’s picture

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -0,0 +1,134 @@
+        // Legacy fallback.
+        // @todo remove when http://drupal.org/node/1830588 lands and all routes
+        // converted to return render arrays.
+        $title = drupal_get_title();

Routes won't be all converted to return render arrays. They'll be returning PageFragment or whatever it gets called. Same impact (we can get rid of drupal_set_title()), but the @todo should be accurate.

+++ b/core/lib/Drupal/Core/Routing/Enhancer/ModalEnhancer.php
@@ -0,0 +1,32 @@
+class ModalEnhancer extends DialogEnhancer {

Interesting. It may make sense to generalize this further. I've been pondering if we're going to end up with more enhancers than we want this way, and some kind of standardized registration would work better. Not something to change in this patch, but something to consider.

Overall this looks sensible. If I read it right, it means that we can modal-ify *any* _content-using route, right? If so, that makes me warm and fuzzy inside.

effulgentsia’s picture

Routes won't be all converted to return render arrays. They'll be returning PageFragment or whatever it gets called.

Just for clarity, per #1594870-26: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply (which is my stance, not Crell's), I agree with Crell that the result of a forward() call of a _content using route will be a standardized object like PageFragment. My only disagreement in that issue is that I think the _content controller itself should generally return a render array (or possibly string, if we want to continue supporting that for 'hello world' controllers), so that the creation of a PageFragment object is centralized in a View subscriber.

but the @todo should be accurate

This code will need to be touched when PageFragment lands anyway. In the meantime, let's keep the @todo accurate for HEAD as it is now. If someone comes up with a way to word it so that it's both accurate now, and gives people a heads up on what the future will be, then that's ok too.

If I read it right, it means that we can modal-ify *any* _content-using route, right? If so, that makes me warm and fuzzy inside.

Agreed. larowlan++.

larowlan’s picture

<cheeky>So does that mean we're RTBC?</cheeky>

effulgentsia’s picture

Almost. I haven't tested manually, but from looking at the code, I think my only concerns are:

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -0,0 +1,134 @@
+        else {
+          // Generate a target based on the controller.
+          $target = '#drupal-dialog-' . drupal_html_id(drupal_clean_css_identifier(drupal_strtolower($_content)));
+        }

How does this end up working? I don't see whose responsibility it is to get this element into the DOM.

+++ b/core/modules/config/config.module
@@ -48,18 +48,9 @@ function config_menu() {
-  $items['admin/config/development/sync/diff/%'] = array(
-    'title' => 'Configuration file diff',
-    'description' => 'Diff between active and staged configuraiton.',
-    'page callback' => 'config_admin_diff_page',
-    'page arguments' => array(5),
-    'access arguments' => array('synchronize configuration'),
-    'file' => 'config.admin.inc',
-  );

I think this means that for nojs requests, that the HTML response lacks a title and breadcrumb. Should we keep a stub hook_menu() entry around for that, like we've been doing for other menu links whose route info we move to the new system?

larowlan’s picture

Status: Needs review » Needs work

I think this means that for nojs requests, that the HTML response lacks a title and breadcrumb. Should we keep a stub hook_menu() entry around for that, like we've been doing for other menu links whose route info we move to the new system?

Yes you're right, setting to needs work.

How does this end up working? I don't see whose responsibility it is to get this element into the DOM.

Its in the existing js from #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases

    var $dialog = $(response.selector);
    if (!$dialog.length) {
      // Create the element if needed.
      $dialog = $('<div id="' + response.selector.replace(/^#/, '') + '"/>').appendTo('body');
    }
larowlan’s picture

Status: Needs work » Needs review
FileSize
34.01 KB
681 bytes

Adds back the hook_menu entry for breadcrumbs etc

nod_’s picture

Seems like there are some redundant checks in ajax.js:

      if ($(this).data('accepts')) {
        element_settings.accepts = $(this).data('accepts');
      }
      if ($(this).data('dialog-options')) {
        element_settings.dialog = $(this).data('dialog-options');
      }
    accepts: {
      json: element_settings.accepts || 'application/vnd.drupal-ajax'
    },
  if (element_settings.dialog) {
    ajax.options.data['dialog_options'] = element_settings.dialog;
  }

can't we just have

        element_settings.accepts = $(this).data('accepts');
        element_settings.dialog = $(this).data('dialog-options');

In the first piece of code?

Also ajax.options.data['dialog_options'] = element_settings.dialog; ought to be dialogOptions if we follow convention. In this case it should be
ajax.options.data.dialog_options = element_settings.dialog; at least, no need for the brackets.

larowlan’s picture

This fixes issues identified by nod_

Status: Needs review » Needs work

The last submitted patch, dialog-route-1944472.54.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
30.69 KB
798 bytes

Missed tests for dialog options.

Status: Needs review » Needs work
Issue tags: -JavaScript, -WSCCI-conversion

The last submitted patch, dialog-route-1944472.56.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +WSCCI-conversion

#56: dialog-route-1944472.56.patch queued for re-testing.

larowlan’s picture

What else needs to be done here?
All issues from reviews addressed.

nod_’s picture

All good for me. The patch makes Crell "warm and fuzzy inside" too :þ

I'll let the RTBC to someone who knows about the whole routing thing though.

effulgentsia’s picture

FileSize
41.38 KB

Here's a screenshot of the config diff modal:
config-diff.png

+++ b/core/modules/config/config.admin.inc
@@ -77,6 +77,10 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa
+            'width' => 500

Did you intentionally reduce this to 500 from 700?

+++ b/core/lib/Drupal/Core/Ajax/DialogController.php
@@ -0,0 +1,134 @@
+      $output = $subrequest->getContent();
+      // A page callback could return a render array or a string.
+      if (is_array($output)) {

This turns out to never be true, because the subrequest's ViewSubscriber::onView() calls $event->setResponse(new Response(drupal_render($page_result))); and should because the signature of Response is that $content must be a string. As a result, the screenshot above shows 2 titles rather than the desired one as the dialog title. Any ideas on how to quick fix the title problem while deferring (and adding a @todo link to) a complete solution to #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]?

tim.plunkett’s picture

Status: Needs review » Needs work

Code looks good, needs some cleanup though

+++ b/core/lib/Drupal/Core/Ajax/DialogController.phpundefined
@@ -0,0 +1,134 @@
+   * Utility to get forward request to a subrequest.
...
+   * Wrapper to display content in a modal dialog.
...
+   * Wrapper to display content in a dialog.

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -0,0 +1,102 @@
+   * Inject the target and source config storage services.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -1196,6 +1196,17 @@ protected function drupalGet($path, array $options = array(), array $headers = a
+   * Retrieve a Drupal path or an absolute path and JSON decode the result.

+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.moduleundefined
@@ -186,26 +205,47 @@ function ajax_test_dialog_form_submit($form, &$form_state) {
+ * Utlity function: Returns the contents for dialog tests.

First word should end in 's'

+++ b/core/lib/Drupal/Core/Ajax/DialogController.phpundefined
@@ -0,0 +1,134 @@
+   * @param Request $request
+   *   The request object.
...
+   * @param Request $request
+   *   The request object.
...
+   * @param Request $request
+   *   The request object.

@param \Symfony\Component\HttpFoundation\Request $request

+++ b/core/lib/Drupal/Core/Ajax/DialogController.phpundefined
@@ -0,0 +1,134 @@
+  protected function forward($request, $content) {

Typehint: Request $request

+++ b/core/lib/Drupal/Core/Ajax/DialogController.phpundefined
@@ -0,0 +1,134 @@
+        // @todo remove when http://drupal.org/node/1830588 lands and all routes
+        // converted to return render arrays.

Second line of @todo is indented with 2 more spaces

+++ b/core/lib/Drupal/Core/Ajax/DialogController.phpundefined
@@ -0,0 +1,134 @@
+      // Set modal flag and re-use the modal id.

s/id/ID

+++ b/core/lib/Drupal/Core/Routing/Enhancer/ModalEnhancer.phpundefined
@@ -0,0 +1,32 @@
+use Symfony\Component\HttpFoundation\Request;
+use Drupal\Core\ContentNegotiation;

Neither of these are used?

+++ b/core/modules/config/config.admin.incundefined
@@ -121,7 +125,7 @@ function config_admin_import_form_submit($form, &$form_state) {
-  else if (config_import()) {
+  elseif (config_import()) {

Out of scope

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -0,0 +1,102 @@
+   * Constructor.

Let's get a real line here

+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.moduleundefined
@@ -186,26 +205,47 @@ function ajax_test_dialog_form_submit($form, &$form_state) {
+/**
+ * Utlity function: Returns the contents for dialog tests.
+ */
+function ajax_test_dialog_contents() {

Utility is spelled wrong

+++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/AjaxTestController.phpundefined
@@ -0,0 +1,21 @@
+class AjaxTestController {

Missing docblock

larowlan’s picture

Title: Add generic content handler for returning dialogs and other AJAX behaviors » Add generic content handler for returning dialogs

We're only tackling dialogs here. There is another issue for converting system/ajax to a route.
Typing on my phone, sorry if terse.

larowlan’s picture

Status: Needs work » Needs review
FileSize
29.61 KB
8.2 KB

Fixes issues identified at #61 and #62

larowlan’s picture

still green, love those ones

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

larowlan’s picture

re-roll

larowlan’s picture

Issue summary: View changes

Updated issue summary.

jibran’s picture

Priority: Normal » Major

A lot of postpone issues on this normal task it should be major.

webchick’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Alex walked me through this patch line by line, looks really great! I love the cool trick with using the mime type to trigger dialog behaviour, rather than mooshing it into the route definition.

There were two classes missing docblock but I just fixed that on commit, with help from Alex:

For DialogController: "Defines a default controller for dialog requests."
For ConfigController: "Returns responses for aggregator module routes."

Please eyeball those for accuracy. Other than that, looks great!

Committed and pushed to 8.x. Thanks! :D

Also, we don't escalate things to major for blocking normals, so undoing that priority change. I think it's been established that those route conversions are not in fact blocked on this patch.

One follow-up we'll need is the ability to use this on _form as well as _content, because we'll need that to use this in Views and on confirm forms.

quicksketch’s picture

Whee, yay thanks @larowlan, @effulgentsia, et. all. Can't wait to see this in action.

jibran’s picture

Title: Add generic content handler for returning dialogs » Change notice: Add generic content handler for returning dialogs
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

http://drupal.org/node/1853388 needs update and we also have to fix #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases as well.

we don't escalate things to major for blocking normals, so undoing that priority change.

Thank you for explaining.

webchick’s picture

no probs, and thanks for catching the need for a change notice here. total brain-fart. :)

aspilicious’s picture

Hmm, when the dialog is long (needs scrolling) and the toolbar header is in horizontal mode the top of the dialog gets rendered under the toolbar.

See image: https://www.diigo.com/item/p/cdrqprqzbpdporpddzbacbasre/abb2954c5bca72db...

Is there an issue for this?

jessebeach’s picture

I can fix the placement issue quickly here. No need for a follow-up. What path should I look at?

aspilicious’s picture

I went to the config sync page. Copied a view from active storage to staging. Changed some values (5 should be enough). When viewing the diff you will get a large modal.

jessebeach’s picture

The patch in #68 does apply at all for me. Can someone try to apply it to HEAD and verify if I'm going crazy or not?

mkadin’s picture

I think #68 is already committed.

tim.plunkett’s picture

jessebeach’s picture

Status: Active » Fixed

This has been committed. Changing the status from active to fixed.

jessebeach’s picture

jibran’s picture

Status: Fixed » Active

@jessebeach this is active because it needs change notification. :)

larowlan’s picture

Status: Active » Needs review

Draft notice is here: http://drupal.org/node/1989646

Crell’s picture

Title: Change notice: Add generic content handler for returning dialogs » Add generic content handler for returning dialogs
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Tweaked the language a little. Looks good now. Yay!

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

Anonymous’s picture

Issue summary: View changes

Updated postponed issues.