Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended

Yay! :)

Turns out the original tests were broken & incomplete to begin with… :(

Furthermore, these tests actually test ajax.inc/ajax_render(), because the tests leverage Form API's AJAX integration, which is still being ported to the new AJAX API at #1843220-35: Convert form AJAX to new AJAX API. So this won't help prevent bugs in AjaxResponse.php yet.

As said at #1812866-65: Rebuild the server side AJAX API by myself (after reading the D7 code — which has always worked well — carefully):

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.

Despite being blocked by #1843220: Convert form AJAX to new AJAX API to test the order of AJAX commands (in testLazyLoad()), it is in fact possible to test the order if we don't use the Form API integration with the AJAX API to do so. That being said, I think it's still valuable to keep the order checks in testLazyLoad() that were added in the OP.

Attached is a patch that does so.

Over at #1875632-39: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings), quicksketch reported a regression introduced accidentally by effulgentsia at #1875632-31: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings). That's the second time now (first time: #1812866-54: Rebuild the server side AJAX API, solution: #1812866-65: Rebuild the server side AJAX API) that AJAX command ordering got broken, because there is no test coverage for it. Hence I've bumped this to major.

I've rolled the patch twice:

• once with just tests (should fail, because it doesn't fix the bug reported by quicksketch)
• once with tests + quicksketch' fix (should pass)

Ugh, these two patches were based on top of a Drupal core which had the bug reported by quicksketch already fixed, so they're wrong. So much careful work and planning only to be ruined by such a stupid mistake!

Ignore files of #2.

Rerolled.

Better title as per #2/#3.

#3: ajax_command_order-1848880-3.patch queued for re-testing.

#3: ajax_command_order-1848880-3.patch queued for re-testing.

Last breakage caused by #1883152-54: Field level access for EntityNG. Hopefully this time it'll come back green.

#3: ajax_command_order-1848880-3.patch queued for re-testing.

Take my 'needs work' here lightly as I've never reviewed a core test before and haven't written one for D8.

Generally this looks pretty good to me. Few small comments / ideas below.

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.phpundefined
@@ -45,6 +40,69 @@ function testAJAXRender() {
+    $expected_commands[1] = array(
+      'command' => 'add_css',
+      'data' => drupal_get_css(drupal_add_css(), TRUE),

Might it make sense to actually create an Ajax Command object and use its render() method to produce the expected command arrays? That way if the data / structure of this array ever changes, the test won't need to be re-written.

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.phpundefined
@@ -103,16 +161,20 @@ function testLazyLoad() {
-    $this->assertTrue(!isset($original_settings[$expected['css']]), format_string('Page originally lacks the %css file, as expected.', array('%css' => $expected['css'])));
-    $this->assertTrue(!isset($original_settings[$expected['js']]), format_string('Page originally lacks the %js file, as expected.', array('%js' => $expected['js'])));
+    $this->assertTrue(!isset($original_css[$expected['css']]), format_string('Page originally lacks the %css file, as expected.', array('%css' => $expected['css'])));
+    $this->assertTrue(!isset($original_js[$expected['js']]), format_string('Page originally lacks the %js file, as expected.', array('%js' => $expected['js'])));
 
-    // Submit the Ajax request without triggering files getting added.

Good catch.

+++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.phpundefined
@@ -127,27 +189,30 @@ function testLazyLoad() {
+    // Verify the expected setting was added, both to drupalSettings, and as
+    // the first AJAX command.
     $this->assertIdentical($new_settings[$expected['setting_name']], $expected['setting_value'], format_string('Page now has the %setting.', array('%setting' => $expected['setting_name'])));

'AJAX' vs. 'ajax' vs. 'Ajax' :) What's the right way and do we care? There are several of these scattered throughout your docs.

+++ b/core/modules/system/tests/modules/ajax_test/ajax_test.moduleundefined
@@ -60,6 +70,27 @@ function ajax_test_render() {
+  $response = new AjaxResponse();
+  $path = drupal_get_path('module', 'system');
+  // Add two CSS files (order should remain the same).
+  drupal_add_css($path . '/system.admin.css');
+  drupal_add_css($path . '/system.maintenance.css');
+  // Add two JavaScript files (first to the footer, should appear last).
+  drupal_add_js($path . '/system.modules.js', array('scope' => 'footer'));
+  drupal_add_js($path . '/system.js');
+  // HTML insertion command.
+  $response->addCommand(new HtmlCommand('body', 'Hello, world!'));
+  // Finally, add a JavaScript setting.
+  drupal_add_js(array('ajax' => 'test'), 'setting');

Since we want to make sure that the CSS and JS get lazy loaded before the html command, it might make sense to put the HTML command before the drupal_add_*. Wouldn't affect the order in how we have it set up now, but who knows in the future?

'AJAX' vs. 'ajax' vs. 'Ajax' :) What's the right way and do we care?

The right way is "Ajax".

Thanks for the review!

RE: "Might it make sense to actually create an Ajax Command object and use its render() method to produce the expected command arrays? That way if the data / structure of this array ever changes, the test won't need to be re-written."
Great point!

I've done this, at the cost of introducing changes that are — strictly seen — unrelated to this patch.

It does make for a nice interdiff though: 94 insertions, 164 deletions :)

RE: "AJAX" capitalization
I don't want to waste time on this while it is extremely inconsistent already. Anything user-facing is "AJAX", most code in the AJAX framework is "Ajax", but e.g. in WebTestCase, it's drupalPostAJAX(). I'd much rather stick to "AJAX" everywhere except in function names, which is the simplest and most logical thing to do, but what we really should do is defer "Bring consistency to Ajax vs. AJAX vs. ajax" to a separate issue, which settles this once and for all and then applies it everywhere, consistently.

RE: "[…] it might make sense to put the HTML command […]"
Agreed. Changed.

Looks good to me.

Wow, great job withe the refactoring/tests in this patch! Thanks for the review as well, mkadin! :)

Committed and pushed to 8.x. Thanks!

Yay, thanks guys!

.