Rebuild the server side AJAX API

Thanks, mkadin! Yes, this is the general direction we were looking at. Some comments below:

+++ b/core/lib/Drupal/Core/Ajax/AjaxCommandReplace.php
@@ -0,0 +1,29 @@
+class AjaxCommandReplace implements AjaxCommandInterface {

I think the "AjaxCommand" prefix on all classes is redundant. I know we sometimes include the namespace per coding standards, but in this case I think just CommandReplace, CommandInterface, etc. is sufficient.

+++ b/core/lib/Drupal/Core/Ajax/AjaxCommandReplace.php
@@ -0,0 +1,29 @@
+  public function __construct($selector, $html, $settings = NULL) {
+    $this->selector = $selector;
+    $this->html = $html;
+    $this->settings = $settings;
+  }

We should investigate replacing the $settings array with well-defined methods. Anonymous config arrays are yucky in general, and good OO lets us avoid them.

(That said, I don't know the Ajax system well enough to know if we need to do it this way due to the Javascript side. In that case, it should be type hinted as an array.)

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -0,0 +1,33 @@
+  public function addCommand($command) {
+    $this->commands[] = $command->render();
+  }

I recommend delaying the rendering process until the send() method. Let send do all of the compilation at the end of the request, then ship the response.

We tried to do early compilation for various parts of the DB query objects in Drupal 7, and eventually gave up and did it all on execute(). Let's just do the same here and avoid going through the same dance.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -0,0 +1,33 @@
+  /**
+   * Sets the response's data to be the array of AJAX commands.
+   */
+  public function ajaxPrepare() {
+    parent::setData($this->commands);
+  }

I don't think this is necessary. There's already a prepare() and send() method that get called in index.php already. If we need to do anything in particular with those, extend them and call their parents as needed.

+++ b/core/modules/file/file.module
@@ -797,9 +797,10 @@ function file_ajax_upload() {
+    $response = new AjaxResponse();
+    $response->addCommand(new AjaxCommandReplace(NULL, theme('status_messages')));
+    $response->ajaxPrepare();
+    return $response;

As above, no need to call ajaxPrepare().

I'd love to also replace theme('status_messages') (dear god), but that's for a different follow up.

I think it's also easier to not convert existing callbacks first but to write proper unit tests. Then we we can test those directly, make sure everything works, then integrate it into real callbacks. That way the testing process is a few seconds per run, not a few minutes per run. :-)

Minor nitpick: CommandInterface and ReplaceCommand would be better names.

don't think its possible that a dev might be confused about what the heck a CommandReplace object is

Objects usage is contextual, I sincerely think that the common human brain will understand it quite well as long as it is used correctly. For the edge cases where we'd find out it's confusing, we still can alias the class name.

In order to make it less confusing, we could adopt something like the Predis library is doing:

return new AjaxResponse()
  ->replace('some_identifier', 'some_content')
  ->remove('some_other_identifer')
  ->html('yet_another_identifier', 'yet_another_new_content');

And this can be achieved "simply" by something like this:

class AjaxResponse {
  // ...
  public function __call($name, $arguments) {
    $className = ucfirst($name) . 'Command';
    if (class_exists($className)) {
      $this->addCommand(new $className(/* Figure out something for arguments */));
    }
    return $this;
  }

I don't know, not much because you wouldn't return 1000 AJAX commands in one page, you'll have a maximum of 10 or 20, in that case, 20 class_exists() is pretty much nothing.

Make the idea in #8 a convenience rather than the only way to use it, and then if it's a performance issue then people won't have to use it.

As for the magic, I would imagine that for end users of the system they will only care that the 'correct' way is documented, and for developers they can have a look at the magic method and as it's actually relatively simple it should make sense.

In practice, these classes would be used mostly from controllers or _content pseudo-controllers. Those will be, if we do it right, fairly short classes. That means "wayyy at the top of a particular file" is maybe 30 lines away in the typical case. That said, let's not bikeshed that if we don't have to.

For __call(), punt. That's a bit of syntactic sugar we can decide about later, after the rest of the refactoring is done. I'm fine without it for now (and I'm not sold on it in the first place).

Yes true, I was just suggesting this it sounded nice at the time. This patch must move forward without or without syntactic sugar.

+++ b/core/lib/Drupal/Core/Ajax/AddCssCommand.php
--- /dev/null
+++ b/core/lib/Drupal/Core/Ajax/AfterCommand.php

+++ b/core/lib/Drupal/Core/Ajax/AfterCommand.php
+++ b/core/lib/Drupal/Core/Ajax/AfterCommand.php
@@ -0,0 +1,38 @@

@@ -0,0 +1,38 @@
+<?php
+
+/**
+ * @file
+ * Definition of Drupal\Core\Ajax\AfterCommand
+ */
+
+namespace Drupal\Core\Ajax;
+
+use Drupal\Core\Ajax\CommandInterface;
+
+/**
+ * The 'insert/after' command instructs the client to use jQuery's after()
+ * method to insert the given HTML content after each element matched by the
+ * given selector.
+ *
+ * This command is implemented by Drupal.ajax.prototype.commands.insert()
+ * defined in misc/ajax.js.
+ * ¶
+ * @see http://docs.jquery.com/Manipulation/after#content
+ */
+class AppendCommand extends InsertCommand {

I think you have the wrong class name here. That's probably causing at least some of the failures.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -0,0 +1,107 @@
+  /**
+   * Sets the response's data to be the array of AJAX commands.
+   * ¶
+   * @param $request A request object.
+   */
+  public function prepare(Request $request) {

Purely organizational nitpicking, but I think it makes sense to move most of this method to a protected compile() method or similar. Then prepare() itself can be dead simple:

$this->setData($this->compile());
parent::prepare($request);

That makes it clearer when reading it that we're doing extra data setting compilation first, then doing "whatever the parent wants", rather than those two lines getting lost at the bottom of a long method.

+++ b/core/lib/Drupal/Core/Ajax/ChangedCommand.php
@@ -0,0 +1,50 @@
+/**
+ * This command instructs the client to mark each of the elements matched by the
+ * given selector as 'ajax-changed'.
+ *
+ * This command is implemented by Drupal.ajax.prototype.commands.changed()
+ * defined in misc/ajax.js.
+ */

Class docblocks need a one-line initial description, just like functions/methods. The same is true for properties, which should then also have an @var declaration stating what their data type is.

That said, I love how heavily documented this code is, even if the format needs tidying. :-)

+++ b/core/lib/Drupal/Core/Ajax/RestripeCommand.php
@@ -0,0 +1,42 @@
+/**
+ * Creates a Drupal Ajax 'restripe' command.
+ *
+ * The 'restripe' command instructs the client to restripe a table. This is
+ * usually used after a table has been modified by a replace or append command.
+ *
+ * This command is implemented by Drupal.ajax.prototype.commands.restripe()
+ * defined in misc/ajax.js.
+ */
+class RestripeCommand implements CommandInterface {

Totally not something to change here, but I wonder if this command is even necessary anymore. Aren't we getting rid of even/odd classes as we only support browsers that understand the appropriate pseudo-classes?

I don't know what the testbot is complaining about. I didn't see a syntax error visually, but if you throw the code into an IDE it will probably make it painfully obvious. :-)

And OMG I had no idea we had so many ajax commands! (Yet another point of this exercise, discovery.)

For tests, start with unit tests. Manually instantiate a class, poke at it, and make sure it behaves as expected. Then add a small number of WebTest cases, which verify that we can wire everything together. Stop by #drupal-contribute for more direct tutorials, as that's easier than me trying to explain something in a comment. :-) (There's a docs page somewhere, but I don't recall where exactly.)

+++ b/core/lib/Drupal/Core/Ajax/DataCommand.php
@@ -0,0 +1,69 @@
+/**
+ * An AJAX command for calling the jQuery remove() method.
+ *
+ * The 'remove' command instructs the client to use jQuery's remove() method
+ * to remove each of elements matched by the given selector, and everything
+ * within them.
+ *
+ * This command is implemented by Drupal.ajax.prototype.commands.remove()
+ * defined in misc/ajax.js.
+ *
+ * @see http://docs.jquery.com/Manipulation/remove#expr
+ */
+class DataCommand implements CommandInterface {

Docblock needs to be updated. Looks like a copy-paste error.

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/AjaxCommandsUnitTest.php
@@ -0,0 +1,238 @@
+  /**
+   * Tests that each AJAX command class can be constructed and rendered.
+   */
+  function testAjaxCommands() {

These need to be broken up to separate methods for each test. It's bad form to shove them all into a single test method.

A lot of the method and property docblocks need reformatting for one-line summaries. I didn't call them out individually but there's a bunch of them.

Also, I asked catch and he said we should convert 1-2 uses of Ajax responses in core to the new system in this patch. It doesn't have to be all of them, just 1-2 to show that it does actually work outside of unit tests.

I think this is really close. Let's drive it home.

#25: ajax_api_1812866_25.patch queued for re-testing.

Retesting because some ajax related code landed recently. This patch may have landed after that but retesting just in case.

Gave this a thorough review. Here are some nitpicks.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.phpundefined
@@ -0,0 +1,127 @@
+        $items[$type] = array();
+      } else {

Code style.

+++ b/core/lib/Drupal/Core/Ajax/CommandInterface.phpundefined
@@ -0,0 +1,22 @@
+
+  /**
+   * Return an array to be run through json_encode and sent to the client.
+   */
+  public function render();

Is this really rendering? Seems more like building.

+++ b/core/lib/Drupal/Core/Ajax/HtmlCommand.phpundefined
@@ -0,0 +1,40 @@
+  /**
+   * Implements  Drupal\Core\Ajax\CommandInterface:render().

It looks like most of the Implements have 2 spaces after them.

+++ b/core/lib/Drupal/Core/Ajax/RemoveCommand.phpundefined
@@ -0,0 +1,53 @@
+/**
+ * @file
+ * Definition of Drupal\Core\Ajax\RemoveCommand

Missing periods after @file statements.

+++ b/core/lib/Drupal/Core/Ajax/ReplaceCommand.phpundefined
@@ -0,0 +1,40 @@
+/**
+ * @file
+ * Definition of Drupal\Core\Ajax\ReplaceCommand;

Semi-colon.

I don't really have a preference about render() vs. some other name.

After #27 is addressed I think this is RTBC.

I'm not very happy with all the references to jQuery in the doc. Most of it not really jQuery specific. For example, when saying: A jQuery selector string. what is meant is A CSS selector string. or at least that what should be encouraged, not everyone will be rolling Sizzle (the selector engine jQuery uses), just like we don't allow (or very much discourage at least) mysql-specific things in the DB API.

Also I'd like to know what is contrib using, really. The CssCommand, it's a terrible thing to use. It should be adding/removing a class, not changing CSS properties directly on an element. JS should not mess around directly with the style (separation of concerns and all). Can someone do a grep on contrib and let me know which ones are using any of the following:

• ajax_command_css (see above)
• ajax_command_invoke (this sounds like someone that should be using a custom ajax command).
• ajax_command_alert (alert… if that's really needed why not a console.log thing to replace it? otherwise we'd need prompt/confirm as well)
• ajax_command_data (stuff that should probably be in drupalSettings somehow)

I'd like to get rid of them altogether or at least depreciate them. I'm happy to know if there is a valid and resonable use case for them. It's not because we can make them that we should :)

Also the append, prepend, before, after and everything that extends InsertCommand we really need all those classes? I don't know but a parameter in the constructor could work just as well to set method of the command array?

woops, xpost with testbot :p

nod_: There's probably a good case to be made that many of these commands are dumb, or outdated. I said above in #16 that the restripe command probably should go away entirely as we won't be supporting any browsers that would require it in the first place. However, this is just an API conversion issue from undocumented arrays to documented classes; cleaning up what commands we actually support is a follow-up issue, unless this issue is introducing any new ones. (I don't think it is, but the current one is so opaque that I can't be sure!)

Let's save rethinking which commands core should ship with (and which should be treated as redundant with each other) to a later issue. (I have some thoughts on changing how the commands are delivered, too, but that also should come later.)

fine with me :)

Well, you can do a lot with CSS3 selectors without fancy jQuery additions. And the jquery-specific non-CSS3 stuff is slower, not a good thing to encourage. And some like let's say, me, could be taking sizzle/jQuery out of his website and would expect the selectors to not break.

The before/after stuff is still tied to jQuery, hopefully that won't be true in the future.

I can give a concrete use case of ajax_command_invoke().
I'm using a jQuery library to make fancy scrollbars, and I need to call its update() method after appending new data via ajax. I suppose it would be more technically correct to create a custom command, but it's very convenient.

Thanks, much better.

Checked, comments from #27 have been fixed. Per #28 if comes back green that's RTBC.

The patch doesn't actually remove core/includes/ajax.inc, is that because not everything has been converted yet or was it an oversight?

Generally this looks good and the various follow-ups sound good to (and good to do in follow-ups rather than the straight conversion here).

I opened #1842576: Review AJAX system commands, decouple from jQuery for reviewing the existing commands and removing restripe.

A few minor things I noticed:

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.phpundefined
@@ -0,0 +1,128 @@
+        $function = 'drupal_add_' . $type;

This desperately needs refactoring when we remove the drupal_add_*() functions but not for this issue.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.phpundefined
@@ -0,0 +1,128 @@
+        // Ensure that the page doesn't reload what it already has.
+        $items[$type] = array_diff_key($items[$type], $_POST['ajax_page_state'][$type]);

Is there no request object available here?

+++ b/core/lib/Drupal/Core/Ajax/ChangedCommand.phpundefined
@@ -0,0 +1,65 @@
+   * @param string $asterisk
+   *   CSS selector for elements to which an askterisk will be appended.

askterix ;)

This fixes the askterisk typo. Asterisk (not askterisk, asterix, or askterix) is the correct spelling.

I haven't reviewed #40, but it was RTBC before. This is the interdiff since then, and it looks good to me, so back to RTBC.

@mkadin: no that's fine, we just need a follow-up to convert/remove once it's committed :)

Patch in #45 isn't green yet, but I'll commit once it is next time I'm doing a run through the queue.

There are also the views_ajax_command_*s that need to be addressed. It looks like at least a few of those should be moved out of views/generalized. Should that go in #1842576: Review AJAX system commands, decouple from jQuery or a separate issue?

Re #47: Once the baseline work (this patch) is in, I'd recommend breaking the follow ups into as many small issues as possible so that they can be parallelized.

#45 is now green, so we're just waiting on catch's next issue sweep. Yay!

Views has lots of custom, redundant, and legacy AJAX that needs refactoring. If opening specific issues for that, please check the VDC queue first for duplicates and apply that tag to any new issues needed.

Committed to 8.x. Thanks.

Change notice: http://drupal.org/node/1843212

Follow up issues:
#1843220: Convert form AJAX to new AJAX API
#1842576: Review AJAX system commands, decouple from jQuery
#1843224: Convert Views Ajax commands to new Ajax API

mkadin++!

Also related: #1667742: Add abstracted dialog to core (resolves accessibility bug). I'm hoping to have a new patch there soon and would really appreciate reviews from knowledgeable AJAX API developers when I post it.

Corrected a typo

Fixed the change notice typo and updated issue summary with follow up and related issues mention in #51 and #52.

This broke core.

Specifically, AjaxResponse::ajaxRender() broke core.

For Edit, it was raised in #1824500-15: In-place editing for Fields that I should convert to this already. So I did that. Everything works. Except for the "Remove" button in a file field. That button doesn't have its behavior executed and thus no Drupal.ajax attached to it, thus it just opens a blob of JSON upon clicking.

Note that this is a file field on a page that does not already contain a file field, thus also does not yet contain file.js. Through AJAX, I load a form with a file field, which thus has the drupal.file library attached (which loads file.js, which contains the corresponding Drupal behavior). Thanks to ajaxPageState, it is then ensured that any JS that the HTML (loaded through AJAX) needs, also gets loaded.

Before this patch, file.js was loaded upon loading a form that needs it through AJAX. Afterwards, it is not. Something appears off with the AJAX page state handling.

I can imagine this also has repercussions for Views' UI.

Wim: I'm not sure I follow. Are you saying that with this code not all of the necessary JS is loaded client-side?

#57: That's right. I don't have time to roll a "limited in scope" patch to reproduce this, unfortunately. ajaxRender() looks fine-ish (since it's a straight port), so maybe we need to look at the data that is being fed into it? ajaxRender() uses $request->parameters, ajax_render() uses $_POST…

You can reproduce this with a file field edited through Edit.module, apply the patch at #1824500-17: In-place editing for Fields: the Edit patch itself and the patch for that patch (patchception!) that leverages the APIs introduced in this issue. But that's very far from ideal for you, because it's as far from a simple test case as you could possibly get.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -66,12 +66,13 @@ protected function ajaxRender($request) {
-      if (empty($request->parameters['ajax_page_state'][$type])) {
+      if ($ajax_page_state[$type]) {

We need the empty(); otherwise, PHP will throw notices if $type doesn't exist in the array at all. Otherwise, this patch looks great.

Also, we're apparently missing test coverage, since the original patch should have triggered some test to fail, given the fix in #59.

Hah, yay! :)

It fixed the problem, but it's causing more problems. There's still something wrong with the AJAX page state.

Without AjaxResponse:

With AjaxResponse, plus the patch in #59:

As you can see, files that *must* have been loaded already are loaded again. E.g. jquery.js, drupal.js, ajax.js.

Crosspost.

Almost.

The remaining problem is the combination of these factors:
- the server side lets know the client side to which elements Drupal.ajax behaviors should be attached. Thus, when Drupal.attachBehaviors() gets called, it must already exist in Drupal.settings.
- the "insert" AJAX command immediately applies Drupal.attachBehaviors on the newly inserted content.

In other words: the new Drupal.settings from the server side ("settings" command) should be known BEFORE the new content is inserted ("insert" command). This used to be the case, but is no longer true.

Without AjaxResponse:

[{command:settings,…}, {command:add_css,…}, {command:insert, method:prepend, selector:head,…},…]
0: {command:settings,…}
1: {command:add_css,…}
2: {command:insert, method:prepend, selector:head,…}
3: {command:edit_field_form, id:node:1:field_image:und:teaser,…}

With AjaxResponse:

0: {command:edit_field_form,…}
1: {command:add_css,…}
2: {command:insert, method:prepend, selector:head,…}
3: {command:settings,…}

As you can see, the settings are being inserted too late. (The "edit_field_form" command does several things, one of which is *also* calling the "insert" command. The "insert" command at position 2 and 3 above is for inserting new JS into the tag.)

So the cause is actually the fact that you're doing this:
$this->addCommand(new SettingsCommand(call_user_func_array('array_merge_recursive', $settings['data']), TRUE));
instead of:
array_unshift($commands, ajax_command_settings(call_user_func_array('array_merge_recursive', $settings['data']), TRUE));

Append instead of prepend.

As well as:

    if (!empty($styles)) {
      $this->addCommand(new AddCssCommand($styles));
    }
    if (!empty($scripts_header)) {
      $this->addCommand(new PrependCommand('head', $scripts_header));
    }
    if (!empty($scripts_footer)) {
      $this->addCommand(new AppendCommand('body', $scripts_footer));
    }

Instead of:

  $extra_commands = array();
  if (!empty($styles)) {
    $extra_commands[] = ajax_command_add_css($styles);
  }
  if (!empty($scripts_header)) {
    $extra_commands[] = ajax_command_prepend('head', $scripts_header);
  }
  if (!empty($scripts_footer)) {
    $extra_commands[] = ajax_command_append('body', $scripts_footer);
  }
  if (!empty($extra_commands)) {
    $commands = array_merge($extra_commands, $commands);
  }

In short: things have been simplified, but absolutely essential order guarantees have been broken. To unbreak everything:
- if there's a "settings" command, it must come first
- if there's a need to add more CSS or JS (be it in header or footer), they must come after any "settings" commands, but before any other commands.

This guarantee of order most definitely needs tests.

That sounds good. As a follow up, I could see that evolving to where the parameter could be a command object in order to add a command immediately after that one. But that would require moving command rendering from addCommand() to ajaxRender(). And it might not be very useful without a public getCommands() method. So for now, a parameter for beginning or end is a great first step.

Also, per #64, let's not hold this up on tests. We have quite a few test classes already based on AjaxTestBase that test the old system, and we can move those existing tests to test the new system, once we switch all of core over to the new system.

Oops. We really were missing test coverage for order, even in the old system. I opened a separate issue for that: #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended.

Is this really a bug? Sounds more like a feature request to me.

It's already been committed, it's a follow-up to the initial commit since #54. Please read the issue it is a bug now.

Downgrading to normal, because nothing is using the new API yet, so the regression doesn't seem critical to me. If #1843220: Convert form AJAX to new AJAX API lands before this one, then this will need to be critical, but I'm confident this one will get fixed first anyway.

What's in the patch looks good, but this part of ajaxRender:

if (!empty($styles)) {
      $this->addCommand(new AddCssCommand($styles));
    }
    if (!empty($scripts_header)) {
      $this->addCommand(new PrependCommand('head', $scripts_header));
    }
    if (!empty($scripts_footer)) {
      $this->addCommand(new AppendCommand('body', $scripts_footer));
    }

is still getting added to the end instead of beginning. We can't just pass TRUE in for each of these though, because then they'll be added in the wrong order relative to each other. We can reverse the order of the if statements, but I think it would be bad DX for these 3 to appear in the code in the opposite order of how they're run. Do you have any ideas for how to do it cleanly? PHP has a really nice set of array functions: it's a shame we lose them by going to OOP here.

How about this?

#79 works flawlessly. RTBC.

Two very very minor doc nitpicks, of which I'm not even sure.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.phpundefined
@@ -27,20 +27,29 @@ class AjaxResponse extends JsonResponse {
+   *   before previously added commands.  Defaults to false.

- Two spaces, should be one?
- "false" should be "FALSE"?

Thanks, @mkadin :)

Committed and pushed to 8.x. Thanks!

I would not normally commit a bug fix like this without test coverage, but it looks like a more holistic approach to test coverage for weighting is being tackled at #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended

Restoring properties. Looks like a change notice was already added in #51.

Do we need an update in change notice?

Nope. The existing one still seems appropriate to me.

Apparently a follow-up issue to delete the D7 Ajax API was created at #1959574: Remove the deprecated Drupal 7 Ajax API without notifying followers on this issue.

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: ".

The change notification here is using a procedural callback. We need to update it to use a controller so that people trying to upgrade modules aren't totally confused.

Wheeee https://drupal.org/node/1843212/revisions/view/2794695/2816863