Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The design of the ajax object in ajax.js is such that it is meant to be externally modifiable, since you can use Drupal.ajax['id'] to modify the ajax attached to an item.
Unfortunately, the 'options' array which is sent to $.ajax or $.ajaxSubmit is not modifiable, because it is kept as a variable rather than a property on the ajax object. The effect of this is that any ajax options (such as the URL) are set and unchangeable as soon as the javascript is bound.
In CTools, I have a pattern where I use a select box and a button; the button initiates an AJAX operation. Often people use POST parameters to read the select box, but for large forms I had preferred to modify the URL to include data in the select box rather than POSTing the entire form. While I *could* switch everything to using a full POST during porting, I would really rather not.
The modification is simple. Change 'var options' to 'ajax.options' so that it becomes a property of the ajax object, and then a couple of references to it. As near as I can tell this has no effect whatsoever on normal operations, but it does make the options externally modifiable. The use-case I name is not the only one where options may want to be externally modified after the fact, so I think it is good that we can support other use cases for this.
Patch to follow.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 922796-options-not-modifiable.patch | 1.14 KB | merlinofchaos |
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedComment #2
sunWell done. Since this cannot be tested, RTBC.
Comment #3
rfayActually, it should be demonstrated even if it can't be tested by simpletest.
<grumpy_hat_on>Nothing should go into core that hasn't at least had some demonstration code applied to it</grumpy_hat_on>. And shouldn't we at least see that a number of normal use-cases work with this change?Comment #4
sun@rfay: Sorry, but I'm not sure what you want. No such JavaScript documentation or JavaScript example exists in Drupal core. This patch is merely shifting a local variable into a globally accessible context. I'm 99% sure that we'd have to invent something entirely new to fulfill your request, but this is definitely not the right issue to start to invent things.
Comment #5
rfay@sun, we need a couple of things.
1. A simple module or other piece of test code that uses the new style. And yes, it's just test code.
2. Some time poking around at the various kinds of existing AJAX to make sure this doesn't break things. (AJAX forms, AJAX commands, AJAX links...)
I'm just in favor of actually trying things.
Comment #6
sun1. That's http://drupal.org/project/ctools -- I have no idea how an example could look like that remotely makes sense to even think of. This patch is a purely technical, not related to any feature.
2. You can try this patch with every single AJAX example module that exists. There is no and there should be no visible change, except if you look into the active DOM via Firebug, in which you should see Drupal.ajax[ID].options as a technically alterable object.
Comment #7
sun1. That's http://drupal.org/project/ctools -- I have no idea how an example could look like that remotely makes sense to even think of. This patch is a purely technical, not related to any feature.
2. You can try this patch with every single AJAX example module that exists. There is no and there should be no visible change, except if you look into the active DOM via Firebug, in which you should see Drupal.ajax[ID].options as a technically alterable object.
Comment #8
merlinofchaos CreditAttribution: merlinofchaos commentedWell, I do have a working example in CTools, but extracting it out into a standalone demo is a little bit complicated for a 3 line patch. Right now, the code I'm working on succeeds with this patch and fails without it. =)
Comment #10
chrisshattuck CreditAttribution: chrisshattuck commentedSwitching title back. Maybe you should open a new issue for discussing the current state of Anime, bot. :P
Comment #11
sunComment #12
rfayI'm not trying to be obstructionist here, but *I* haven't run through the possibly affected UI to see what's affected (and have seen no other reports), and we don't have any demonstration code or test code.
I'm *way* in favor of getting this into core for ctools. It just needs to have the appropriate work done first.
Comment #13
sunI still have no clue how to move forward here.
Comment #14
merlinofchaos CreditAttribution: merlinofchaos commentedI think we need to build a small example module that demonstrates something like using a select to change the URL that is modified that can become part of the ajax examples suite.
Comment #15
rfayIMO the *simplest* demonstration code of this change working + ONE user reporting that they've tested with this change and everything looks fine == RTBC.
Comment #16
ksenzeeIt is with the greatest hesitation that I disagree with Randy, but I am in favor of committing this without requiring any demonstration. It is much more Drupalish to expose things for outside alteration. JS is great for that because no alter hook is required - all we need is to make variables into object properties, which is totally harmless. And that's just what's happening here. Let's go ahead with it.
Comment #17
webchickI'm of two minds on this.
On the one hand, I share Randy's concern, and it's been extremely disheartening since it seems like every single time we commit one of these patches that touches JS/markup/AJAX/something else that can't be tested directly by testbot, it inevitably ends up with a critical follow-up (or three), and causes us to waste a bunch of time by a bunch of really smart people.
On the other hand, enforcing a rule that people have to submit working demonstration code with their JS patches, while certainly not a bad idea at all, is not a rule we've had the entirety of the D7 release cycle. Major development culture changes like that can really only be introduced during code thaw, IMO. And it sounds like we have a working test, of sorts, in CTools module, albeit not in a nice, well-contained demo testing only this one thing.
So, since this has reviews and +1s from all the right people, and since it blocks the D7 version of Panels, committed #1 to HEAD.
Comment #18
merlinofchaos CreditAttribution: merlinofchaos commentedI do agree that it would be nice to add something that demonstrates this to the examples module. Nonetheless, thank you very much for committing this! With luck when I release Panels/CTools alpha I won't have to say that patches are needed for everything that works to work. =)
Comment #19
rfayAnd it's soooo good to have it in there for CTools, Panels, and Earl! Thanks for all the good work on this.