JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides [#2019481]

as shown in #1678002: Edit should provide a usable entity-level toolbar for saving fields the way our ajax commands are implemented is wrong.

our docs say:

/**
 * Ajax object.
 *
 * All Ajax objects on a page are accessible through the global Drupal.ajax
 * object and are keyed by the submit button's ID. You can access them from
 * your module's JavaScript file to override properties or functions.
 *
 * For example, if your Ajax enabled button has the ID 'edit-submit', you can
 * redefine the function that is called to insert the new content like this
 * (inside a Drupal.behaviors attach block):
 * @code
 *    Drupal.behaviors.myCustomAJAXStuff = {
 *      attach: function (context, settings) {
 *        Drupal.ajax['edit-submit'].commands.insert = function (ajax, response, status) {
 *          new_content = $(response.data);
 *          $('#my-wrapper').append(new_content);
 *          alert('New content was appended to #my-wrapper');
 *        }
 *      }
 *    };
 * @endcode
 */

This is good, this is what everyone expects. But it's a lie.

What happens is that Drupal.ajax.prototype.commands = {} is a single object shared by all ajax objects. What was ment is something like Drupal.ajax.prototype.commands.prototype = {} (don't try it at home that's not where we want to end up). When you follow the docs, you don't override one command from the one ajax object you're working with, you override the command for all ajax objects ever.

what we need is

Drupal.ajax = function () {
  this.commands = new Drupal.AjaxCommands();
};

Drupal.AjaxCommands = function () {};  // empty, on purpose
Drupal.AjaxCommands.prototype = {/* the old command object */};

and now the doc is right.

HAI I DO JAVASCRIPT!

Comment	File	Size	Author
#3	core-js-ajaxcommand-2019481-3.patch	23.4 KB	nod_
#3
#1	core-js-ajaxcommand-2019481-1.patch	6.6 KB	nod_
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

nod_

French

Lille

CreditAttribution: nod_ commented 14 June 2013 at 09:18

Status:

Active

» Needs review

File	Size
core-js-ajaxcommand-2019481-1.patch	6.6 KB

Comment #2

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 14 June 2013 at 11:10

Title:	javascript ajax commands object is borked	» JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides
Status:	Needs review	» Needs work
Issue tags:		+sprint, +in-place editing, +Spark

Context

I spent almost 4 hours on figuring out WTF was going wrong. Any Drupal.ajax command that did something like

Drupal.ajax['edit-submit'].commands.insert = function (…) {…}

would effectively be changing the "insert" command for all other Drupal.ajax instances, even those that are created later.

After about 4 hours of banging my head against the desk, after getting very close to tears of desperation several times (because I was really doing exactly what the docs prescribed!), after investigating whether maybe Chrome's JS engine was applying an optimization it shouldn't be doing, after questioning pretty much anything, it dawned upon me that this was just JavaScript's prototype being used incorrectly.

I implemented a work-around to get my patch done, and I fixed the docs. The code here is the better solution, that instead keeps the docs as-is and introduces prototype at the right object.

So, all of the above is all based on problems I found and discussed with nod_. Thanks to nod_ for writing it up in a less hate-driven manner than I would definitely have done.

To test

If a reviewer/core committer wants to verify this does indeed fix things, go to the D8 HEAD frontpage and type Drupal.ajax.prototype.commands.insert in the browser console:

without patch, you see that the prototype is overridden:

function (ajax, response, status) {
    _.defer(function() { callback(); });
    realInsert(ajax, response, status);
  }

with patch, not anymore:

unction (ajax, response, status) {
    // Get information from the response. If it is not there, default to
    // our presets.
…

Detailed testing/validation

I rerolled #1678002: Edit should provide a usable entity-level toolbar for saving fields on top of the patch in #1 (see #147 there), to thoroughly validate this patch. It works perfectly.

Code review

The changes are crystal clear.

What's missing though, is documentation updates in PHP files, which all still refer to Drupal.ajax.prototype.commands. If that's fixed, this is RTBC.

Comment #3

nod_

French

Lille

CreditAttribution: nod_ commented 14 June 2013 at 11:18

Status:

Needs work

» Needs review

File	Size
core-js-ajaxcommand-2019481-3.patch	23.4 KB

with doc changes.

Comment #4

nod_

French

Lille

CreditAttribution: nod_ commented 14 June 2013 at 11:21

Issue tags:

+Needs change record

an I guess

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 14 June 2013 at 11:49

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

Comment #6

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 14 June 2013 at 13:12

Status:

Reviewed & tested by the community

» Fixed

Committed d087f4e and pushed to 8.x. Thanks!

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 14 June 2013 at 13:26

Issue tags:

-sprint

Wow, that must've been one of the fastest fixes ever! :D

I accidentally removed the "Needs change notification" (well, d.o did). I'm not sure if it's really necessary, it's a very superficial change. But it's a change. I'll leave it to nod_ to decide.

Comment #8

jessebeach CreditAttribution: jessebeach commented 14 June 2013 at 16:41

/**
  * Provide a series of commands that the server can request the client perform.
  */
-Drupal.ajax.prototype.commands = {
+Drupal.AjaxCommands = function () {};
+Drupal.AjaxCommands.prototype = {

That is so much more sane!