drupal_add_js includes settings twice
walkah - January 10, 2008 - 22:43
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
When creating a CCK formatter with token.module installed the formatter is included twice, thus causing 2 calls to drupal_add_js() with a 'setting' parameter. This creates the setting to be included twice which can break certain javascript files.
#1
_drupal_add_js does not do anything to prevent settings or inline types of js from being called more than once. Patch is an idea to do so for the settings.
#2
Here is a slightly different approach than Lynn's.
#3
Shouldn't this be applied to both settings and inline? And which is faster? The hash or array_search?
#4
I agree it should be applied to inline js as well. I think Doug's hash method is a little better and more analogous to what is already done for external js files.
#5
I would like to see this committed to the development version first, and then backported as needed. The code looks okay on reading, except the first added lines are indented with tabs instead of spaces. The patch does apply to HEAD.
#6
Here's the HEAD patch with the suggested adjustments.
Thanks,
Doug
#7
Forgot to change the status with my last post.
#8
Mmm, I'm not sure about this solution. Why are we adding it with a different key? It's still in the array twice.
#9
This is strange. I think the root of the problem is in drupal_get_js where we have:
<?phpswitch ($type) {
case 'setting':
$output .= '<script type="text/javascript">jQuery.extend(Drupal.settings, ' . drupal_to_js(call_user_func_array('array_merge_recursive', $data)) . ");</script>\n";
break;
?>
It looks like the intent of call_user_func_array('array_merge_recursive', $data) is to roll-up the arrays and their settings so this condition doesn't happen. This might be a good place to start looking. Why isn't this eliminating the duplicates?
#10
The method used in #6 will only work if it's the exact same setting call.
For example, it would catch this:
<?phpdrupal_add_js(array('myvar' => 'value'), 'setting');
drupal_add_js(array('myvar' => 'value'), 'setting');
?>
But not this:
<?phpdrupal_add_js(array('myvar' => 'value'), 'setting');
drupal_add_js(array('myvar' => array('value', 'value 2')), 'setting');
?>
The hashes would be different so 'value' would still be added twice.
We almost need an array_merge_recursive_unique function that knows the difference between numbers and associative keys and only does performs unique calls on numbers.
#11
@#10, mfer, the expected behavior would be for
drupal_add_js(array('myvar' => array('value', 'value 2')), 'setting');to overwrite the previous value set bydrupal_add_js(array('myvar' => 'value'), 'setting');, not?It looks like the problem is easily solved by changing the way $data is stored internally. In case of settings, we need to extract keys and values, and overwrite duplicate keys. It looks to me like drupal_add_js() is doing too much at once. This problem is easily solved by breaking up the function and defining a better API like
drupal_add_js_setting($key, $value, $defer = FALSE, ...). Such function could easily solve duplicate keys, and is also easier to grok.#12
@dries - the actual behavior of:
<?phpdrupal_add_js(array('myvar' => 'value'), 'setting');
drupal_add_js(array('myvar' => array('value', 'value 2')), 'setting');
?>
is an array that looks like:
<?phparray( 'value', 'value', 'value2' );
?>
This is when it gets transfered to drupal_to_js. Both setting calls store the settings separately and they are recursively combined before being sent to drupal_to_js. The problem is when they are recursively combined it isn't checking for uniqueness. And, the array_unique php function looks as unique values and not key value pairs.
I like your idea for a drupal_add_js_setting function. Currently, the value can be arrays with unlimited nesting that can be added to with multiple drupal_add_js calls. I would like to see this ability remain.
#13
#14
Had the same issue. Calling drupal_add_js twice for a setting changes it's datatype from your setting to an array containing both settings. (Thus not overridding the previous setting)
Read http://nl.php.net/manual/en/function.array-merge-recursive.php#92195 about the problem. His function 'array_merge_recursive_distinct' could solve this problem.
#15
I did not follow the complete discussion (coming from uc_option_image), but here is an example for how PHP's array_merge_recursive sucks badly with numeric keys.
<?php
<?php
$x = array('str' => array(4 => 'x'));
$y = array('str' => array(8 => 'y'));
print_r(array_merge_recursive($x, $y));
?>
The result:
Array
(
[str] => Array
(
[4] => x
[5] => y
)
)
As you can see, the 8 key becomes a 5, while the 4 key is not changed. Feels quite inconsistent.
Solution for us: Either use something different from array_merge_recursive, or use string keys instead of numeric ones.
#16
Yes, array_merge_recursive() is buggy.
When using array_merge_recursive_distinct() the result is correct:
Array
(
[str] => Array
(
[4] => x
[8] => y
)
)
added a patch file for this.
#17
This has got me a couple times.
#18
ok, this patch needs a review.
#19
The last submitted patch failed testing.
#21
whoops, wrong patch
#22
The last submitted patch failed testing.