When I encountered #595654: AJAX command 'settings' broken I wondered how that command was intended to be used.
My first impression when reading the documentation on ajax_command_settings was that it triggered a $.extend(Drupal.settings, response.settings) on the client, which seemed natural as it would allow all modules to rely on the settings being in sync with the actual document.

But when looking in ajax.js, it seems that's not the case at all. All settings passed via an AJAX response are temporary and will be removed as soon as all the commands are dealt with. This makes it a pain to implement modules which rely on the global settings being up to date with the contents.

A specific use case I'm thinking about is the Wysiwyg module and how it needs the settings for each new multi-value field instance available in Drupal.settings.wysiwyg.triggers[field_id]. Obviously, any new AHAH field is not in that list so Wysiwyg fails to attach to it. Wysiwyg, nor any other module which did not actually initiate the AJAX request, knows they were called this way and just drupal_add_js(...) their settings as usual on the server. That's fine since those settings are nicely bundled together and sent as a 'settings' command by ajax_render(). But on the client, the settings are not available where they are expected.

For example, Wysiwyg would currently need to pass the settings argument from the behaviors all the way down to the editor implementations and plugins, and maybe also store a copy of it somewhere, as parts of the module are asynchronous and can no longer rely on the settings staying there.

Adjusting modules to use the new settings argument in the behavior implementations is trivial, but deeper than that it gets nasty. The modules simply have no way of telling if the settings they are passed are persistent or not. Nor can they rely on the global settings from modules they depend upon to be up to date. In worst case, we'll start seeing a myriad of custom solutions for storing the settings and referencing them across modules. If module B relies on module A, but A does not store its AJAX settings, B is probably not able to get them at all.

If the AJAX settings are merged into Drupal.settings, nothing would need to change and all modules could still rely on that single storage location. It would also simplify the way settings are handled both on the server and client. Modules can use drupal_add_js() and rely on it getting into Drupal.settings no matter if it was an AJAX request or not. JavaScript code that does not directly handle AJAX responses need not worry about where to find up to date settings.

While digging into this I found #360081: Pass Settings Locally to JS Behaviors, which is where the idea of having command-specific settings originated.
The original post mentions changing the global Drupal.settings as the AHAH responses come in, but does not explain why it that was a bad idea and not tested.

#544418: Integrate CTools AJAX framework with Drupal to extend (and replace) existing ahah framework expands the framework to full AJAX support, including the 'settings' command getting prepended to all JSON responses (unless it's empty) to keep track of updated settings, though be it temporarily.
But, this code even goes as far as explicitly resetting ajax.settings to an empty object once all commands are processed (in the code it makes sense, but what about design-wise?). I have yet to find a use-case where updating the global settings storage would be terribly wrong, maybe someone can help with that?

I think it would be great to make ajax_command_settings() (or rather it's JavaScript counterpart) actually do what the documentation says! That way we have a standard way of updating Drupal.settings, a way which most modules [indirectly] already use.

In short:
This patch removes ajax.settings in favor of permanently extending Drupal.settings.

This was tested with Wysiwyg module 3.x-dev, TinyMCE, the patch from #595654, and an unlimited-value textarea field. Without this patch, TinyMCE can not be initiated on new field instances. With the patch it does work. Note that the editor contents of all the fields will be reset when adding a new field or rearranging the fields. Those two are unrelated issues.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TwoD’s picture

Tagging...

sun’s picture

$.extend() only extends, never removes. We have to account for the possibility of countless/unlimited AJAX requests.

Wysiwyg's case is somewhat special, because we need our stuff both in the global AND in the local scope. When a form is loaded via AJAX, we still need to add editor instances globally, and we need to remove the instances when either the form is submitted via AJAX or (any form on the page is submitted) without AJAX.

So I think that this is rather an issue of Wysiwyg module.

However, one particular thing I do not understand at all is ajax_command_settings(). The settings contained in there are only copied over to ajax.settings, which means that only following commands would have access to them.

My initial expectation was rather that ajax_command_settings() would invoke drupal_add_js($data, 'setting') to merge those into the settings that are passed anyway... or similar.

TwoD’s picture

It's true that $.extend doesn't remove anything but can't, and shouldn't, we use the detach behaviors for that? If they make sure settings related to removed elements are deleted, there should be no or limited cumulative bad effects.

The documentation for ajax_command_settings spefically says

The 'settings' command instructs the client to extend Drupal.settings with the given array.

so that would be the expected behavior. Though that's not actually what it does now as it completely overrides Drupal.settings, even for code that doesn't know about the AJAX request. But worst of all, the new settings are no longer around once newly attached event handlers are executed, unless every module takes care of that themselves.

IMHO, there would even be no need for the new settings argument in behaviors if the above was accomplished. The argument just adds a new indirection when trying to access the settings. If developers wish to return data which should not go into the global settings object, they can do so in the actual command data. Then other code would not need to care about AJAX operations at all, they'd just work anyway.

If #54418: not security checked had come first, and included a solution to the problem at hand, I'm sure #360081: Pass Settings Locally to JS Behaviors would not have been a problem.

Keep it simple.

sun’s picture

Category: feature » task
Priority: Normal » Critical
Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

katbailey’s picture

subscribing

TwoD’s picture

Quick note: It should be $.extend(true, Drupal.settings, response.settings). Will update patch later tonight.

Sun suggested on IRC that we might need a new command for 'temporary' settings. Then we'd also need to change the ajax_command_settings() description to

The 'settings' command instructs the client to temporarily pass the given array instead of Drupal.settings to behaviors.

. I'm not sure introducing a new command for persistent settings is the way to go though. My main concern is, like I said above, the code which gets called 'indirectly' as a result of an AJAX request. That code has no clue it should use ajax_command_settings_persistent() instead of drupal_add_js(..., 'settings'). That code expects its output to be persistent as it would also run the same way when the document is first generated. Code which actively responds to an AJAX request would also be the one using ajax_command_* , and could then also make use of response.settings on the client.

I'm not so sure extending Drupal.settings over time will be that dangerous. A lot of data in it would be overwritten as hundreds of AJAX responses get in, but is it likely a module would keep adding data there unless the document also grows in a corresponding way? If the module uses a lot of AJAX requests, like a chat client, it wouldn't consider the response/messages a setting, right? Any other module 'caught' in the wrapper area/form updated by the chat client, would respond with the same settings over and over again (along with the same content generated by it). A chat client probably doesn't display the messages in an area which other modules generate content for, but you get the point...

EDIT: Btw, this change would of course prevent clientside code from writing data to Drupal.settings and relying on getting the same data back after an AJAX request. If many modules use it that way, a temporary storage for AJAX settings might be more appropriate, or dynamic 'states' will have to be kept somewhere else...

katbailey’s picture

FileSize
1.46 KB

I'm in agreement with TwoD here - I think any new settings coming from ajax requests should be merged into Drupal.settings. I can't imagine where this would cause a problem; it would certainly be the desired behaviour when you're loading content such as views and quicktabs via ajax - once content like that is loaded into the DOM, you're most likely going to want to keep it there, so you'll need its settings too.

I've re-rolled the patch with the correction to make it a deep copy onto Drupal.settings as per TwoD's note above.

quicksketch’s picture

Priority: Critical » Normal

After reading (most) of this, I wonder why can't the problem stated here:

A specific use case I'm thinking about is the Wysiwyg module and how it needs the settings for each new multi-value field instance available in Drupal.settings.wysiwyg.triggers[field_id].

Why wouldn't you just put $.extend(Drupal.settings.wysiwyg, settings.wysiwyg) in your Behavior.attach()? That seems like it would prevent this problem and it wouldn't have unexpected side-effects of blowing away all the existing Drupal.settings that were there on page load.

katbailey stated:

I can't imagine where this would cause a problem;

yet TwoD just stated an exact use-case that would be broken:

Btw, this change would of course prevent clientside code from writing data to Drupal.settings and relying on getting the same data back after an AJAX request.

I think it makes sense to keep this manual, "automatic" futzing of Drupal.settings worries me. This is not critical, since you can just put $.extend() in your behavior if you need it.

quicksketch’s picture

TwoD reinforces the "manual" argument in #3:

It's true that $.extend doesn't remove anything but can't, and shouldn't, we use the detach behaviors for that? If they make sure settings related to removed elements are deleted, there should be no or limited cumulative bad effects.

So if we're supposed to manually clean up our data on behavior.detach(), why would we automatically extend the data on attach? Sounds just like our documentation for ajax_command_settings needs to be updated to reflect the way it really works.

dmitrig01’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

Looks great, and pretty important to make this command actually work

sun’s picture

Status: Reviewed & tested by the community » Needs review

uhm. Now we have some kind of contradictory perspectives in here.

The more I think about it, the more I tend to agree with TwoD and katbailey. But I'm not 100% sure about the proper way forward. I'll try to get some more people in here.

dmitrig01’s picture

Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

well not that critical (as in, whoops)

merlinofchaos’s picture

I generally agree with quicksketch. If we need our settings permanent, we should be able to handle that in our behaviors. I run into minor futzy problems pretty regularly with Panels and the modal, where the settings just grow and grow and grow and it gets a little worrisome.

merlinofchaos’s picture

Note that I do believe this will change how Views has to do its AJAX. I also think this change will be a marked improvement once it's in. Right now it's quite messy.

merlinofchaos’s picture

I will say that one downside is that its inconsistent:

On initial page load, settings are persistent. On AJAX they are not. What if we instead make settings NOT persistent and require behaviors to cope with the temporary nature of settings? Then we will get consistent behavior.

dmitrig01’s picture

Status: Reviewed & tested by the community » Needs review

I'm convinced. What about splitting this up into ajax_command_temporary_settings and ajax_command_permanent_settings?

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

What if we instead make settings NOT persistent and require behaviors to cope with the temporary nature of settings?

This suggestion is (in theory) what I was trying to accomplish by passing settings in as local variables to behaviors (#360081: Pass Settings Locally to JS Behaviors). Basically using Drupal.settings for everything is very much akin to using a global PHP variable instead of passing parameters into functions.

That said, we don't currently "enforce" that behaviors *only* use the local settings variable they are given. They can still reference/update the global Drupal.settings if they want, even though I'd consider that a bad idea. If a script needs persistent storage, it really should put that into Drupal.myModule instead. I don't feel the need to seriously enforce this by making Drupal.settings temporary, but that would certainly prevent it from being misused.

This patch as it is though just further encourages the misuse of the global Drupal.settings variable.

quicksketch’s picture

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

Consensus? What's the actionable task for D7, i.e. now?

RobLoach’s picture

Usually when adding settings using drupal_add_js, you want the settings to be persistent throughout the user's page lifespan. When using AJAX, you might also want those settings to be persistent, so that you can effect items on the rest of the page too. This leads you to the conclusion that you want to merge the settings together. As discussed earlier though, there are cases where you want the AJAX call to change existing settings.

When I first read through, it seemed like the approach Kat took at #7 seemed great as we're getting all the new settings from the AJAX call, but then there's that use case where sometimes you'd want to completely override the settings rather then merge them. Maybe if we merge the settings, and then have an additional response.overriden_settings or something?.... Not sure.

sun’s picture

Did we all consider that we have detaching behaviors now? I think that's an important point in the consideration. Basically, TwoD's argument is that a behavior should maintain its own settings in the global settings upon attaching/detaching, if necessary.

It's kinda the reverse of the previous suggestion: If all settings would be temporary, then attachBehaviors() would have to take the initial Drupal.settings or ajax.settings, pass that along to attaching behaviors, and unset Drupal.settings afterwards, because all settings are deemed to be handled locally. detachBehaviors() wouldn't pass any settings at all (because there are none that could be passed).

katbailey’s picture

Priority: Normal » Critical

OK, I take quicksketch's point in #8; currently I'm taking advantage of #360081: Pass Settings Locally to JS Behaviors and in Drupal.behaviors.mymodule.attach I'm doing $.extend(true, Drupal.settings, response.settings) because in the case of Quicktabs that's definitely what I need (new content is being added to the page and it stays there once it's been loaded). My saying "I can't imagine where this would cause a problem" is more a reflection on me than on the idea itself: I'm not seeing beyond my own use cases.

If we leave it up to individual modules to decide whether to merge the settings or not, according to what their particular use case requires, doesn't that make everyone happy? To make that easy, what about adding an extra parameter $merge, which defaults to true but can be set to false or vice versa...? i.e.

function ajax_command_settings($argument, $merge = TRUE) {
  return array(
    'command' => 'settings',
    'settings' => $argument,
    'merge' => $merge,
  );
}

... and then in the js:

  settings: function (ajax, response, status) {
    if (response.merge) {
      $.extend(true, Drupal.settings, response.settings);
    }
    else {
      ajax.settings = response.settings;
    }
  },

and then your ajax.settings could trump your Drupal.settings for passing on to Drupal.attachBehaviors, as per the previous way of doing it.

katbailey’s picture

FileSize
1.57 KB

After discussing this with quicksketch and sun in irc we decided that the merge option should default to false. Here's a patch. I wasn't sure whether to include the correction to the settings key that's in this other patch #595654: AJAX command 'settings' broken, decided to leave it in as this code makes direct reference to it. If that causes problems I can re-roll.

sun’s picture

+++ includes/ajax.inc	15 Oct 2009 01:18:31 -0000
@@ -740,10 +740,11 @@ function ajax_command_css($selector, $ar
-function ajax_command_settings($argument) {
+function ajax_command_settings($argument, $merge = FALSE) {

Looks like the PHPDoc needs to be updated, too? :)

I'm on crack. Are you, too?

katbailey’s picture

FileSize
2.74 KB

OOOPS!! sorry about that, completely forgot. Here it is with updated PHPDoc...

TwoD’s picture

I was most worried about code which gets indirectly invoked by AJAX requests. That code just uses drupal_add_js(..., 'settings'). Those settings are only passed in the very first command controlled by ajax_render, the #23 patch does not affect that. The indirectly called code would not issue any ajax_commmand*s at all (unless it can somehow figure out it's in AJAX-mode, but I don't know if that's easy to do).

I completely agree with quicksketch about not using Drupal.settings as some kind of persistent storage. Doing so, or make it easier to do so, was really not my intention. (In fact, the merging would discourage such practice because of the effect mentioned last in my previous comment.) I don't like adding/changing things in Drupal.settings in 'regular' code any more than you do.
I was merely confused by the documentation stating one thing and the implementation doing another. Since I saw a couple of opportunities in doing thing the way the documentation said, I wanted to change the implementation. I like to think of Drupal.settings as representing the initial state of the document, maybe something to compare changes to. But AJAX adding/removing content changes could just as well have been part of the original document sent from the server. (And would have been had JS been turned off.) Thus, I just thought it would be most consistent to treat it as part of the original document.
The only ajax_command_settings() settings I would 'blindly' want to merge into Drupal.settings would be those generated by ajax_render(), since those are from modules not knowing they're AJAX:ed so they have no reason to return tons of data unless something changed. For other purposes, the #23 patch might come in handy.

In any case, I don't think I really care about which way this issue tilts things anymore, as long as implementation and documentation agrees.
This whole thing has given me a headache so I'm just going to drop my battleaxe for a while and get some sleep.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.28 KB

Re-rolled against HEAD.

sun’s picture

We have a (perhaps serious) AJAX/caching problem, which may be unrelated to this issue, but to my knowledge, this is the only issue that deals with AJAX settings.

So, wanted to let you know about #656782: ajax_process_form() results in settings being returned for elements that aren't re-rendered as part of the AJAX request

Status: Reviewed & tested by the community » Needs review

Re-test of drupal.ajax-command-settings.28.patch from comment #28 was requested by int.

int’s picture

Status: Needs review » Reviewed & tested by the community

it's seems that is working fine one month later.

Status: Reviewed & tested by the community » Needs review

Re-test of drupal.ajax-command-settings.28.patch from comment #28 was requested by webchick.

sun’s picture

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

Status: Reviewed & tested by the community » Fixed

Huh weird. An API change that was actually RTBC by 10/15 that somehow consistently slipped under my radar since then. Sorry, folks. :\

I guess I was maybe waiting for some of the nay-sayers earlier to come and give one last review, but upon further reading of #23, it sounds like this consensus was reached in IRC among a few of them.

Committed to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -DX (Developer Experience), -Ajax, -API clean-up, -D7 API clean-up

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