Prevent duplicate code by adding an AJAX command to invoke simple jQuery methods

subscribe

subscribe

I'm ok with this patch. Code looks good, should be ready to fly.

Is it time to just add an ajax_command_invoke() with a 'method' parameter? Why only 'css', 'attr', 'addClass', and 'removeClass'? Not changing the RTBC status of this though: in case there isn't sufficient motivation to add an 'invoke', the ones added in this patch have a clear use-case.

Pretend someone coming into this issue, such as, say, your friendly neighbourhood core committer, had no idea what the heck a attr(), addClass(), removeClass() AJAX method was or what you do with them.

Pretend further that said core committer was trying really hard to keep developers focused on critical issues and not in further polishing up APIs so we can maybe, someday, release Drupal 7.

Can you explain to said core committer A) why this patch is important, B) why it should not be kicked to D8, since it's a feature request, an API change, and doesn't fix a critical issue?

Is there a good before/after code case you could cite, for example?

ajax_attr_class.patch queued for re-testing.

Dear friendly neighbourhood core committer,

I was hoping to write you earlier, so I hope you don't mind me writing you now. I had plenty of things to do just recently and totally lost time to care for my babies. It apparently seems like you've been in a very similar situation, since after the lovely letter you sent me last, I was waiting for some sort of a sign from you after writing this one. Of course, I'm not as important as other obligations, but it would still be nice to hear from you from time to time, I'd really love to!
Your suggestion to clean and polish up our baby is just brilliant! As we've been doing this for quite some time now already, I was hoping that we would see the same dirt and mud in the meantime. Of course you may disagree, but I really believe that also overhauling and making the fingernails does make a difference to others. Normally, I would have simply gone ahead and quickly done it, but it's also perfectly fine if you want us to talk about and align on the issue first. After all, it's our baby.
Let's just make sure that we don't take too much time, please, as I think that people are expecting our baby already and I wouldn't want that someone else sees our baby in this way.

Love,
sun :)

@sun, I tried to read your post carefullly, but my impression is that it's either hostile or misdirecting or both. I know that language differences might affect this, and the attempt at continuing the dialog in the same tone might have failed. But please be clear and direct and let's try to get issues like this resolved one way or another. @webchick made a gentle, clear request for a specific argument.

What we need here is:
1. An issue summary
2. Use cases that justify this API change
3. A further defense of this for D7.

Yeah, I don't understand #9. Sun, if you and I need to talk, I'm on IRC every hour that I'm awake. You know how to find me.

Thanks to Randy for the bullet list of what's needed in this issue.

Seriously, my message didn't come through. Sorry for that.

If there is a concrete problem with the code in this patch, then I'm happy to discuss. To my knowledge, CTools is the only module that registers any AJAX commands, but those commands are much more advanced than this simple wrapper.

This simple wrapper? Yes, eff was right that we can implement this much simpler, and at the same time, much more flexible. Unlike the other existing, special AJAX commands, there is no use-case to individually override simple jQuery methods. If at all, you'd probably override the jQuery method itself (instead of Drupal's ajax command), like jQuery UI does, if I'm not mistaken.

The reason why it's worth to add this command is that the AJAX framework does not support a wildcard command, i.e., every single command you want to execute needs to be registered first -- although most of the commands just simply look like this:

  commandName: function (ajax, response, status) {
    $(response.selector)[response.foo](response.bar);
  }

But as mentioned before, there is no "pass-through" command, which means that for every single AJAX command, there needs to be a dedicated PHP function (like in includes/ajax.inc) and a corresponding JS method (liek in misc/ajax.js).

Thus, for the most simple commands, which actually are jQuery methods only, we will very likely see module Foo registering

foo.module:
function foo_ajax_command_simpleCommand($selector, $argument) {
  return array(
    'command' => 'simpleCommand',
    'selector' => $selector,
    'argument' => $argument,
  );
}

foo.js:
Drupal.ajax.prototype.commands.simpleCommand = function (ajax, response, status) {
  $(response.selector).simpleCommand(response.argument);
};

and we will see module Bar registering the same, because it doesn't know of module Foo or has otherwise nothing to do with it:

bar.module:
function bar_ajax_command_simpleCommand($selector, $argument) {
  return array(
    'command' => 'simpleCommand',
    'selector' => $selector,
    'argument' => $argument,
  );
}

bar.js:
Drupal.ajax.prototype.commands.simpleCommand = function (ajax, response, status) {
  $(response.selector).simpleCommand(response.argument);
};

Looking closely at both functions, there's absolutely no difference. However, without this patch, both modules need to implement that code.

Therefore, this patch avoids 100% code duplication in contributed modules, by allowing to invoke simple jQuery methods through a dedicated AJAX 'invoke' command. Technically, this invoke command can invoke any function that's available on the jQuery object.

Effectively, #12 is the same patch as before, just less code but more flexible. Also, #12 properly "defends" this patch for D7. RTBC, anyone?

#12: drupal.ajax-command-invoke.12.patch queued for re-testing.

#12: drupal.ajax-command-invoke.12.patch queued for re-testing.

Just because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.

Sounds like D8 material to me.

#12: drupal.ajax-command-invoke.12.patch queued for re-testing.

This is a very simple but very useful feature request. I would support it for D7.

That said:

+ invoke: function (ajax, response, status) {
+ $(response.selector)[response.method](response.argument);
+ },

..; any reason this is not to be: var element = $(response.selector); element[response.method].apply(element, response.arguments); and as a consequence support a variable number of arguments?

Re-rolled against HEAD.

Cross-post.

.apply() would require to always pass an array of arguments, no?

.apply() would require to always pass an array of arguments, no?

Yep, we would change 'argument' into 'arguments'.

Nice. Took me a couple of iterations and manual tests to understand 'this' in the context of a jQuery method applied on a jQuery object.

Attached patch changes argument to arguments, and expects that to be an array. Works.

To manually test this, drush -y en ajax_forms_test

Forgot to update the test accordingly.

#28: drupal.ajax-command-invoke.28.patch queued for re-testing.

Back to RTBC.

sub

.

Revisiting this again.

Just as was true in #7, it's generally too late for new stuff of this nature. However, in #12 sun has done a good job of outlining the consequences of not having such an API in core, and in this respect it's quite similar to #942310: Field form cannot be attached more than once and #830704: Entity forms cannot be properly extended which did pass my "pre-RC sniff test."

I'd love to see someone other than sun review this one last time, though, because he's effectively marked his own patch RTBC.

One more final confirmation, anyone?

Makes complete sense to me. Here's some nits, but they don't affect the functionality, and can be done in a follow-up, so RTBC.

+++ includes/ajax.inc	6 Nov 2010 22:28:47 -0000
@@ -1088,6 +1088,37 @@ function ajax_command_data($selector, $n
+ * method with the supplied argument on the elements matched by the given

s/argument/arguments/

+++ includes/ajax.inc	6 Nov 2010 22:28:47 -0000
@@ -1088,6 +1088,37 @@ function ajax_command_data($selector, $n
+ * selector. Should be used for simple jQuery commands only, such as attr(),
+ * addClass(), removeClass(), toggleClass(), etc.

Why is this restricted to simple jQuery commands only?

+++ misc/ajax.js	6 Nov 2010 22:35:11 -0000
@@ -487,6 +487,14 @@ Drupal.ajax.prototype.commands = {
+    $element[response.method].apply($element, response['arguments']);

Why not response.arguments?

Powered by Dreditor.

I took a tour around the AJAX Example with this patch applied and found no breakage. As far as I can tell, this should only be a minor API addition. It will save a lot of pain for simple jquery usage in D7.

+1

Can we get a quick reroll based on the feedback in #36? Thanks.

Sorry, I was mistaken in that "arguments" would be a reserved word, but it's not. Evidence:

https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words
http://en.wikibooks.org/wiki/JavaScript/Reserved_Words

Also fixed the other two comments.

Ok. Committed #40 to HEAD. Thanks!

I'll take this on and add it to the Examples project's AJAX commands and try to get AJAX commands more generally understood.