Remove the deprecated Drupal 7 Ajax API

The same happens in #1957590: Remove remaining procedural ajax command usages.

#1957590: Remove remaining procedural ajax command usages. landed, so unpostponing this.

Thanks. These are the relevant issues related to those failures: #1967036: WebTestBase::drupalGetAJAX() and WebTestBase::drupalPostAJAX() don't set the Accept header needed for content negotiation and #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice.

This could use a reroll for #1967036: WebTestBase::drupalGetAJAX() and WebTestBase::drupalPostAJAX() don't set the Accept header needed for content negotiation and other HEAD changes. Good news is, since that issue landed, once rerolled, it should pass.

4 files changed, 24 insertions, 430 deletions.

Pretty. :-)

Just noting that in core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.php ajax_render still exists.

Actually... Shouldn't this also fix AjaxEnhancer, which was doing things the old backward way? (Bad Crell for RTBCing without thinking about that.)

Indeed, I'm afraid this is not even close to RTBC…

In ViewSubscriber, the old AJAX system stuff is being called in two methods (not one as you would expect):

1. onAjax (as expected; should be safely deletable)
2. onIframeUpload (rather unexpected, to me at least)

The latter could turn out to be kinda tricky. I'd love to remove it, but that might be impossible, it's necessary for #1009382: Several popular browser extensions corrupt AJAX responses, causing errors (for dealing with crappy browser add-ons/extensions, which I think we should not do at all) and http://malsup.com/jquery/form/#file-upload (for dealing with browsers that don't support XHR level 2). The latter is less relevant now, though it still appears to be necessary for IE 9 and Opera Mini: http://caniuse.com/xhr2.

Figuring out how to deal with that seems to be rather tricky and will require manual testing in many browsers. It must be done as part of this issue too — even though it is unrelated at the surface, because it currently depends on the D7 AJAX API, which is precisely what we want to remove in this issue.

Finally: it'd be nice if you'd like to this rather critical follow-up issue from the main issue next time — I did that just now: #1812866-88: Rebuild the server side AJAX API. How else are followers of that issue supposed to find this one?

The current issue summary is not linking to the original issue, nor explaining why this was not done in the original issue (i.e. converting existing calling code to the new D8 Ajax API), nor explaining tricky edge cases like the one I laid out. An updated issue summary would be very helpful for other reviewers.

Updated issue summary.

#22: No problem! :) Hope I didn't sound too harsh.

Thanks for the issue summary update! Are the open questions in the issue summary really still open questions though?

And indeed, the AjaxEnhancer stuff in #19 needs input/guidance from people familiar with those matters, that's relatively deep stuff that only few people are familiar with.

.

mkadin, are you still interested in this? We really do need to finish it. :-)

And I found yet another major bug with the D8 Ajax API: #2063303: A Drupal 8 AjaxResponse that must return a 403, returns a 403 and prints "A fatal error occurred: ".

Seems appropriate, considering #1812866: Rebuild the server side AJAX API was also tagged WSCCI.

#29 implies this.

This is just a straight reroll of #16, which bitrotted.

I'm going to see how much farther I can take this.

Just setting to needs review to have tests run. I expect them to fail.

I've uncommented this test in the latest patch. It would seem that this test must pass in order for this issue to be considered resolved.

/**
 * Checks that an ajax request gets rendered as an Ajax response, by mime.
 *
 * @todo This test will not work until the Ajax enhancer is corrected. However,
 *   that is dependent on fixes to the Ajax system. Re-enable this test once
 *   http://drupal.org/node/1938980 is fixed.
 */
public function testControllerResolutionAjax() {
  // This will fail with a JSON parse error if the request is not routed to
  // The correct controller.
  $this->drupalGetAJAX('/router_test/test10');

  $this->assertEqual($this->drupalGetHeader('Content-Type'), 'application/json', 'Correct mime content type was returned');

  $this->assertRaw('abcde', 'Correct body was found.');
}

I am not the right person to look into this issue it turns out. This is too far down into kernel for me.

32: remove-d7-ajax-api-1959574-32.patch queued for re-testing.

To be honest this feels like an issue which should be fixed after the htmlpage issue.

I think you're right. They'd likely conflict and just make rolling harder.

#2068471: Normalize Controller/View-listener behavior with a Page object is in, unpostponing.

Straight reroll of #32. Let's see what fails.

In order to use the logic of AjaxController in the view subscriber as well I introduced the ajax response renderer, similar to the HtmlPageRenderer
and in the future the HtmlFragmentRenderer.

@wim
I started independent from your rerole, though most of the code should be identical.

#40: you know this area of Drupal so much better, so *thank you*! :)

I double-checked, and #40 indeed is a superset of #39. So ignore #39, #40 is the best one!

1. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +use Drupal\Core\Page\HtmlFragment;
   ...
   +namespace Drupal\Core\Ajax;
   

   Missing newline.

2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +   *   The result of a controller, so for example a string or a render array.
   

   Unneeded 'so.' What about HtmlFragment, Response, and AjaxResponse?

3. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +    if ($content instanceof HtmlFragment) {
   +      $content = $content->getContent();
   +    }
   +    if ($content instanceof Response) {
   +      $content = $content->getContent();
   +    }
   

   if/elseif ?

4. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +      // Complex Ajax callbacks can return a result that contains an error message
   

   80 char limit.

5. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +          $error = t('An error occurred while handling the request: The server received invalid input.');
   

   Inject the translation service?

1. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   + * Converts a controller result into an ajax response object.
   ...
   +   * Renders the controller result as ajax response object.
   

   ajax -> Ajax
   Also I think the second one would be much clearer if it were identical to the first one. "Render" is not exactly clear and "an" is missing.

2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +    if (!is_array($content)) {
   +      $content = array(
   +        '#markup' => $content,
   +      );
   +    }
   ...
   +    if (is_array($content) && isset($content['#type']) && ($content['#type'] == 'ajax')) {
   

   The is_array() can be saved in the second one, it will never be FALSE.

3. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +} ¶
   

   Trailing whitespace.

4. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,76 @@
   +    if (!empty($output)) {
   +      $response->addCommand(new PrependCommand(NULL, $output));
   +    }
   

   Previously this was added unconditionally. Is there a specific reason for that?

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/AjaxTestBase.php
   @@ -24,22 +24,22 @@
   +   * An AjaxResponse object stores an array of Ajax commands This array
   

   Missing period after "commands".

6. +++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.php
   @@ -27,7 +27,7 @@ public static function getInfo() {
   +   * Ensures AjaxController adds JavaScript settings from the page request.
   ...
   -   * Ensures ajax_render() returns JavaScript settings from the page request.
   

   In case it still fits in the 80 chars (I think it does) add a "that" after "Ensures".

7. +++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.php
   @@ -101,7 +101,7 @@ public function testOrder() {
   +   * Tests behavior of an error alert command.
   ...
   -   * Tests behavior of ajax_render_error().
   

   Since we're already changing this, maybe "Tests *the* behavior"

8. +++ b/core/modules/system/system.api.php
   @@ -399,17 +399,19 @@ function hook_css_alter(&$css) {
   + * framework.
   ...
   + * Alter the Ajax command data that are sent to the client through the Ajax
   

   are -> is
   Also: Dropping the "through the Ajax framework" would make it < 80 chars.

Wow, two reviews in a short time, thank you!

Previously this was added unconditionally. Is there a specific reason for that?

Well, this code was moved from AjaxController to AjaxResponseRenderer, which is the more modern code compared to ajax_prepare_...

In case it still fits in the 80 chars (I think it does) add a "that" after "Ensures".

It fits exactly.

Since we're already changing this, maybe "Tests *the* behavior"

Sure.

Also: Dropping the "through the Ajax framework" would make it < 80 chars.

In general I wonder whether we should add an interface for the AjaxResponseRenderer?
Additional this replaces all references of ajax_render() in core.

January 4, 2014 at 4:11am

No comment.

Awesome thanks! Looks great to me, but I don't know the Ajax system or the new page rendering system well enough to RTBC.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ViewSubscriber.php
   @@ -43,16 +44,26 @@ class ViewSubscriber implements EventSubscriberInterface {
   @@ -122,12 +133,8 @@ public function onAjax(GetResponseForControllerResultEvent $event) {
   
   @@ -122,12 +133,8 @@ public function onAjax(GetResponseForControllerResultEvent $event) {
        $page_callback_result = $event->getControllerResult();
    
        // Construct the response content from the page callback result.
   -    $commands = ajax_prepare_response($page_callback_result);
   -    $json = ajax_render($commands);
   -
   -    // Build the actual response object.
   -    $response = new JsonResponse();
   -    $response->setContent($json);
   +    $response = $this->ajaxRenderer->render($page_callback_result);
   +    $response->prepareResponse($event->getRequest());
    
        return $response;
      }
   

   We may as well split this to its own subscriber, as we did for Html.

2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,77 @@
   +    // Most controllers return a render array, but some return a string.
   +    if (!is_array($content)) {
   +      $content = array(
   +        '#markup' => $content,
   +      );
   +    }
   

   For parallelism with the HTML pipeline, we should not be mixing the array rendering in with the rest of this logic. The renderer should already have a domain object (HtmlFragment, HtmlPage, etc.), and a render array is not a domain object.

   Which does suggest that perhaps we need a Drupal Ajax object other than AjaxResponse? Hm. It's not a perfect parallelism, I grant, but I am not comfortable with the Ajax renderer doing all three things when for Html the controller is responsible for normalizing render arrays to a domain object, and then the renderer just has to deal with the domain object.

Also, as written this means we have 2 redundant pipelines again, depending on whether an ajax request can sneak past the AjaxController. (If AJaxController does its job, then we never actually fire a view listener.) That's what we just fixed for HTML. Let's not introduce it here. :-)

Given that the same code is executed for onAjax and the proper ajaxController we should rather use the AjaxController on those cases as well.

Hm. It's still not exactly parallel, but under the circumstances that's probably OK. I think that's the best we're going to be able to do, but we still get a delightful diffstat out of it. :-)

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AjaxEnhancer.php
@@ -37,7 +37,7 @@ public function __construct(ContentNegotiation $negotiation) {
   public function enhance(array $defaults, Request $request) {
-    if (empty($defaults['_content']) && $defaults['_controller'] != 'controller.ajax:content' && $this->negotiation->getContentType($request) == 'drupal_ajax') {
+    if (empty($defaults['_content']) && $defaults['_controller'] != 'controller.ajax:content' && in_array($this->negotiation->getContentType($request), array('drupal_ajax', 'ajax', 'iframeupload'))) {
       $defaults['_content'] = isset($defaults['_controller']) ? $defaults['_controller'] : NULL;
       $defaults['_controller'] = 'controller.ajax:content';
     }

Do we still need this class at all?

This logic was intended as a BC shiv; in theory, ContentControllerEnhancer should be able to take care of ajax formats, too.

However, that would mean ajax-only routes (which we have a number of) would need to specify _content, not _controller, if they want to get the auto-wrapping controller. If they specify _controller, they MUST return an AjaxResponse themselves and they're on their own.

However, that is consistent with what is now required for HTML routes; if you specify _controller, you'd better not return a render array (or anything other than Response, HtmlFragment, or HtmlPage) or things WILL break and it's your own fault. And, in practice, I think that's what all of our ajax routes are doing now already, isn't it...?

So let's see what happens if we take AjaxEnhancer away completely and just add more mappings to ContentControllerEnhancer.

Seeing as this changes a major API, this should most likely be a beta blocker.

I agree that using _content for everything returning a render array would be nice consistency, though I really wonder whether the word "content" describes that properly. Personally I kind of connect content with HTML.
In addition though we would also add iframeupload and ajax support to the ContentControlleEnhancer if we really want to use it.
Do core maintainer have issues with this kind of small api changes these days?

I think it's fine. This issue is already ripping out the old API; let's rip it all the way.

Yeah we can always iterate on top of it.

This is just a normal reroll, #2168113: Add leading underscore and other discouragement to drupal_add_css() and drupal_add_js() conflict with it.

dawehner: Er, I meant it should be fine to go ahead and fold AjaxEnhancer into ContentControllerEnhancer.

Note that this will conflict with the new plan in #2026431: Make ContentNegotiation a "internal" service, used only by the router, so that core or contrib can implement real negotiation. I'm not sure which we want to do first.

dawehner: Er, I meant it should be fine to go ahead and fold AjaxEnhancer into ContentControllerEnhancer.

This would require ALL ajax callbacks to use _content instead which is kind of fine in, but certainly not in scope of this issue.
We just want to get rid of an old API.

56: ajax-1959574.patch queued for re-testing.

Screw it, let's unblock stuff.

56: ajax-1959574.patch queued for re-testing.

Fixed the documentation at the top of the ajax.inc file.
I kinda think that we should put that information into a better place but not sure really where.
Let's do that in a follow up, as otherwise the patch is too hard to review.

1. +++ b/core/includes/ajax.inc
   @@ -185,6 +185,10 @@
   + * As developer you basically create a \Drupal\Core\Ajax\AjaxResponse and add
   + * a couple of \Drupal\Core\Ajax\CommandInterface onto it, which will converted
   + * to a commands array automaticall.y
   

   huh?

2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,77 @@
   +      $error = $content['#error'];
   +      if (isset($error) && $error !== FALSE) {
   +        if ((empty($error) || $error === TRUE)) {
   +          $error = 'An error occurred while handling the request: The server received invalid input.';
   +        }
   +        $response->addCommand(new AlertCommand($error));
   +      }
   

   This logic looks really odd.

This is probably black magic.
if (!empty($error)) {} // Should have been enough I guess.
Or if you want to keep typing: if (NULL !== $error && FALSE !== $error) {} // And there you go.

This logic is indeed really confusing, what about this really simple concept? I would guess that the entire block could be skipped some as #type ajax is not really recommended anymore but just a AjaxResponse object.

That interdiff seems to be from another issue :) And here's a small review of the doc since the functionality is a bit alien to me:

1. +++ b/core/includes/ajax.inc
   @@ -185,6 +185,10 @@
   + * As developer you basically create a \Drupal\Core\Ajax\AjaxResponse and add
   

   Like Alex pointed out, this needs to be rephrased a bit. How about merging it with the phrase below?

2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
   @@ -0,0 +1,78 @@
   + * Converts a controller result into an Ajax response object.
   

   I'm not sure what to say about this one as well, does a controller have a "result"? Because it sounds really weird..

3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
   @@ -1670,7 +1670,8 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr
   +          //   and 'body', since these are used by
   +          // \Drupal\Core\Ajax\AjaxResponse::ajaxRender().
   

   Since I've been nitpicking so far, this needs two extra spaces.

Edit: For the second point above, how about 'Converts the output of a controller ...' or 'Converts the return value of a controller ....'?

Like Alex pointed out, this needs to be rephrased a bit. How about merging it with the phrase below?

Good idea, what about the small merge in the interdiff?

I'm not sure what to say about this one as well, does a controller have a "result"? Because it sounds really weird..

Lets use output in the main description and "return value" on the @param to be more precise on the technical level

Amateescu found that.

#65 and my doc review was addressed => back to RTBC. @dawehner++

Fixed a bunch of nitpicks. One question.

+++ b/core/lib/Drupal/Core/Routing/Enhancer/AjaxEnhancer.php
@@ -37,7 +37,7 @@ public function __construct(ContentNegotiation $negotiation) {
-    if (empty($defaults['_content']) && $defaults['_controller'] != 'controller.ajax:content' && $this->negotiation->getContentType($request) == 'drupal_ajax') {
+    if (empty($defaults['_content']) && $defaults['_controller'] != 'controller.ajax:content' && in_array($this->negotiation->getContentType($request), array('drupal_ajax', 'ajax', 'iframeupload'))) {

Shouldn't we at least document why we have both "drupal_ajax" and "ajax" as content types, and why AjaxEnhancer also needs to support the "iframeupload" content type?

Does this help?

+++ b/core/includes/ajax.inc
@@ -196,177 +197,30 @@
-    // Add the status messages inside the new content's wrapper element, so that
-    // on subsequent Ajax requests, it is treated as old content.
-    $commands[] = ajax_command_prepend(NULL, theme('status_messages'));

This hunk changed in #2177637: Replace theme() with drupal_render() in ajax.inc

Thank you, luckily this was already part of the code before.

Just unescaped the html in the issue summary

The last several patches were all just docs tweaks, so I think we're good to go here. Thanks, dawehner et al!

Committed e4e74fc and pushed to 8.x. Thanks!

Many of these changes seem opaque to anyone using the AJAX system through forms. Am I mistaken?

So it seems the change notice just needs some code examples from ajax_test.module, especially an example of adding a command e.g. $response->addCommand(new HtmlCommand('body', 'Hello, world!'));. And then it seems the only major change is the removal of ajax_render.

I think the JsonResponse->callback method is now returning the data on an insert command, meaning we can't simply invoke jQuery.ajax on a route that should return jsonp data to be eval'ed. I believe this issue broke the loading of the toolbar submenus.

Ultimately I think we'll need a jsonp command that does the data eval'ing in ajax.js. I started an issue here #2187483: Toolbar subtree is broken | JsonResponse->callback now returns data in an Insert Command without any support in ajax.js to eval the callback in the client.

We can also reenable this test now, too: #2187897: The testControllerResolutionAjax() test was commented out pending resolution of #1959574; reenable it.

Many of these changes seem opaque to anyone using the AJAX system through forms. Am I mistaken?

Right, if you use #ajax nothing changed for you.

So it seems the change notice just needs some code examples from ajax_test.module, especially an example of adding a command e.g. $response->addCommand(new HtmlCommand('body', 'Hello, world!'));. And then it seems the only major change is the removal of ajax_render.

https://drupal.org/node/1843212 already has the change notice here ... this issue was just about deprecating the existing code.

Ultimately I think we'll need a jsonp command that does the data eval'ing in ajax.js. I started an issue here #2187483: JsonResponse->callback now returns data in an Insert Command without any support in ajax.js to eval the callback in the client.

Cu on that issue. Sorry for breaking it.

Issue needs follow-up!

There's still ajax_render() and ajax_prepare_response() functions in core/includes/ajax.inc that are totally broken because calls removed ajax_command_*

Damnit I totally failed at the last reroll.

Filed the same to #2187735-3: Add removal information to docblock of all @deprecated functions

That works too. Thanks!

Oh xjm, I am sorry

#2203239: Remove ajax_render and co.