The Ajax framework was originally intended to be used with FAPI. It was later extended to make it possible to use it with links or other elements.

The current framework is not flexible enough and the code is more complicated than it needs to be. It is also unclear how to use it from the frontend.

Some practical goals (bold are what is addressed by the patch, the other are kept for historical purpose and putting initial comments in context):

  1. try to simplify and merge certain portion of the code,
  2. find a better place than Drupal.ajax to keep created ajax objects,
  3. Lay down the API and clean it up,
  4. check the implementation of each method of the API,
  5. Validate the way we're dealing with ajax commands on the JS side,
  6. BONUS: Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events

This is a JS issue, no changes on the PHP side of this framework.

New API

There is no need to have a dummy DOM element to trigger an ajax request. The ajax object has a new 'execute' method to programatically trigger an ajax request that uses the Drupal API.

var ajaxObject = Drupal.ajax({url: 'some/url.json'});
ajaxObject.execute();

// All created ajax objects are stored in the Drupal.ajax.instances array. 

Before

new Drupal.ajax('ckeditor-dialog', $content.find('a').get(0), {
  /* ajax settings */
  event: 'ckeditor-internal.ckeditor'
});

$content.find('a')
  .on('click', function () { return false; })
  .trigger('ckeditor-internal.ckeditor');

After

var ckeditorAjaxDialog = Drupal.ajax({/* ajax settings */});
ckeditorAjaxDialog.execute();

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Major because it's very hard to use the Drupal ajax API from the frontend, making it useless.
Prioritized changes The main goal of this issue is developer experience, coding standards conformity, and code clean-up.
Disruption Disruptive for core/contributed and custom modules/themes because it require an API break and a slight internal code refactoring.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

litwol’s picture

To whomever going to tackle this,
see this module (it is in the patch in linked issue) that makes use of various ajax initializers from php side to trigger front end ajax behaviors: http://drupal.org/node/1473314#comment-5871142 . Useful for testing during ajax.js refactoring. Good luck.

Ralt’s picture

I may have missed something, but it looks like there is not many functions in ajax.js (11, to be exact). And except minor issues, I haven't found anything needing a complete rewrite.

What is there that I completely missed?

Wim Leers’s picture

I encountered a use case for Drupal.ajax that is not well supported by Drupal.ajax in the Edit module (part of Spark).

Copy/pasted from a chat:

For Edit, I'm currently loading a small form that has just the subform for the field being edited. I do this through a custom #ajax command, which I let then call the insert command, so that it can handle all the behavior attaching and effects stuff. I load my custom AJAX command through Drupal.ajax[some id] = new Drupal.ajax(…) — i.e. the usual pattern.

No problems until here. Even the "add more" button in this form works.

Now there's the case of *removing* this inline form, however… AFAICT there's no clean-up mechanism for Drupal.ajax? Hence, Drupal.ajax[some id] continues to exist. So, whenever I'd close the inline form and load it again, the previous Drupal.ajax would still be there, plus the previous event binding to trigger the AJAX call would also still be there. My current hacks that partially solves this look like this:

   var element_settings = {
     url: url,
     event: 'edit-internal',
     wrapper: 'wat-een-bucht',
   };
   if (Drupal.ajax.hasOwnProperty(edit_id)) {
     delete Drupal.ajax[edit_id];
     $editable.unbind('edit-internal');
   }
   Drupal.ajax[edit_id] = new Drupal.ajax(edit_id, $editable, element_settings); 
   $editable.trigger('edit-internal');

Surely there has to be a better way to deal with this?

On top of that, there doesn't seem to be a way to cleanly remove e.g. the Drupal.ajax[…] entry for the "add more" button?

So:

  1. Yes, we very much need this to be a proper JS API, so that modules doing AJAX callbacks can actually use Drupal.ajax. (Why not use jQuery.ajax? Because then you still need to deal with calling the right AJAX commands, animations, etc. — Drupal.ajax does this for you.)
    It also needs to be less "wrapper/id"-centric: maybe you want to use Drupal.ajax just to retrieve custom AJAX commands, for which you don't actually need to specify a wrapper that's going to be replaced/deleted/appended to/whatnot.
  2. The fact that there is *no* clean-up is very bad. This is fine if you only have "fixed" AJAXy DOM elements, but not if you have "dynamic" (as in, they may disappear) AJAXy DOM elements. The latter is the case for me: I need to use AJAX to load forms and those forms may have AJAX too, but those forms may be removed from the DOM after a few seconds.
effulgentsia’s picture

I'm happy to help on the PHP side (if anything is needed there) and with patch reviews, but need nod_, Wim, or another JS developer to lead the effort here on the JS side.

effulgentsia’s picture

Issue tags: +WSCCI

Adding WSCCI tag, because to the extent we refactor things here, let's also use it as an opportunity to follow proper REST standards. I don't know yet what that means in detail, but will add links/comments here as I learn, and maybe other WSCCI developers will as well.

nod_’s picture

Issue tags: -WSCCI

Yep, I'm still thinking about it. We should have a JS sprint/meeting in a month or so where this will come up. Hopefully we'll have a plan after that.

nod_’s picture

Issue tags: +WSCCI

oups, tag

sun’s picture

One potentially major problem I see for this refactoring:

While I believe we have at least some basic test coverage for the fundamental Ajax framework functionality, AFAIK that does not cover all details of the expected functionality, and I also think we do not have many tests for the individual commands. The original patches that introduced the Ajax framework in D7, as well as various follow-ups that fixed regressions and added some tests, never added a complete suite of test coverage.

Technically, refactoring a subsystem can be done relatively easily, if there's a decent amount of test coverage, since that ensures that no expected or required detail in functionality is lost. Without that, very detailed reviews and careful coding/collaboration will be required.

Obviously, we never had frontend/JS tests, so I'm mainly talking about the PHP interaction layer here, for which we extended Simpletest with drupalGetAjax() and drupalPostAjax() methods, which apparently parse the Drupal.settings in the page output and resemble the JS Ajax commands in PHP.

nod_’s picture

If that can make people feel better I've been all over the AJAX framework as well as pretty deep with ctools ajax JS code, especially the modal commands. I know how it work and I have time to spend on it, that should help things during the refactor.

The ultimate regression tests will come from VDC I guess. But you're right, it might be a good time to go over AJAX testing and see how it can be made better as well, should that be in an other issue ?

attiks’s picture

Regarding the testing part, if you want to use/extend the testswarm module, I can try to free up some time to help.

nod_’s picture

Crell’s picture

Just spotted this via a tag. I cannot speak to the JS side, but from a REST perspective, IMO this should be a custom mime type.

That is, the browser sends an XHR request with mime type application/vnd.drupal-ajax (or something like that; I forget the proper format off hand) to /system/block/$block_id/$node/$user/$whatever. That in turn maps to a controller (nee page callback) that returns a Response object. But not just any response object. It returns a subclass of JsonResponse (probably; maybe just Response, I don't know). That subclass has extra methods on it for us to use that correspond to the various array elements of the current ajax return array. That lets us focus on making that Response class have a really really nice API, and separate out the concerns of the internal data structure (which could be very similar to what we have now, potentially).

Then on send(), that object takes whatever data has been passed to it and parses that down into an appropriate JSON string, then sends that. That JSON string could potentially be exactly the same as what we have now, or it could get cleaned up. I have no preference there.

At least that's how I envisioned it working. Someone else feel free to suggest an alternative, but that's my current thinking there.

nod_’s picture

The PHP side sounds lovely like that. I'm not sure we have much to clean-up in the json data that is passed around.

The PHP side definitely needs it's own issue too :)

Wim Leers’s picture

Crell++ :)

Steven Jones’s picture

Following on from #8 do we need to come up with a list of things we need to test, and get those into Drupal 8 first, and then tackle the refactor?

klonos’s picture

Crell’s picture

I'm inclined to say "write the new system, unit test it properly, and throw out the old tests". :-)

kristiaanvandeneynde’s picture

I'm willing to try and tackle (parts of) this issue.
If I get this correctly, a complete rewrite on both back and front end is a viable option?

In that case:

  • Are there features that currently are in Drupal AJAX that we would like to see removed?
  • Are there features that currently are in Drupal AJAX that we would like to see improved?
  • Are there features that currently aren't in Drupal AJAX that we would like to see implemented?
Crell’s picture

I am not an expert, but I think we do want to add history/pushstate support, per: http://groups.drupal.org/node/247073#comment-801003

Definitely we want to improve the error handling, which feels funky at times now.

The internal API is also something we want to improve, which the custom response object should help with by providing real methods rather than anonymous arrays.

Anyone else?

Steven Jones’s picture

Seems really short-sighted to implement each of the current arrays as a method on a response object, if you go that way then how do I extend the ajax commands with my own? Would it be better to build up a list of objects that represent the commands if we're going to perform? Then I can extend by just creating a new subclass, and adding that to the response object somehow.

You can the current 'API' for the PHP side here:
http://api.drupal.org/api/search/7/ajax_command

Also, it's currently practically impossible to use the AJAX framework with GET requests, #956186: Allow AJAX to use GET requests, because of the amount of data being unnecessarily sent by the server. It's not abnormal for the requests to be significantly bigger than the responses at the moment. Ideally we'd get that cleaned up, even if by default most of Drupal core's AJAX request used POST.

nod_’s picture

kristiaanvandeneynde’s picture

From all the links posted here so far I've gathered that the new AJAX implementation should:

  • be easier to understand and use, perhaps a complete rework of the back end
  • iron out the kinks on the front end
  • be extensible on the front end, i.e. fire more events (or use the JS observer pattern)
  • limit sent data to the bare necessities so GET requests are possible
  • be able to style this on the front end, "the D8 way" (think SCOTCH, see #1499460-250: [meta] New theme system)

I've also noted a wish for PJAX, i.e. using the HTML5 History API.
This might not be very practical in combination with forms, though.

Crell’s picture

Re #22, that seems like a good target list.

Re #20, a more robust framework-esque approach to shuffling commands forward seems sensible, but I worry about complexity and time to implement. Do you have an idea for how that can be done expediently without introducing several additional layers of complication (both for implementers and for people using the API)?

Crell’s picture

Issue summary: View changes

details

Kiphaas7’s picture

Updated the issue summary (see 'bonus').

Steven Jones’s picture

So what we have at the moment in AJAX commands is not completely dire, in the case of implementing your own menu callback and returning an AJAX response you'd do something like this in Drupal 7:

function my_module_ajax_callback() {
  // An array of commands to return to the client.
  $commands = array();

  // An annoying popup alert.
  $commands[] = ajax_command_alert(t('This is an annoying popup.'));

  // Replace a div with something else.
  $commands[] = ajax_command_replace('div#my-div', 'something else');

  // Return a render array for ajax_deliver()
  return array('#type' => 'ajax', '#commands' => $commands);
}

But it should be noted that most of the time when dealing with AJAX most people will probably be using forms, and actually you don't really have to do anything different at all then, and you don't have to care about the commands really.

I guess that in an ideal object orientated world then we'd replace the methods with objects, and define an interface something like this (and yes, this would need to be namespaced):


interface DrupalAJAXCommandInterface {
  /**
   * Return an array that will be run through json_encode and sent to the client.
   */
  public function render();

}

Then the above callback would become more like:

function my_module_ajax_callback() {
  $response = new DrupalAJAXResponse();

  // An annoying popup alert.
  $response->addCommand(new DrupalAJAXCommandAlert(t('This is an annoying popup.')));

  // Replace a div with something else.
  $response->addCommand(new DrupalAJAXCommandReplace('div#my-div', 'something else'));

  // Return a render array for ajax_deliver()
  return $response;
}

At some later point in the chain, the response is rendered and renders each individual command passed to it, as it does now.

Not sure if that really makes anything clearer, and at some point we still have to render our command down to an array suitable for converting to JSON.

We really should get some feedback from the people originally involved in the AJAX framework, which I think came from ctools?

Crell’s picture

My initial thinking was to have a fixed list of commands with known methods for each, which is therefore super easy to document and for IDEs to auto-hint. However, I like the idea, in general, of making it extensible with command objects. I believe that's still an improvement over the current status quo, because we can encapsulate cases where we're doing more than just throwing a string into an array. It's also still more self-documenting than an anonymous array is. (I couldn't actually find any documentation in core for the various commands the last time I was looking; as command objects, the docblocks are entirely self-evident.) It also makes it more obvious that you can (presumably) extend it with your own commands. (We'd need to figure out how the client side of that works, of course. Maybe go all hypermedia on it's arse and send the necessary command JS along with the response? Or rather, have no client-side code waiting for it at all, and have the command object just compile down into the code to run on its own, thank you very much, have a nice day.)

Steven Jones’s picture

As I previously hinted at the current documentation for the ajax commands are here: http://api.drupal.org/api/search/7/ajax_command

Crell’s picture

Wow, there's more than I thought! Which is itself indicative of something. ;-)

Since we want to OOPify that code anyway so that it can get taken out of the bootstrap process, we should probably get started here. Who wants to take a first crack at it? (Assign to yourself and get cracking!)

kristiaanvandeneynde’s picture

I'm still interested in trying to tackle this, but I'm currently tied up in (paid) work.
So if anyone with more time on his hands wants to kick things off already, don't let #18 stop you.

Crell’s picture

In today's WSCCI meeting, cosmicdreams said he would dig in on this issue to get it bootstrapped.

Wim Leers’s picture

Sounds like this issue is moving forward :) Thanks to all of you!

mkadin’s picture

I'd be willing to help. I'm very familiar with Drupal's AJAX API, though I'm not quite as familiar with all of the new WSSCI changes. Has there been any bootstrapping of this yet?

cosmicdreams’s picture

Yes please help move this one forward. I haven't been able to do so.

mkadin’s picture

Seems to me (who has admittedly very little context in these discussions) like this #1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply might need some doing before the AJAX stuff can be worked on no?

Crell’s picture

I don't think it's related. VIEW listeners are only called if what the controller returns is not a Response object. If it is a Response object, they are not even called.

If the AjaxResponse class described in #25 is a subclass of Response (or, probably, JsonResponse), then it would do its ow self-compiling in its send() method. None of the View listeners would get called, so ViewSubscriber (which is just a collection of View listeners) would be ignored.

Thank you for stepping up, mkadin! If you have questions, pop into #drupal-wscci in IRC and we'll try to help you out.

RobLoach’s picture

Crell’s picture

I don't know backbone well enough to have a firm opinion, other than to ask "do we really need something that big for a task this small?" It feels like overkill.

mkadin’s picture

OK i've been reading up in an attempt to get this started. If we want to follow through with #12...what are the first steps?

Am I right in saying that we need to set up routing for a custom mime-type to a controller that is specific for our ajax work? If so, I can't seem to figure out how the mime-type routing is implemented.

nod_’s picture

This one is the JS issue, it needs renaming or taking the PHP side of things in an other issue. I'm happy with the json that's returned for JS to take care of things, I don't have an opinion on the PHP side of this thing but this issue was meant to rewrite the handling and interraction with the ajax framework from the JS side.

Please rename this issue as a meta with an issue summary or open a new one PHP-specific. Thanks.

Crell’s picture

mkadin: The mime-based routing doesn't exist yet, so for now just have normal controllers that return a Response object, which should skip any of the existing mime-sensitive logic.

I don't much care what issue it's in, so go ahead and spin up a new one if you want. :-)

mkadin’s picture

#1812866: Rebuild the server side AJAX API is a new issue for rebuilding the PHP end of things.

Crell’s picture

#1812866: Rebuild the server side AJAX API just landed. Yay!

A thought I had recently: Currently we've a lot of code on the client side to take in a bunch of array data from PHP, parse it, map it out to a series of Ajax functions that we are then loading whether it's needed or not (not on every page, but even if you're just using replace you need the whole file), etc. And it's still fairly inscrutable.

What if instead we stripped the API down to bare-bones, and allowed the AjaxCommand object to generate not an array for Drupal.settings but the actual Javascript to execute? So an "insert before" command simply turns into rendering out the one-line jQuery command to run an insert(), which in the browser just gets eval()ed (or whatever the safe alternative is). PHP generating Javascript is not an issue, and this way the browser needs barely any code in it, and processing the commands that come back is a for/eval loop, basically. It also makes the code you have to actually read much much simpler, and you don't need to worry about loading ajax.js. Its replacement gets thin enough that we don't need to break it out to a separate file. Writing a new command gets easier, too, because everything is in one place.

Thoughts?

pounard’s picture

What you're suggesting is a big eval() on server side generated code?

Wim Leers’s picture

#42: I have a hard time imagining what that would look like exactly — either your explanation isn't sufficiently clear, or I'm not getting it (very possible — I'm tired). To be honest, it sounds like a JS developer nightmare. It sounds like you intend to move *all* (or as much as possible) logic to the server side. That's already what Drupal.ajax is about today. We need to move it the other way: Drupal.ajax should become easier to use for JS developers.

Crell’s picture

Wim: Well, right now we do all the JS client side, but control it all server side. That makes both sides gross.

Currently, AlertCommand (which I use as an example because it's the simplest of our available commands) generates a PHP array, which then gets JSON-ified. There's then Javascript code on the client-side (and a non-trivial amount of it for some commands) that takes that array and translates it into an alert() call. Most of the commands are also jQuery-dependent.

I'm suggesting instead that AlertCommand look like this:

class AlertCommand implements CommandInterface {
  public function render() {
    return "alert(" . $this->message . ");";
  }
}

And then the client side is dead-simple code to eval() that. (Again, I don't know client-side standards these days well enough to know if eval() is still as legit client-side as it used to be, or if there's a preferred approach.) Then, writing a server-sent JS command (which you no matter what need server-side code for) consists of "one PHP class in one place that can do whatever it wants", rather than "one PHP class in one place and a Javascript function in another place that you have to make sure is already loaded on the client just in case it may get used, communicating back and forth via arrays that in general seem to presume jQuery although it's not clear if that's a hard requirement". It's probably faster, too.

Drupal.ajax becomes easier for JS devs in that it goes away, and they don't need to try to balance JS and PHP together and keep it all lined up. It's all packaged up in one PHP class; and that doesn't introduce a "need to know PHP" requirement, because to write a new Drupal.ajax command you already have to know PHP, because, well, that's what that system is: Server-side actions that respond with client-side actions.

nod_’s picture

We can't eval() JS returned from the server like that. Alert is an easy example but it's an example nobody uses. Try having the insert command JS on the PHP side. We use JSON to do the transport and that's great, that's what it's made for. The jQuery dependency is just something that needs to be taken out of commands that don't need it. I agree that we might want to split commands into their own file, and have each command depend on a JS file implementing the command but JS on the PHP side, not going to happen.

The refactor is not about commands which are a good concept with a sensible implementation, it's about the mess that is Drupal.ajax, how to make an Ajax call that is using Drupal commands, calling attachBehaviors when it needs to, etc. Currently you need to have a DOM object and a DOM event to trigger a call. We shouldn't need either to call anything from contrib (or core).

kristiaanvandeneynde’s picture

Although it sounds like a good and clean(er) idea at first, passing server generated code into eval() can quickly become a nightmare, both implementation and security wise.

Keep in mind that not all PHP devs know what can and can't be done safely on the frond end. Same goes for JS devs and back end code. I'm afraid we'll unwillingly open the gates for XSS attacks.

Crell’s picture

How is that any worse than it is now, where writing a new Ajax command requires writing both PHP code and JS code, just in separate files and in more abstracted and indirect form?

effulgentsia’s picture

Title: Refactor Drupal.ajax » Simplify and optimize Drupal.ajax() instantiation and implementation

So an "insert before" command simply turns into rendering out the one-line jQuery command to run an insert()

The part of this that I agree with is that half of our server-side commands reduce to an 'insert', and I'd be in favor of removing them, and making PHP code just use the Insert command and pass the desired 'method'. Then again, now that we have these commands as Drupal\Core\Ajax\* classes, if we want to leave the specific InsertCommand subclasses around, but maybe change their render() implementation to call parent::render(), or remove the render() override and instead give them a constructor that sets $this->method, we could do that too.

We also have the 'invoke' command for simple 1-line jQuery method invocations, and if we're willing to keep it, then we can drop the 'css' and 'data' commands, unless in #1842576: Review AJAX system commands, decouple from jQuery we decide that we want those commands to be available without jQuery.

What we then have left is a pretty small set of commands: insert, remove, changed, invoke, add_css, settings, restripe, and alert. We can remove 'restripe' if we get #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors in. I think we might want to replace 'alert' with some more generic way of dealing with errors, warnings, and notices. The remaining commands, though, seem like the right "API" for server-side code to communicate to the client, and I don't think it makes sense to reduce it further. However, we may want to look at other AJAXy frameworks like Backbone to see if they've come up with better or more standard APIs we'd rather model.

Anyway, I agree with #46 that rethinking the command API should happen in #1842576: Review AJAX system commands, decouple from jQuery, and that this issue should focus on the other details of Drupal.ajax(), so retitling this issue. Please feel free to further improve the title as it becomes clearer on what exactly should be done.

sun’s picture

Title: Simplify and optimize Drupal.ajax() instantiation and implementation » Refactor Drupal.ajax

I agree with the aforementioned arguments, writing actual JS in PHP doesn't sound like a good idea.

We already introduced the invoke() Ajax command, which essentially is a catch-all command to apply any kind of jQuery method on the jQuery object of matched elements, and which can be used for many jQuery plugins without having to write a custom/specialized Ajax command on the PHP side nor the JS side.

The main differentiation however is that the Ajax commands only transfer data to be used as function arguments in JavaScript. They don't attempt to be the actual, executed JS code. (Aforementioned invoke() command borderline crosses that line, but as mentioned, it has a clear purpose and allows one to do really neat things.)

I guess that @merlinofchaos had very good additional reasons for designing the original Ajax framework with this separation, and I could very well imagine that he tried a pure PHP-driven solution first. It would probably make sense to ask him for his experience and reasoning as the original architect of the Ajax framework before discussing this further.

sun’s picture

Title: Refactor Drupal.ajax » Simplify and optimize Drupal.ajax() instantiation and implementation
nod_’s picture

Issue tags: -WSCCI

The PHP issue was split off, tag doesn't make sense for the new scope.

merlinofchaos’s picture

More or less I agree with nod.

Changing how the commands are processed isn't really going to benefit, I don't think; fixing up Drupal.ajax to be simpler and easier to deal with would be important. Drupal.ajax is a merger of my original AJAX code from CTools Drupal 6, which didn't have that kind of a construct, and ahah.js from Drupal 6 core, which did.

merlinofchaos’s picture

To elaborate, these are my reasons for the separation:

1) I don't really like eval() to begin with, and didn't feel confident I understood the security implications. Whereas, a simplified "macro language" which is what I think of, is much safer in general.
2) The AJAX commands, at least the non-trivial ones, can need context, so if you're writing complex code, you're more or less going to have to do what is done now anyway, only you'll have to eval something that calls to your object. This would likely be more brittle than what we have now.
3) Debugging eval() processed from JSON is much harder than debugging Drupal.AJAX.command.foo with input data.

nod_’s picture

from #1810934: Adding custom ajax progress indicators more easily

Maybe we could improve the way(s) developers can implement custom progress indicators.

What do you think about the following code which is a refactored version of the current 8.x-dev code:

Drupal.ajax.prototype.beforeSend = function (xmlhttprequest, options) {
  // ...
  // Insert progress indicator
  var progressIndicatorMethod = 'setProgressIndicator' + this.progress.type.slice(0,1).toUpperCase() + this.progress.type.slice(1).toLowerCase();
  if (this[progressIndicatorMethod] && this[progressIndicatorMethod].call) {
    this[progressIndicatorMethod].call(this);
    $(this.element).after(this.progress.element);
  }
};

So we simply factor out the method for the existing progress indicators, having the side effect of allowing custom progress indicators:

Drupal.ajax.prototype.setProgressIndicatorProgressbar = function() {
  var progressBar = new Drupal.progressBar('ajax-progress-' + this.element.id, eval(this.progress.update_callback), this.progress.method, eval(this.progress.error_callback));
  if (this.progress.message) {
    progressBar.setProgress(-1, this.progress.message);
  }
  if (this.progress.url) {
    progressBar.startMonitoring(this.progress.url, this.progress.interval || 1500);
  }
  this.progress.element = $(progressBar.element).addClass('ajax-progress ajax-progress-bar');
  this.progress.object = progressBar;
};
Drupal.ajax.prototype.setProgressIndicatorThrobber = function() {
  this.progress.element = $('<div class="ajax-progress ajax-progress-throbber"><div class="throbber">&nbsp;</div></div>');
  if (this.progress.message) {
    $('.throbber', this.progress.element).after('<div class="message">' + this.progress.message + '</div>');
  }
  $(this.element).after(this.progress.element);
};

This way developers can use their custom progress indicators simply by using:

  $form = array(
    'element' => array(
      '#type' => 'textfield',
      '#ajax' => array(
        'callback' => 'mymodule_ajax_callback',
        'wrapper' => 'ajax-wrapper',
        'progress' => array(
          'type' => 'custom'
          // Additional custom options
        ),
      ),
    ),
  );

And then implementing:

Drupal.ajax.prototype.setProgressIndicatorCustom = function() {
  // Setting this.progress.element is the only necessary thing to happen in here.
  this.progress.element = $('<div class="ajax-progress ajax-progress-custom">indicator</div>');
};
nod_’s picture

Priority: Normal » Major

I know it's past API freeze and all but we can't make thousands of developers miserable because of Drupal.ajax in D8 cycle. Raising priority.

nod_’s picture

Issue summary: View changes

detach/events bonus point

Jelle_S’s picture

Status: Active » Needs review
FileSize
8.8 KB

I've done some really, really small stuff, just to get the discussion going again and maybe get some actual coding done.

  • All ajax objects have moved from Drupal.ajax to Drupal.ajaxInstances.
  • A Drupal ajax request does not have to have an HTML element. It is now possible to do:
      var myAjax = Drupal.ajax(null, null, 'my-url');
      myAjax.execute();
  • execute method was added.
  • Progress indicators are implemented as described in #55

I've read the discussion on being able to do GET requests. I've not implemented it yet as I'm not sure what the implications would be for the html ids and css and js files (ajax_page_state)

Jelle_S’s picture

Wim Leers’s picture

Jelle_S++

Jelle_S’s picture

Thanks! Any other suggestions/improvements? I'll be happy to work on this if and when I have the time to do so :-D

joelpittet’s picture

This looks great @Jelle_S.
Drupal.ajax(null, null, 'my-url'); It would be nice if the arguments were reversed, but that would be an API change, though I like this feature regardless!

@nod_ what do you think of #58. Can we use it as a jumping off point to move this forward?

Jelle_S’s picture

#62: Yeah I would have loved to do that as well, but I'm not sure what the policy is regarding API-changes and JavaScript... I'm guessing it's the same though.

I'd love to hear more comments/feedback regarding #58. I'm not really sure what else should be added/removed/refactored?

nod_’s picture

Status: Needs review » Needs work

Way overdue RTBC. This is good, we need it to keep our sanity.

Doesn't apply though, when it does, I'll RTBC. Need reroll.

Wim Leers’s picture

Issue tags: +Needs reroll
valthebald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.89 KB

Rerolled patch attached.

2 chunks failed to apply in patch#58:

- Drupal.ajax[element_settings.selector] = new Drupal.ajax(base, this, element_settings);
+ Drupal.ajaxInstances[element_settings.selector] = new Drupal.ajax(base, this, element_settings);

changed to

- Drupal.ajax[element_settings.selector] = new Drupal.ajax(baseUseAjaxSubmit, this, element_settings);
+ Drupal.ajaxInstances[element_settings.selector] = new Drupal.ajax(baseUseAjaxSubmit, this, element_settings);

becaus of the change in constructor parameter name

Jelle_S’s picture

Status: Needs review » Reviewed & tested by the community

Straight reroll of #58. RTBC as per #64.

nod_’s picture

Status: Reviewed & tested by the community » Needs work

Looks good to me, there are a few eslint errors, please get them fixed before I can RTBC. Also a small improvments in the progressbar stuff:

I was going to complain about progressindicator but 2 years ago I put this in scope for this issue :p anyway a little improvment:

// Insert progress indicator
    if (this[progressIndicatorMethod] && this[progressIndicatorMethod].call) {

should be replaced with the more explicit:

if (progressIndicatorMethod in this && typeof this[progressIndicatorMethod] === 'function') {

Wim Leers’s picture

#68: wow, I never even would've known the the current code is equivalent with what you suggest :D

valthebald’s picture

@nod_: can you list eslint errors that you get? I'm sure not everyone uses this tool. Or - if possible - they can be fixed in a followup issue?

nod_’s picture

Everyone working on core JS should be using this tool, it's in our docs : eslint configuration. For information: it can't be fixed in a follow-up, the policy is to not commit code that doesn't lint. We went to the trouble of fixing every last linting error, it's not to introduce new ones :)

For the sake of making this easy here is the error (thought there were more):

core/misc/ajax.js
  470:86  error  A space is required after ','  comma-spacing

✖ 1 problem (1 error, 0 warnings)

Fixing that and the comment in #68 and we're RTBC.

valthebald’s picture

Status: Needs work » Needs review
FileSize
8.91 KB
814 bytes

Fixed spacing and changed check as per #68

nod_’s picture

Status: Needs review » Needs work

Tested it, there is a problem when using quickedit, the progress type is undefined and it crashes the JS.

Need to add some code to check that this.progress is defined before messing with it.

nod_’s picture

Status: Needs work » Needs review
FileSize
536 bytes
8.94 KB

Status: Needs review » Needs work

The last submitted patch, 74: core-simplify-ajax-1533366-74.patch, failed testing.

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Reviewed & tested by the community

testbot gone wild

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs issue summary update, +API addition, +DX (Developer Experience)

This issue is basically about fixing the absolutely horrendous DX of logic triggering AJAX commands, rater than letting the AJAX Framework trigger it itself — therefore this is basically a requirement for making front-end developers doing advanced things in Drupal 8 sites not shoot themselves in the head, like I had to (see #3).


  1. +++ b/core/misc/ajax.js
    @@ -281,6 +284,31 @@
    +   * Execute the ajax request.
    ...
    +    // Do not perform another ajax command if one is already in progress.
    

    s/ajax/AJAX/

  2. +++ b/core/misc/ajax.js
    @@ -281,6 +284,31 @@
    +   * Allows developers to execute an AJAX request manually without specifying
    +   * an event to respond to.
    

    Hallelujah!

    I think it'd be nice if Quick Edit's JS were updated to use this, then there also is an example in core of using this.

dawehner’s picture

I kinda dislike that the patch not only change one thing, but also renames the variable for the instances. This kinda makes it hard to review ...

nod_’s picture

Status: Needs work » Needs review
FileSize
22.63 KB

You won't like this one but Wim will :p

Made some more changes. Most important one is that Drupal.ajax is not a constructor anymore and it's parameters got reordered to something sensible.

Before:

new Drupal.ajax(id, element, ajaxSettings);

After:

Drupal.ajax(ajaxSettings, id, element);

The old Drupal.ajax has been renamed Drupal.Ajax, because constructors get capitalized, it's in the standards. Drupal.ajax throw an error to help things when people refactor.

Drupal.ajax automatically tracks all ajax objects created in Drupal.ajax.instances, that way no need to pollute Drupal.ajax or think about putting the object somewhere everyone can see.

I replaced all the wonky uses of Drupal.ajax in quickedit, editor, ckeditor. Everything works fine for me, views included.

Fabianx’s picture

RTBC + 1. but agree this needs a change record and IS update to explain the changes introduced here.

The split of ajax and ajaxInstances is a real real real win!

When I read code like that a year ago for the first time, I was like: But why ... how ... what?

With splitting the class and the instances this makes it way way way cleaner ...

nod_’s picture

Issue summary: View changes
nod_’s picture

Made a CR to explain changes.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record, -Needs issue summary update
  1. +++ b/core/misc/ajax.js
    @@ -133,19 +119,22 @@
    +   * The ajax request returns an array of commands encoded in JSON, which is
    
    @@ -289,6 +309,30 @@
    +   * Execute the ajax request.
    ...
    +   * Allows developers to execute an AJAX request manually without specifying
    

    Inconsistent capitalization: sometimes "AJAX request", sometimes "ajax request".

    Should use the same capitalization as the serverside code.

  2. +++ b/core/modules/editor/js/editor.formattedTextEditor.js
    @@ -171,9 +171,8 @@
           // Create a Drupal.ajax instance to load the form.
    -      var textLoaderAjax = new Drupal.ajax(fieldID, this.$el, {
    +      var textLoaderAjax = Drupal.ajax({
             url: Drupal.quickedit.util.buildUrl(fieldID, Drupal.url('editor/!entity_type/!id/!field_name/!langcode/!view_mode')),
    -        event: 'editor-internal.editor',
             submit: {nocssjs: true},
             progress: {type: null} // No progress indicator.
           });
    @@ -186,7 +185,7 @@
    
    @@ -186,7 +185,7 @@
     
           // This will ensure our scoped editorGetUntransformedText AJAX command
           // gets called.
    -      this.$el.trigger('editor-internal.editor');
    +      textLoaderAjax.execute();
    

    So much better :)

    But, the progress: {type: null} bit can now also be removed, I think?

  3. +++ b/core/modules/quickedit/js/models/EntityModel.js
    @@ -362,18 +362,11 @@
    -      // @todo Simplify this once https://drupal.org/node/1533366 lands.
    -      // @see https://drupal.org/node/2029999.
    -      var id = 'quickedit-save-entity';
    -      // Create a temporary element to be able to use Drupal.ajax.
    -      var $el = $('#quickedit-entity-toolbar').find('.action-save'); // This is the span element inside the button.
           // Create a Drupal.ajax instance to save the entity.
    -      var entitySaverAjax = new Drupal.ajax(id, $el, {
    +      var entitySaverAjax = Drupal.ajax({
             url: Drupal.url('quickedit/entity/' + entityModel.get('entityID')),
    -        event: 'quickedit-save.quickedit',
             progress: {type: 'none'},
             error: function () {
    -          $el.off('quickedit-save.quickedit');
               // Let the Drupal.quickedit.EntityModel Backbone model's error()=
               // method handle errors.
               options.error.call(entityModel);
    @@ -381,8 +374,6 @@
    
    @@ -381,8 +374,6 @@
           });
           // Entity saved successfully.
           entitySaverAjax.commands.quickeditEntitySaved = function (ajax, response, status) {
    -        // Clean up.
    -        $(ajax.element).off('quickedit-save.quickedit');
             // All fields have been moved from PrivateTempStore to permanent
             // storage, update the "inTempStore" attribute on FieldModels, on the
             // EntityModel and clear EntityModel's "fieldInTempStore" attribute.
    @@ -399,7 +390,7 @@
    
    @@ -399,7 +390,7 @@
           };
           // Trigger the AJAX request, which will will return the
           // quickeditEntitySaved AJAX command to which we then react.
    -      $el.trigger('quickedit-save.quickedit');
    +      entitySaverAjax.execute();
    

    So very much better!

nod_’s picture

Status: Needs work » Needs review
FileSize
24.39 KB
3.15 KB

Changed comments. About the progress type, looks like it could be removed but don't really want to remove it here.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Fabianx’s picture

Looks great now!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/misc/ajax.js
@@ -153,12 +142,41 @@
+  Drupal.ajax = function (settings, base, element) {
...
+  Drupal.Ajax = function (base, element, element_settings) {

Are we not concerned about any confusion between ajax and Ajax. Also should we make the params consistent?

nod_’s picture

About the naming it's to make things easier on people who port modules. If they don't want to change their code they can replace Drupal.ajax with Drupal.Ajax and things will work for them, the API will not be used as I expect it to but it's good enough.

Now for anyone who used the drupal Ajax api and know how hard it is to work with, the benefits of this change will be clear and it shouldn't be an issue to move a few parameters around (because I also want to get to a point where we have some JS documentation at least).

One thing we could do though, is to go jquery-like and have only one parameter that is the whole config object, essentially putting base and element (the two extra parameters) in the config object like the rest of the options. It would make detecting a bad use of the improved Drupal.ajax easier too.

Drupal.ajax({
  base: base,
  element: this,
  url: 'somethign',
  // etc.
});

What do you think?

Fabianx’s picture

Assigned: Unassigned » alexpott

Assigning to alexpott for feedback, but the proposal in #89 looks sensible to me.

nod_’s picture

FileSize
27.14 KB
10.65 KB

With some perspective, I agree it's a good idea :p

Reroll + API change described in #89.

Views, quickedit, node forms are working fine.

Status: Needs review » Needs work

The last submitted patch, 91: core-simplify-ajax-1533366-91.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great. We need a CR update for #89.

nod_’s picture

CR updated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 91: core-simplify-ajax-1533366-91.patch, failed testing.

The last submitted patch, 91: core-simplify-ajax-1533366-91.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
27.1 KB
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think the changes in #91 look great and will indeed make porting much simpler. Looks like they need documenting in the code though. The CR looks good to go.

nod_’s picture

Status: Needs work » Needs review
FileSize
29.11 KB
3.91 KB

Not sure what more to add.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Changes look great!

Wim Leers’s picture

I bow to Théodore, for making this obtuse mess a thousand times less painful to deal with.

nod_’s picture

Bow to Jelle_S first then :)

Wim Leers’s picture

D'oh, stupid me! Of course, I bow to Jelle_S too!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for addressing all my minor nits. This does indeed look like a great clean up. Given the support for this change and how hard it is to use the current implementation I think we should proceed with this change under the maintainer discretion provision in the beta evaluation. The changes necessary to a contrib module will be minor and now the error message will help them.

Committed 4c49e1e and pushed to 8.0.x. Thanks!

  • alexpott committed 4c49e1e on 8.0.x
    Issue #1533366 by nod_, valthebald, Jelle_S, Wim Leers: Simplify and...
nod_’s picture

Thanks!

Wim Leers’s picture

If I think too much about how much pain, misery and lost time I suffered because of the state of Drupal.ajax prior to patch, I may actually end up crying and curling up in a corner.

This makes it much, much more feasible for JS to use Drupal's AJAX system.

Jelle_S’s picture

@Wim haha, thanks! Don't throw your back out with all that bowing ;-)

Glad to see this fixed!

yched’s picture

I totally second @Wim's #111.
Thanks so much @Jelle_S & @nod_

/me throws his hat in the air and makes loud high-pitch noises

Wim Leers’s picture

/me throws his hat in the air and makes loud high-pitch noises

:D

pounard’s picture

loud high-pitch noises

Like seagulls you mean?

yched’s picture

@pounard: right, a yodeling seagull, something like that

Wim Leers’s picture

ROFL

alexpott’s picture

Issue summary: View changes
FileSize
26.6 KB

In homage to french men talking about seagulls...

LewisNyman’s picture

Issue summary: View changes
Status: Fixed » Needs work
FileSize
628.49 KB

This broke the UI of the fullscreen AJAX throbber:

+++ b/core/misc/ajax.js
@@ -462,36 +559,57 @@
+    // Insert progress indicator
+    var progressIndicatorMethod = 'setProgressIndicator' + this.progress.type.slice(0, 1).toUpperCase() + this.progress.type.slice(1).toLowerCase();
+    if (progressIndicatorMethod in this && typeof this[progressIndicatorMethod] === 'function') {
+      this[progressIndicatorMethod].call(this);
       $(this.element).after(this.progress.element);
     }
-    else if (this.progress.type === 'fullscreen') {
-      this.progress.element = $('<div class="ajax-progress ajax-progress-fullscreen">&nbsp;</div>');
-      $('body').after(this.progress.element);

I think this code now inserts the throbber inline even if it a fullscreen throbber. The fullscreen throbber was designed to only be inserted after the body tag to prevent it accidentally inheriting styling in other contexts. See #2207695: Expand the throbber API to include a 'full screen' option

I'm not sure I understand the changes here enough to know if this can be fixed in a followup so setting to needs work.

nod_’s picture

Status: Needs work » Needs review
FileSize
515 bytes

Leftover line. Should have been removed.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Ah nice :) Thanks!

Fabianx’s picture

Please open a new issue. Per some policy (that I can't find right now), regressions need to be either reverted and then a new whole patch uploaded or pushed to a follow-up issue to ensure that all data is correct.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Yep please let's get a followup for #119 and #120. Sorry to be a pain but followups on issue mess with metrics, issue statuses and credit.

joelpittet’s picture

Status: Fixed » Needs work

The last submitted patch, 120: core-simplify-ajax-1533366-120.patch, failed testing.

joelpittet’s picture

Status: Needs work » Fixed
webchick’s picture

Looks like either way, credit gets messed with (nod_ and LewisNyman didn't comment in the spin-off issue, so can't be credited) so just committed/pushed #120 instead.

Thanks!

  • webchick committed 1b355bc on 8.0.x
    Issue #1533366 follow-up by nod_, LewisNyman: Fix broken fullscreen AJAX...
dawehner’s picture

Really nice API improvement!

joelpittet’s picture

There is no need to have a dummy DOM element to trigger an ajax request. The ajax object has a new execute method to programatically trigger an ajax request using Drupal API.

Awesome!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.