The attached patch uses the deep copy version of $.extend. If your module adds JS values to the Drupal.settings.myModuleValues by using deep copy $.extend we make sure that an AJAX response that adds a new value to those settings is actually merged into the object instead of replacing it completely.
Ajax Load module suffered from the same issue: #943132: Scripts Settings need to be deep copied.

CommentFileSizeAuthor
#14 ctools_ajax_render_3-999164.patch2.18 KBmrjeeves
#6 ctools_ajax_render_2.patch2.08 KBmrjeeves
#5 ctools_ajax_render.patch2.09 KBmrjeeves
#4 ctools_deepcopy_2.patch1.12 KBmrjeeves
ctools_deepcopy_1.patch411 bytesandreiashu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

drewish CreditAttribution: drewish commented

+1 I just ran into a bug were settings were disappearing.

drewish’s picture

drewish CreditAttribution: drewish commented

Also worth pointing out that this is what D7's setting command does:

  /**
   * Command to set the settings that will be used for other commands in this response.
   */
  settings: function (ajax, response, status) {
    if (response.merge) {
      $.extend(true, Drupal.settings, response.settings);
    }
    else {
      ajax.settings = response.settings;
    }
  },
andreiashu’s picture

andreiashu CreditAttribution: andreiashu commented
Version: 6.x-1.8 » 6.x-1.x-dev

Hopefully we can push this into the next version of ctools?

mrjeeves’s picture

mrjeeves CreditAttribution: mrjeeves commented
FileSize
ctools_deepcopy_2.patch1.12 KB

i've reviewed the patch above and added an update to the preprocess function on the server-side to the same effect. this issue is blocking the dependency of ctools for the 2.x branch of ajax_load. this is against the latest dev

mrjeeves’s picture

mrjeeves CreditAttribution: mrjeeves commented
FileSize
ctools_ajax_render.patch2.09 KB

ooook, after working on this issue more, and looking over the d7 stuff again, the merge flag is really handy, this patch includes a merge arg and also implements a $return arg for the ctools_ajax_render function because I prefer the argument order and I need it as well, If you would like the $return arg patch separate, just let me know.

mrjeeves’s picture

mrjeeves CreditAttribution: mrjeeves commented
FileSize
ctools_ajax_render_2.patch2.08 KB

and once more changing the 'else if' to 'elseif'....

kevinob11’s picture

kevinob11 CreditAttribution: kevinob11 commented

+1 this just killed me on a custom module. Any chance we can get his committed?

andreiashu’s picture

andreiashu CreditAttribution: andreiashu commented
Status: Needs review » Reviewed & tested by the community

We can set this as rtbc. @mrjeeves patch in #6 looks robust and extends the API while keeping the existing defaults.

mrjeeves’s picture

mrjeeves CreditAttribution: mrjeeves commented

Thank you. I've been waiting a good minute for this to be rtbc, I'd really like to see this change made as it would allow me to work further on the "live" updates system I've imagined possible for some time.

kevinob11’s picture

kevinob11 CreditAttribution: kevinob11 commented

I ended up just overriding the js function with my own with the same name. I'm not particularly sure if this is 100% safe to do or best practices (I would love to hear about it if it isn't) but since ctools ajax ended up in core in D7 I think most of the changes to D6 version are bug fixes rather than feature requests. I just wanted to throw the idea out there because it works quite well for me and doesn't require hacking ctools or waiting for this to be committed.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented

...coming from #1060340-10: Port AJAX load module to drupal 7.

mrjeeves’s picture

mrjeeves CreditAttribution: mrjeeves commented

This issue has be open for ages, the patch is small and robust, what do we have to do to get some attention on this matter???

rjbrown99’s picture

rjbrown99 CreditAttribution: rjbrown99 commented

Not sure why, but #6 was causing issues for me when using jCarousel in a modal. Specifically, this line in ajax-responder.js:

$.extend(data.merge, Drupal.settings, data.argument);

It was causing my jCarousel javascript to not be executed. I tried it with the original patch in the issue, and I do not have a problem if it looks like this:

$.extend(true, Drupal.settings, data.argument);

I'm short on time at the moment so I'm just reporting it. No suggested fix for now but I'm happy to provide extra information if needed.


Update: It works properly for me if we do something like the D7 version, such as:

  Drupal.CTools.AJAX.commands.settings = function(data) {
    if (data.merge) {
       $.extend(true, Drupal.settings, data.argument);
    }
    else {
      $.extend(Drupal.settings, data.argument);
    }
  };
mrjeeves’s picture

mrjeeves CreditAttribution: mrjeeves commented
Status: Reviewed & tested by the community » Needs review
FileSize
ctools_ajax_render_3-999164.patch2.18 KB

reroll with additions from #13

chiddicks’s picture

chiddicks CreditAttribution: chiddicks commented

I'm just in the process of testing #14. I appreciate that from a backward-compatibility perspective adding a parameter to ctools_ajax_render is the safe choice, but is there any case where doing a deep merge is going to break functionality? As far as I can see, there's very little risk aside from a slight performance hit client-side?