Use drupal_add_js as documented [#935892]

The micro-optimization introduced in http://drupal.org/node/646912 tries to detect for json_encode and, if present, introduces a local work-around to the 'setting' mode of drupal_add_js, by injecting a Drupal.settings variable through the 'inline' mode of drupal_add_js.

Using an inline insert for the Drupal.settings variable is problematic because it does not use the jQuery extend() function to add to Drupal.settings, which means that it could end up destroying instead of adding to the maps array. In addition, this isn't following the documented uses for drupal_add_js.

As it happens, I ran into this by trying to embed an openlayers map into a Dialog using the Dialog and CTools modules. CTools' AJAX rendering doesn't currently support the 'inline' mode from drupal_add_js, but it does have support for the 'setting' mode.

I think that it is better to use drupal_add_js as proscribed by the documentation and use the 'setting' mode for adding variables to Drupal.settings. If the concern is that drupal_to_js is too slow, that is where performance-concerned distributions like Pressflow come in. Pressflow, for what it's worth, switches to use json_encode in all places, so modules that properly implement drupal_add_js('setting') all benefit.

Attached is a patch against 6.x-2.x-dev that rolls back the json_encode detection and uses drupal_add_js properly.

Comment	File	Size	Author
	openlayers-no-inline-js.patch	840 bytes	bmcmurray

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tmcw CreditAttribution: tmcw commented 11 October 2010 at 14:47

Category:

bug

» task

drupal_add_js is a serious bottleneck, and the optimization of using json_encode should be accepted upstream, but is unlikely to be. We can't accept this patch outright, because, despite using exclusively Pressflow in production around here, other people don't.

Comment #2

zzolo CreditAttribution: zzolo commented 13 October 2010 at 18:03

How about an admin setting to use json_encode by default and the option to turn it off? I know its totallly a bad hack, but given the situation of things, it makes some sense?

Comment #3

tom_o_t CreditAttribution: tom_o_t commented 2 November 2010 at 14:24

Status:

Active

» Needs work

@tmcw - are you open to @zzolo's suggestion? If so I'll refactor the patch. I'm working with @bmcmurray on the project that he created the patch for, so would love to get this patch accepted.

Comment #4

zzolo CreditAttribution: zzolo commented 5 August 2011 at 20:49

Category:

task

» feature

Let's go ahead with the admin setting (if anyone is still paying attention). Also, does this still apply to D7?

Comment #5

tom_o_t CreditAttribution: tom_o_t commented 5 August 2011 at 20:54

I'm still paying attention ;)

No idea about D7 yet unfortunately.

Comment #6

ezman CreditAttribution: ezman commented 26 October 2013 at 19:11

On my Drupal 7 site I use the Dialog API and AJAX functions to load node add/edit forms into a popup.

I have a content type that contains an Openlayers Map widget which doesn't display when delivered via AJAX.

Does anyone know a workaround for this in Drupal 7 ?

The response to the AJAX POST contains settings for openlayers and geofield which look correct. It's prepending its CSS and JS files to the element, and those files are requested by the browser.
There's no Javascript error in the console, and I can't work out why it's not working in my Dialog when all of my other fields are.

(This happens for me when using both stable and dev versions of openlayers, geofield, and geophp.)