Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkadin’s picture

This should mostly be really straight forward. The only thing that might take some thnking / real work is the function ajax_prepare_response() which is only called in ViewSubscriber.php.

Is this ViewSubscriber.php thing still going to be around when D8 is all done? Or is every page callback going to return a response object?

effulgentsia’s picture

Issue tags: +Stalking Crell

Is this ViewSubscriber.php thing still going to be around when D8 is all done? Or is every page callback going to return a response object?

Tagging for Crell to weigh in on that.

My current thinking is that most _content controllers (what most page callbacks will become) should return something that can be inserted into either an HTML page or an AJAX response. I don't know whether it matters architecturally or for DX whether that is a render array or a Response object. If it's a Response object, then ViewSubscriber will go away, but some other piece of code will need to get the content out of the Response and call ajax_prepare_response() (or whatever ajax_prepare_response turns into as part of this issue) on it.

Crell’s picture

ViewSubscriber in its current form I hope SCOTCH kills. We likely will still want a view subscriber of some sort, but if rejiggering that makes sense, absolutely go for it.

If a controller returns a response object, then the kernel.view event never fires in the first place.

mkadin’s picture

The way Drupal's AJAX stuff works now, is that an AJAX callback could return any number of different things (an integer HTTP response code, a render array, a set of AJAX commands, or a string of HTML). This is pretty convenient for simplifying the AJAX callbacks themselves. The Book module has a good example:

function book_some_form_somewhere($form, $form_state) {
  // ... 
  // Add a drop-down to select the destination book.
  $form['book']['bid'] = array(
    '#type' => 'select',
    '#title' => t('Book'),
   // ...
    '#ajax' => array(
      'callback' => 'book_form_update',
      'wrapper' => 'edit-book-plid-wrapper',
      'effect' => 'fade',
      'speed' => 'fast',
    ),
  );
}

function book_form_update($form, $form_state) {
  return $form['book']['plid'];
}

The book_form_update() function just returns a little render array. The alternative would be to force developers to put a bit of extra time in:

function  book_form_update($form, $form_state) {
  $response = new AjaxResponse();
  $response->addCommand(new ReplaceCommand(NULL, render($form['book']['plid']);
  return $response;
}

I think the former is a better DX for sure...so if we want to stick with it, ViewSubscriber::onAjax() needs to stay or we need some other cool symfony way (of which I know very little) of dealing with HTML or render arrays that get returned.

effulgentsia’s picture

There's two slightly different things here I'd like to clarify. One is Ajax responses in general. The other is the role of the #ajax['callback'] function.

For the first one, a really important concept to understand is that *any* Drupal 'page callback' result can be delivered as an Ajax response. With #1667742: Add abstracted dialog to core (resolves accessibility bug) recently committed, this is actually really easy to see. Add '#ajax' => array('dialog' => array('modal' => TRUE)) to any element of '#type' => 'link', and poof, no matter what the 'href' of that link, the result will be sent back as an Ajax response.

In the old routing system, this is accomplished by the 'page callback' function returning a render array, and ViewSubscriber::onAjax() packaging that into a Response.

In the new routing system, most 'page callback' functions will be replaced by '_content' controllers. An example of one is for /user/register, defined in user.routing.yml. UserRouteController::register() currently returns a render array, but according to #3, Crell would like that changed to it returning a Response object. I don't know if we've actually agreed on that anywhere, so we might need a spin off issue to discuss it. I don't have a strong preference yet one way or the other: I'd like to see wider feedback on what people consider better DX.

However, even if we do make it return a Response object, this Response object won't be an AjaxResponse. It'll be a regular Response, because UserRouteController::register() shouldn't need to know whether someone is viewing the registration form as part of a full HTML page, or in a modal dialog. So this Response object isn't what's returned to the browser. Instead, RouteProcessorSubscriber::onRequestSetController() is responsible for setting the _controller that needs to wrap the _content. Currently, it always sets that controller to HtmlPageController, but that's wrong. It needs to content negotiate like ViewSubscriber::onView() does. If the request is an Ajax request, it needs to set the controller to AjaxController. AjaxController can be modeled on HtmlPageController: i.e., invoke _content as a subrequest, take the content out of the returned Response, and package it into a new Response. This new Response can be an AjaxResponse.

Ok, on to #ajax['callback']. Essentially the 'system/ajax' path needs to be converted to the new routing system, and ajax_form_callback() needs to be made into either a _content controller or into a _controller controller. Logically, I think it makes more sense as a _controller. But it would need to have a lot of similar code as AjaxController. Maybe it could even be another method on AjaxController or be a subclass like AjaxFormController? Its functionality is to process the form, call the triggering element's #ajax['callback'], and then turn the result of that into an AjaxResponse. I think we still have room to decide on what we therefore want the interface of #ajax['callback'] itself to be. Unless there's a compelling reason to change it from what it currently is (a function that (usually) returns a render array), let's leave it as that for now. Leaving it as that does not require us to keep ViewSubscriber, since AjaxFormController can handle the packaging to an AjaxResponse.

#ajax['callback'] could return any number of different things (an integer HTTP response code, a render array, a set of AJAX commands, or a string of HTML)

  • We should replace the integer response codes with throwing the corresponding subclass of HttpException.
  • We should replace a set of AJAX commands masquerading as a render array with an AjaxResponse object.
  • We maybe should stop supporting the string of HTML possibility, since returning array('#markup' => 'Foo') isn't that much harder than returning 'Foo'.
  • But until Crell convinces me otherwise, I think we should continue to allow returning render arrays.
mkadin’s picture

This makes a lot of sense to me, but I'm not sure I've got the Symfony chops to write this patch, but I'll try to get it started. To confirm, we're talking about the following steps:

1) Change RouteProcessorSubscriber::onRequestSetController() to route AJAX requests to AjaxController.
2) Create the AjaxController class, modeled after HtmlPageController which invokes _content as a subrequest, takes the content out of the returned Response, and package it into a new AjaxResponse.
3) Rewrite the 'system/ajax' path to use the new routing system, route to a new controller that will process the form and handle the data returned by whatever is in #ajax['callback']. If its an AjaxResponse, just return that, if its a render array, prep it and stick it in an AjaxResponse and return it.

Sound right?

I'm assuming this doesn't need to be finished in the next few days?

Crell’s picture

The irony is that I'm working on a pjax-ish Drupal 7 implementation *right now*, so I'm staring some of these issues right in the face this week. :-)

I don't think we can forbid returning render arrays, not yet. However, in *most* cases we want controllers returning response objects because that's what gets cached... as strings. Or, actually, if most content objects are actually blocks, then in whatever BlockResponse we end up using that can carry data about attached JS and CSS and meta tags and so forth.

One of the most important things that the WSCCI rewrite is doing is moving the negotiation from post-body callback to pre-body callback. In D7, the delivery callback happens after the page callback is called. Having written code a week ago to take an arbitrary page and turn it into an ajax_replace() call, it's not necessary for it to be a render array that gets returned. A string would work just as well for my use.

ViewSubscriber is, in current form, a total hack to keep the system working. It was always intended to get replaced by something more robust, hopefully SCOTCH.

The request listener that currently says "if you just specify a _content, then you get this _controller" was also a temporary measure. The assumption was that class, and HtmlPageController, would both get removed entirely by SCOTCH and replaced with something more robust; conceptually that could even happen via contrib right now, but I'd rather do it in core.

IMO, the correct solution is, as effulgentsia said and as I noted in the prior Ajax issue, to change the Ajax callbacks in JS to have a custom mimetype. Then the current default-_controller code gets enhanced (mechanism TBD, but this could even be baked right into the router, perhaps) to specify a different _controller if that mimetype is detected. That controller can call _content() the exact same way that HtmlPageController does (via a subrequest, which hopefully turns into a render(), which makes it HTTP cached), and then takes the resulting string and puts it into an AjaxReplace command object, and then puts that into an AjaxResponse object.

URIs that we know in advance will only be used for Ajax purposes can just specify that _controller in the route definition.

In either case, ViewSubscriber becomes unnecessary.

Make sense?

mkadin’s picture

This does make sense to me. So it sounds like we need to start by converting core's AJAX JS to use a custom MIME type?

The for the routing piece, I'm not sure how to proceed...is this something we need to wait on SCOTCH for?

Crell’s picture

Yes, that sounds like step 1. And no, no need to wait for SCOTCH here. We'll both be editing the same default controller-setting class (the name of which I forget off hand), but as long as this issue gets there first we should be fine. :-)

mkadin’s picture

It seemed like a simple thing to do :) Adding the contentType: 'application/vnd.drupal-ajax' parameter to the jQuery's ajax options works well. Ajax requests get sent with that mime type. It works perfectly for an ajax enabled link.

However, PHP doesn't parse the posted data from an ajax submitted form into the $_POST array because it isn't interpreting the data as urlencoded form data. If I add something like this:

parse_str(file_get_contents("php://input"), $_POST);

(from: http://stackoverflow.com/questions/8213974/empty-post-without-content-type)

It works, but I imagine we don't want to do something like that. Seems hackish to me.

There's an additional problem, which I believe is related to jquery.form.js. Forms that have files in them are submitted via a hidden iframe or some craziness like that. It doesn't seem to be picking up the mime type from the jquery options that are passed along.

There's a patch attached, its not intended to be used, but is just to illustrate the point. I've had to wrap the parse_str hack in a conditional so that it doesn't break forms with files. But again, those forms are not submitted with the new MIME type, and I couldn't figure out how to make it so.

I've also attached a sample module that can help to easily debug this stuff. There at a test form at /mymodule-test-form and a test link a /mymodule-test-link. Both will return the mime type of the request in an alert box. You can comment the file field in and out of the module to see how the mime type changes.

Given the 'forms with files' issue and the hackey parse_str thing necessary for this (is there a better workaround?)...is the mime type worth it? Or should we just stick to ajax calls to system/ajax a la D7?

Damien Tournoud’s picture

However, PHP doesn't parse the posted data from an ajax submitted form into the $_POST array because it isn't interpreting the data as urlencoded form data.

I suppose Crell meant to say that the Accept: header of the XHR call needs to be changed, not the Content-Type: one. But in any case, as you noted it is not possible to do this in the iframe fallback.

Damien Tournoud’s picture

Also to be noted: jquery.form can do direct XHR file upload on browsers which have a XHR level 2 implementation. This means not every browser, but very close.

effulgentsia’s picture

From what I can tell, D8 core is not using XHR2 for file uploads on Firefox 12. It's not even using XHR for form submissions with empty file fields. Are we not using a recent enough jquery.form, or not invoking it correctly?

effulgentsia’s picture

Or should we just stick to ajax calls to system/ajax a la D7?

That's an unrelated issue. Where we do or do not want to use system/ajax is unrelated to mime type, because even without mime type, we can detect an XHR submission as ContentNegotiation::getContentType() already does.

For IFrameUploads, not setting mime type is the HTTP proper way, because what we return in that case isn't a JSON string, but a JSON string wrapped in a <textarea>, or in other words, HTML, as per the mime type the browser requests.

I don't know offhand what a good solution to getting form decoding working with custom mime types is. I'll post back here if I think of anything, or await to see what others come up with.

Crell’s picture

Erf. Yes, I meant set the Accept header, not Content-Type.

mkadin’s picture

Status: Active » Needs review
FileSize
65.66 KB

Ok here's a little something to get started with. In this patch:

1) An update to core's copy of the jquery.form.js plugin. I couldn't get the accept header to work on forms with file fields without it. This needs to be tested across browsers.

2) A tiny change to ajax.js to set the accept header to include 'application/vnd.drupal-ajax'

3) An additional conditional in RouteProcessorSubscriber::onRequestSetController() that checks the accept header for the presence of 'application/vnd.drupal-ajax' and sets the controller to the new AjaxController (see next item). It was discussed in earlier comments that this should be done differently (perhaps baked into the router), but I leave that to someone else who knows a bit more symfony.

4) A new AjaxController which extends HtmlPageController. The only overridden method is the content method, which is a combination of HtmlPageConroller's content method and ajax_prepare_response() from D7's ajax API. This method should probably have the most eyes on it.

Everything seems to work well as far as I can tell.

One issue: The new dialog stuff introduced a complication that doesn't really fit in with our new AJAX API.
From ajax_prepare_response():

// Since this is the primary response content returned to the client, we
// also attach the page title. It is up to client code to determine if and
// how to display that. For example, if the requesting element is configured
// to display the response in a dialog (via #ajax['dialog']), it can use
// this for the dialog title.
$html = is_string($page_callback_result) ? $page_callback_result : drupal_render($page_callback_result);
$commands[] = ajax_command_insert(NULL, $html) + array('title' => drupal_get_title());
// Add the status messages inside the new content's wrapper element, so that
// on subsequent Ajax requests, it is treated as old content.

We can't just insert additional pieces to the insert command's array anymore. That doesn't fit in with the new API. Do we need to include a new title command? A parameter on every insert comment? To me, shouldnt this just be a SettingsCommand?

Damien Tournoud’s picture

In #12, I meant to say that we cannot rely on XHR2, as it has only fairly recently be implemented in one of the major browsers.

Crell’s picture

I'd say setting the page title should be its own discrete command.

+++ b/core/lib/Drupal/Core/AjaxController.php
@@ -0,0 +1,80 @@
+class AjaxController extends HtmlPageController {

Why does this need to extent HtmlPageController? It's overriding the only useful method already, isn't it?

+++ b/core/lib/Drupal/Core/AjaxController.php
@@ -0,0 +1,80 @@
+        // Like normal page callbacks, simple Ajax callbacks can return HTML
+        // content, as a string or render array. This HTML is inserted in some
+        // relationship to #ajax['wrapper'], as determined by which jQuery DOM
+        // manipulation method is used. The method used is specified by
+        // #ajax['method']. The default method is 'replaceWith', which
+        // completely replaces the old wrapper element and its content with the
+        // new HTML. Since this is the primary response content returned to the
+        // client, we also attach the page title. It is up to client code to
+        // determine if and how to display that. For example, if the requesting
+        // element is configured to display the response in a dialog (via
+        // #ajax['dialog']), it can use this for the dialog title.
+        $html = is_string($content) ? $content : drupal_render($content);
+        $response = new AjaxResponse();
+        $response->addCommand(new InsertCommand(NULL, $html));
+        $response->addCommand(new PrependCommand(NULL, theme('status_messages')));

This is an awesome level of documentation. :-) I don't see where the title is being added, though. The comment says it, but the code disagrees.

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteProcessorSubscriber.php
@@ -32,7 +32,12 @@ public function onRequestSetController(GetResponseEvent $event) {
     if (!$request->attributes->has('_controller') && $request->attributes->has('_content')) {
-      $request->attributes->set('_controller', '\Drupal\Core\HtmlPageController::content');
+      if ($request->headers->has('accept') && strpos($request->headers->get('accept'), 'application/vnd.drupal-ajax') !== FALSE) {
+        $request->attributes->set('_controller', '\Drupal\Core\AjaxController::content');
+      }
+      else {
+        $request->attributes->set('_controller', '\Drupal\Core\HtmlPageController::content');
+      }
     }

This is probably OK for now, but once the content negotiation stuff goes in we should have an attribute that we can rely on to determine the format to use. This also needs to get generalized, but I'm not sure yet how.

I can't really speak to the JS parts here.

effulgentsia’s picture

Do we need to include a new title command? A parameter on every insert comment?...I'd say setting the page title should be its own discrete command.

Related: #1851414: Convert Views to use the abstracted dialog modal. So I agree that simply adding 'title' to the insert command the way HEAD now does was just a hack to unblock the modal dialog issue, and that we need a better way. Per #18.2, the thinking I had when adding this was that in many modal dialog situations, the code that is processing the AJAX request shouldn't know or care whether the result will be shown in a dialog or in some other kind of DOM element. As far as the server is concerned, it just needs to return some content, and the client-side code should deal with deciding how to display it. So I've been opposed to 'openModal' / 'closeModal' commands in the general case, though per #1851414-6: Convert Views to use the abstracted dialog modal, I'm not necessarily opposed to such commands for Views or other places where it makes sense for the PHP code handling the AJAX response to control that behavior.

If we want to at least allow some AJAX workflows where the PHP code processing the AJAX response can return content without concerning itself about whether it's in a dialog or not, then we need some generic command that JS code can use for determining how to initialize the #ajax['wrapper'] element that triggered the AJAX request. Moving this from "the Insert command that magically contains a title property" to "a Title command" seems like a fine step. Maybe there's something even better that we can come up with either in this issue or in a follow up (e.g., a "initializeWrapper" command or something like that).

Crell’s picture

Related thoughts:

1) Yes, we do want any page to be Ajaxable. In fact, any block should be ajaxable, automagically, by design.

2) Perhaps instead of a "replace this wrapper element with this new wrapper and this body", we should just have a "replace the contents of this wrapper element with this body" command? (innerHtml, basically). Would that simplify matters at all? (I would certainly find the latter far more useful.)

3) While a SetPageTitleCommand sounds like a good idea in general (really just a subclass of insert?), do we also want to allow a title to piggyback on some other command? Would that make it easier for a response to be flexible about where it's going to go, but note that it needs a title associated with it? (We've talked about block responses needing that capability anyway to get rid of the global drupal_set_title().)

effulgentsia’s picture

Re #20.2, it would simplify some things, but not anything related to this issue. I opened a new issue for discussing that: #1858562: Change default Ajax insertion method from replaceWith to html.

mkadin’s picture

Re @Crell:

Why does this need to extent HtmlPageController? It's overriding the only useful method already, isn't it?

True. This could easily be re-implemented as its own class if that's a better choice.

This is an awesome level of documentation. :-) I don't see where the title is being added, though. The comment says it, but the code disagrees.

Well I'll take the thanks, but I didn't write it. Its a copy and paste from the ajax_prepare_response() docs. I should have taken it out, but I didn't want it to be lost that this code broke the dialog stuff.

I don't think a set title command is really possible - can't individual themes set the id / class of the page title as they see fit? Thus, I don't think we could make a generic solution.

Can't we just send the title along as a JS setting when needed? I don't see it as such a big deal. @effulgentsia, can you elaborate on what you mean by "Maybe there's something even better that we can come up with either in this issue or in a follow up (e.g., a "initializeWrapper" command or something like that)." I'm not really following what you'd like to see happen.

mkadin’s picture

I'd like to continue to move this forward. Here's a patch that incorporates Crell's feedback from #18.

1) I've split off the AJaxController class off from the HtmlPageController, by doing a little more copying and pasting from HtmlPageController.

2) The comments for the modal dialog title stuff are still there...but a solution for the modal title is not. Does anyone see an easy solution?

dawehner’s picture

Issue tags: +WSCCI
FileSize
65.6 KB

From an issue standpoint the jquery form.js should have been maybe put in it's own issue.
Btw. is there something like composer but for javascript we want to use?

Rerolled against HEAD and fixed some minor problems just to get the flow going on.

dawehner’s picture

FileSize
2.27 KB

Forgot the interdiff.

Status: Needs review » Needs work

The last submitted patch, drupal-1843220-24.patch, failed testing.

dawehner’s picture

FileSize
65.67 KB

Core is moving fast.

dawehner’s picture

Status: Needs work » Needs review

.

podarok’s picture

Status: Needs work » Needs review
+++ b/core/misc/ajax.jsundefined
@@ -208,6 +208,9 @@ Drupal.ajax = function (base, element, element_settings) {
diff --git a/core/misc/jquery.form.js b/core/misc/jquery.form.js

+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+    });
+    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+    });
+    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+    }
+    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+    }
+    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+        q = ( q ? (q + '&' + qx) : qx );
+    }    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+    var fileInputs = $('input[type=file]:enabled[value!=""]', this); ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+        });
+        ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+        }
+        ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+        }
+        ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+        }
+        ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+            }
+            ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -1,467 +1,606 @@
+                io.detachEvent('onload', cb);
+            else    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -573,60 +724,83 @@ $.fn.ajaxSubmit = function(options) {
+    options.delegation = options.delegation && $.isFunction($.fn.on);
+    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -573,60 +724,83 @@ $.fn.ajaxSubmit = function(options) {
+// private event handlers    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -573,60 +724,83 @@ $.fn.ajaxSubmit = function(options) {
+}
+    ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -640,56 +814,74 @@ $.fn.ajaxFormUnbind = function() {
+        if (v && v.constructor == Array) {
+            if (elements) ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -640,56 +814,74 @@ $.fn.ajaxFormUnbind = function() {
+        else if (feature.fileapi && el.type == 'file' && !el.disabled) {
+            if (elements) ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -640,56 +814,74 @@ $.fn.ajaxFormUnbind = function() {
+        else if (v !== null && typeof v != 'undefined') {
+            if (elements) ¶
+++ b/core/misc/jquery.form.jsundefined
@@ -905,15 +1109,15 @@ $.fn.ajaxSubmit.debug = false;
-};
+    if (!$.fn.ajaxSubmit.debug) ¶

a lot of trailing whitespace cleanup needed

This patch has a huge amount of logic, hard to review, possibly needs manual testing with different input parameters

podarok’s picture

Status: Needs review » Needs work

status

mkadin’s picture

Do we care about / do we change the formatting of third party libraries? I'm guessing no...

podarok’s picture

Status: Needs review » Needs work

#31 I`m talking not about third party, but about patch additions

mkadin’s picture

The extra logic and white space that you're referring to is all in jquery.form.js, which is a third party JS library that is being updated in this patch.

It was mentioned in #24 that this should have probably been its own patch

podarok’s picture

Status: Needs work » Needs review

#33 woops
ok
looks like this still need review without #29 comment

Wim Leers’s picture

Title: Convert form Ajax to new Ajax API » Convert form AJAX to new AJAX API
Status: Needs review » Needs work
Issue tags: +sprint, +Spark

NOTE: #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended is kind of blocked on this issue: without this issue, the tests are testing the old AJAX rendering pipeline instead of the new one (which was introduced at #1812866: Rebuild the server side AJAX API). This in turn has allowed for a regression at #1875632-31: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings), which is causing problems at #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms.

Review

  • Why isn't ajax.inc/ajax_render() being removed yet?
  • The current version of jquery.form.js is 3.27, not 3.21 (i.e. it's been updated in the mean time).
  • system_library_info() was not updated to indicate the new jquery.form.js version
  • AjaxController.php is the single most important change to review; I'm not in a good position to review it because I don't know the entire request/response architecture well enough. Despite that, I tried to give as useful feedback as possible.
+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+ * Default controller for Ajax Requests.

s/Ajax/AJAX/

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+   *   The callable that returns the content of the ajax response.

s/ajax/AJAX/

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+    // We need to clean off the derived information and such so that the

s/clean off/clean up/

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+      //manipulation.

Missing leading space.

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+        // Like normal page callbacks, simple Ajax callbacks can return HTML

s/Ajax/AJAX/

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+        // content, as a string or render array. This HTML is inserted in some
+        // relationship to #ajax['wrapper'], as determined by which jQuery DOM

"in some relationship to" sounds very strange.

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+        // #ajax['method']. The default method is 'replaceWith', which
+        // completely replaces the old wrapper element and its content with the
+        // new HTML. Since this is the primary response content returned to the
+        // client, we also attach the page title. It is up to client code to
+        // determine if and how to display that. For example, if the requesting
+        // element is configured to display the response in a dialog (via
+        // #ajax['dialog']), it can use this for the dialog title.

Is this entire massive comment really necessary?

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,78 @@
+        $response->addCommand(new PrependCommand(NULL, theme('status_messages')));

Shouldn't this check whether theme('status_messages') returns a non-empty string?

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteProcessorSubscriber.phpundefined
@@ -38,7 +38,10 @@ public function __construct(ContentNegotiation $negotiation) {
-    if (!$request->attributes->has('_controller') && $this->negotiation->getContentType($request) === 'html') {
+    if ($request->headers->has('accept') && strpos($request->headers->get('accept'), 'application/vnd.drupal-ajax') !== FALSE) {

Looks sensible, except that this is not checking if the request has a "_controller" attribute. Is that intentional? And if so, then why must this be checked for the next if-test?

sun’s picture

Please note that we are using "Ajax" in all strings, comments, and documentation in Drupal core. This is documented in the Drupal core string terminology guidelines (somewhere in the d.o handbooks). Therefore, these parts of #35 should be ignored.

dawehner’s picture

Let's have separated issues for the ajax controller and the jquery forms update: #1843220: Convert form AJAX to new AJAX API

Why isn't ajax.inc/ajax_render() being removed yet?

This is the central question before we should forward. So we have basically two use-cases, the automatic way by #ajax and the manual ajax callbacks, that support custom commands etc. This patch seems to be used just for the first one, while the custom ajax callbacks need for example ajax_prepare_response.

mkadin’s picture

Yes this patch is just for the automatic Form API #ajax property stuff.

#1812866: Rebuild the server side AJAX API covered creating a new API for ajax commands. Once this is in, I was thinking a big 'Convert All Ajax to the new API' issue would be next, which would remove ajax.inc (and hence ajax_render()) all at once.

If we want to split off the new jquery.forms.js plugin into a separate issue, that'll have to happen first as this will be dependent on it.

dawehner’s picture

Wim Leers’s picture

#36: huh? Really? That doesn't seem to make much sense? :( Can you link to those docs?

mkadin’s picture

The new Ajax API functions / classes (I.e. AjaxResponse) also use Ajax, not AJAX. Just a note in case we end up changing.

mkadin’s picture

Assigned: Unassigned » mkadin

I'm going to hop on this today.

Wim Leers’s picture

#41: It *can* make sense in function names et al. (cfr. #1627350: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName)). I just don't see how it makes sense in comments. It'd be like writing "Html" in all comments, or "Css". We don't do that either. Hence my astonishment.

tstoeckler’s picture

Re #43: Ajax is "officially" not an acronym (anymore). That still trips me up, too, but that's how it has been decided (don't know by whom).

mkadin’s picture

Want to get this through, but its been a while since I've done any D8 stuff. Applying the most recent patch gives the following error on any Ajax (or is it AJAX ;) operation:

RuntimeException: Controller "Drupal\Core\AjaxController::content()" requires that you provide a value for the "$_content" argument (because there is no default value or because there is a non optional argument after this one). in Symfony\Component\HttpKernel\Controller\ControllerResolver->doGetArguments() (line 134 of /var/www/drupal8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Controller/ControllerResolver.php)

Tried to dig and figure this out but to no avail. Any WSCCI folks know what's up?

Crell’s picture

That error means that the controller has a $_content parameter, but the $request->attributes bag doesn't have a _content key. That usually means an issue with the route definition.

On what URL did you get that? As written, the AjaxController will only be able to handle fronting for routes that have _content callback defined, which is apparently not all of them. I suspect that code needs to be made more flexible. Without knowing which URL that happened on I couldn't tell you if that's a problem with new routes or old routes.

mkadin’s picture

It was doing this on core Ajax form elements which send requests to 'system/ajax'

mkadin’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

The page callback ajax_form_callback comes in the _callback parameter which was being overwritten. Now it gets stuck in _content before shipped off to AjaxController. I have no idea if that's the best way to do that, but it works.

I've left the capitalization of AJAX as is, but incorporated the rest of wim's feedback.

My biggest concern is what's going on to check for the accept header in RouteProcessorSubscriber::onRequestSetController(), at this point, is there no better way to route on a header?

Wim Leers’s picture

An interdiff would've been nice :)


I'm wondering if this:

  $items['system/ajax'] = array(
    …
    'theme callback' => 'ajax_base_page_theme',
    …
  );

should be converted to the routing system as part of this patch.

Because, modules that do use the routing system (as they should in D8), like the edit and editor modules, are stuck with a nasty work-around as an alternative to the theme callback in the menu item:

/**
 * Implements hook_custom_theme().
 *
 * @todo Add an event subscriber to the Ajax system to automatically set the
 *   base page theme for all Ajax requests, and then remove this one off.
 */
function editor_custom_theme() {
  if (substr(current_path(), 0, 7) === 'editor/') {
    return ajax_base_page_theme();
  }
}

It seems like it might be out of scope, but in that case, a follow-up issue should be created for it.


+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,71 @@
+        $response->addCommand(new InsertCommand(NULL, $html));

We should document why the selector is NULL.

+++ b/core/lib/Drupal/Core/AjaxController.phpundefined
@@ -0,0 +1,71 @@
+        $status_messages = theme('status_messages');
+        if (!empty($status_messages)) {
+          $response->addCommand(new PrependCommand(NULL, theme('status_messages')));

Rendering status messages twice.

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteProcessorSubscriber.phpundefined
@@ -38,7 +38,11 @@ public function __construct(ContentNegotiation $negotiation) {
-    if (!$request->attributes->has('_controller') && $this->negotiation->getContentType($request) === 'html') {
+    if ($request->headers->has('accept') && strpos($request->headers->get('accept'), 'application/vnd.drupal-ajax') !== FALSE) {

Unchanged from #35, last remark.

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteProcessorSubscriber.phpundefined
@@ -38,7 +38,11 @@ public function __construct(ContentNegotiation $negotiation) {
+      $request->attributes->set('_content', $request->attributes->get('_controller'));
+      $request->attributes->set('_controller', '\Drupal\Core\AjaxController::content');

This looks extremely weird.

Wim Leers’s picture

Status: Needs review » Needs work
mkadin’s picture

FileSize
4.74 KB

I do think the ajax_base_page_theme belongs in a separate issue because it applies to non-form AJAX requests as well.

As for Wim's last two points in 49, someone with a better understanding of all things WSCCI will have to guide me, because I'm sure the stuff in RouteProcessorSubscriber::onRequestSetController() needs work.

Also, after further testing, this isn't exactly working. Wrappers in the form are being replaced by a full drupal page, html tags and all, which contains the replacement content. Looks like the subrequest is wrapping the new content in an entire page. How can this be avoided?

mkadin’s picture

FileSize
2.71 KB

Interdiff

Wim Leers’s picture

#51:
- Ok, could you create a new issue for that then? :)
- That's scary, that means the current test coverage is insufficient! How could it possibly that the AJAX tests pass if it's not working at all? :/

mkadin’s picture

Status: Needs work » Needs review

RE:#53, Triggering the testbot for #51. I wasn't getting that behavior yesterday, so maybe something has changed.

The html that replaces the wrapper contains all the right stuff, but just has extra, so maybe thats why the tests are happy.

mkadin’s picture

FileSize
4.77 KB

Strips off the '_legacy' attribute from the subrequest, and solves the 'whole page insert' problem, which was caused by ViewSubscriber::onView()

if ($request->attributes->get('_legacy')) {
      // This is an old hook_menu-based subrequest, which means we assume
      // the body is supposed to be the complete page.
      $page_result = $event->getControllerResult();
      if (!is_array($page_result)) {
        $page_result = array(
          '#markup' => $page_result,
        );
      }
      $event->setResponse(new Response(drupal_render_page($page_result)));
    }
Wim Leers’s picture

Interdiff please? :)

mkadin’s picture

FileSize
583 bytes

Funny how I always make it and then forget to post it.

Wim Leers’s picture

I can't really sign off on the WSCCI parts of this patch. But testing it manually causes no problems.

As per #37/#38, the patch of #55 covers the full intended scope of this issue, and a follow-up issue will be created where "the big conversion" and the removal of ajax.inc will have to happen.

So, I think that the last thing that needs to happen before #55 can become RTBC is sign-off from a WSCCI person. Pinging Crell on Twitter.

mkadin’s picture

FileSize
4.77 KB
1000 bytes

A little more symfony-y way of checking for the accept header.

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteProcessorSubscriber.php
@@ -38,7 +38,11 @@ public function __construct(ContentNegotiation $negotiation) {
-    if (!$request->attributes->has('_controller') && $this->negotiation->getContentType($request) === 'html') {
+    if ($request->headers->has('accept') && strpos($request->headers->get('accept'), 'application/vnd.drupal-ajax') !== FALSE && !$request->attributes->has('_content')) {
+      $request->attributes->set('_content', $request->attributes->get('_controller'));
+      $request->attributes->set('_controller', '\Drupal\Core\AjaxController::content');
+    }
+    elseif (!$request->attributes->has('_controller') && $this->negotiation->getContentType($request) === 'html') {

For the rest and serialization modules, we've been adding new mime types to the request's list of known types on the fly, and then checking against that. See the "HalSubscriber" class in the patch at #1924220-14: Support serialization in hal+json

We should probably do the same for consistency.

Also, this block is guaranteed to conflict with #1934832: Provide a dedicated approach for using forms in routes, which is mucking with the same code. :-( (We're trying to land that by Sprint Weekend.) So this will need to get rerolled once that's in. A follow up on my agenda is to refactor this subscriber to be just a series of RouteEnhancers, which will largely eliminate this sort of conflict.

Other than that, this looks awesome. :-) Nice work, guys!

Wim Leers’s picture

Status: Needs review » Needs work

As per #60.

mkadin’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
7.57 KB

Hows this? I used the hal module as a model.

Status: Needs review » Needs work

The last submitted patch, drupal-1843220-62.patch, failed testing.

mkadin’s picture

Status: Needs work » Needs review
FileSize
7.72 KB

Rebase! interdiff from 62 applies.

Status: Needs review » Needs work

The last submitted patch, drupal-1843220-64.patch, failed testing.

Wim Leers’s picture

Yay, proper test failures! If you want to test locally while attempting to fix test failures, I recommend the CKEditor tests — its whole suite runs in <1 minute.

mkadin’s picture

Status: Needs work » Needs review
FileSize
8.35 KB

This should pass, if HEAD has been fixed ;)

There was a little name conflict for the 'ajax' content type that existed before for all XHR's and the new 'ajax' content type I had created for Drupal form ajax requests. This changes the form ajax requests to 'drupal_ajax' which we can keep, or refactor back to ajax in a follow up issue for this where we remove all the old ajax stuff.

mkadin’s picture

FileSize
2.47 KB

Interdiff

Crell’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. We can tidy up that subscriber after this is in. If the bot approves of the latest patch, so do I.

mkadin’s picture

Issue tags: +SprintWeekend2013

tagging

Wim Leers’s picture

Hurray :) And yes, HEAD has been unbroken :)

http://drupalcode.org/project/drupal.git/commit/2af2c93

webchick’s picture

Title: Convert form AJAX to new AJAX API » Change notice: Convert form AJAX to new AJAX API
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

I winced when clicking into this issue, but actually this patch is not bad at all. It's also nice because it starts to show off some of the power we get from all this work around the routing system/Symfony.

Committed and pushed to 8.x. Thanks!

I believe this needs a change notice.

mkadin’s picture

Does this need a change notice? Using the #ajax property of a form element should be exactly the same.

jessehs’s picture

This commit seems to have broken the diff dialog added by this commit: #1821548: Add a "diff" of some kind to the CMI UI

A form was triggering a dialog via the following lines:

    foreach ($config_files as $config_file) {
      $links['view_diff'] = array(
        'title' => t('View differences'),
        'href' => 'admin/config/development/sync/diff/' . $config_file,
        'ajax' => array('dialog' => array('modal' =>TRUE, 'width' => '700px')),
      );
      $form[$config_change_type]['list']['#rows'][] = array(
        'name' => $config_file,
        'operations' => array(
          'data' => array(
            '#type' => 'operations',
            '#links' => $links,
          ),
        ),
      );
    }

After this commit, however, the dialog does not appear.

I'm not experienced enough to know the correct fix, and nothing jumped out at me looking through the D8 form api documentation.

mkadin’s picture

Grrrr....I even noticed that this was going to break the dialog stuff in #16...I just never fixed it. That's what I get for finishing an issue months after I start it.

The inserting of the page title into the insert command was sort of hackish though...we need to find a cleaner solution.

Does this mean we need some more dialog tests as well?

mkadin’s picture

Status: Active » Needs review
FileSize
2.39 KB

This ought to fix the modal dialog problem in a quick and dirty way...though I contend...this is no dirtier than the original methodology for getting the title to the modal dialog.

I've created a follow up issue to remove this hacky-ness and come up with a better solution: #1939758: Figure out a better way to get the page title to a modal dialog

mkadin’s picture

Issue tags: +Needs change record

OK, that's green, let's get it in quickly to fix my mistake.

Also, change notice: http://drupal.org/node/1939862

mkadin’s picture

Issue tags: -Needs change record

Like me holding a beer in a facebook picture, you know I be de-tagging.

michaelfavia’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Confirmed proper operation with CMI xml modal as well. RTBC as easy as ABC's.

catch’s picture

Title: Change notice: Convert form AJAX to new AJAX API » Convert form AJAX to new AJAX API
Status: Reviewed & tested by the community » Fixed

Committed/pushed the follow-up. #1939758: Figure out a better way to get the page title to a modal dialog is fine for me - the whole page title handling needs to change dramatically.

Since there's a change notice moving this to fixed.

Wim Leers’s picture

Issue tags: -sprint

.

swentel’s picture

Status: Fixed » Reviewed & tested by the community

Actually, this hasn't been pushed at all, the dialog on the configuration is still broken, Patch still applies and makes it work, so setting to RTBC once more.

swentel’s picture

Status: Reviewed & tested by the community » Fixed

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