ajax_deliver() ignores #ajax['method'] and incorrectly forces 'replaceWith' for simple AJAX callbacks, D6->D7 regression

This is an unintended side-effect of the AJAX framework upgrade, and ideally should be fixed. I believe this patch fixes it. @rfay: can you add a test to this patch?

This looks good to me - I tested it using the Quick Tabs admin form and everything worked as it should (tried various methods). One thing I noticed is that in ajax.js, we have

  insert: function (ajax, response, status) {
    ....
    var method = response.method || ajax.method;
    ....

But surely response.method will always be null because of how ajax_command_insert is defined, i.e.

function ajax_command_insert($selector, $html, $settings = NULL) {
  return array(
    'command' => 'insert',
    'method' => NULL,  // Means client will use #ajax['method']
    'selector' => $selector,
    'data' => $html,
    'settings' => $settings,
  );
}

Therefore, in the js code, shouldn't it just be var method = ajax.method?

Thanks! Some of the other ajax_command_*() functions (such as ajax_command_replace()) return a 'command' => 'insert' and a 'method' => SOMETHING. That's why response.method is checked in ajax.js.

OK, ignore that last comment - I didn't realise that Drupal.ajax.prototype.commands.insert() is used by ajax_command_prepend() as well as ajax_command_insert(). Still playing around with it...

@effulgentsia - thanks, cross-posted ;-)

Finished playing around - works perfectly :-)

+++ includes/ajax.inc
@@ -396,13 +397,16 @@ function ajax_deliver($page_callback_result) {
-    $commands[] = ajax_command_replace(NULL, $html);
+    $commands[] = ajax_command_insert(NULL, $html);

But _replace() is not removed in this patch?

+++ includes/ajax.inc
@@ -548,6 +552,37 @@ function ajax_command_alert($text) {
+    'method' => NULL,  // Means client will use #ajax['method']

Either move this comment above the remarked line or drop it and clarify the PHPDoc instead.

+++ misc/ajax.js
@@ -323,6 +323,12 @@ Drupal.ajax.prototype.commands = {
+    // 'replace' is deprecated in favor of 'replaceWith', but continue to
+    // support Drupal modules using the older method name.
+    if (method == 'replace') {
+      method = 'replaceWith';
+    }

Which kind of BC is meant here? D6 or D7? Either way, I'd say we can drop this. Yet, we are in alpha, not beta.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -180,6 +180,17 @@ function ajax_forms_test_ajax_commands_form($form, &$form_state) {
+    // Shows the AJAX 'insert' command.
+  $form['insert_command_example'] = array(

Wrong indentation here.

+++ modules/simpletest/tests/ajax_forms_test.module
@@ -315,6 +326,15 @@ function ajax_forms_test_advanced_commands_html_callback($form, $form_state) {
+  return array('#type' => 'ajax_commands', '#ajax_commands' => $commands);

Wasn't the patch changing the #properties already committed?

Powered by Dreditor.

CNW for #8. Am hoping to get to it soon.

Rerolled as per sun's comments in #8.

Adjusted the test as necessary...

+++ includes/ajax.inc	26 Apr 2010 06:04:39 -0000
@@ -20,11 +20,10 @@
+ * ajax_form_callback() and a defined #ajax['callback'] function. ¶

Trailing white-space.

Powered by Dreditor.

No more trailing white-space.

Looks good to me. I hope that effulgentsia can approve this patch and RTBC.

Re #8:

Which kind of BC is meant here? D6 or D7? Either way, I'd say we can drop this. Yet, we are in alpha, not beta.

With the #15 patch dropping the suggested hunk, we are introducing a BC break. Any module that is setting #ajax['method'] to 'replace' (instead of 'replaceWith' or NULL) won't work any more. This patch removes the redundant setting of 'method' from field, file, and poll modules, but is this an acceptable break to contrib modules? It is quite likely that there is some D7 contrib module out there using 'method' => 'replace'.

This adds the following hunk to ajax_process_form() to maintain BC:

+    // @todo Legacy support. Remove in Drupal 8.
+    if ($settings['method'] == 'replace') {
+      $settings['method'] = 'replaceWith';
+    }
+

I think this is better than #3 which did the equivalent in ajax.js, but maybe that's just my bias towards preferring cruft in PHP rather than in JS.

I just started to play with something weird (effulgentsia knows what) and stumbled over this issue.

Problem is: #ajax[wrapper], without anything else, defines a "wrapper", not an "element to replace", but totally a wrapping bounding box.

When using #ajax[wrapper], the entire element is replaced currently. Therefore, you can't use #ajax on links and stuff - without specifying "replace" as method (...somehow...).

This patch properly fixes the bug and my fancy code starts to work.

Committed to CVS HEAD. Thanks.