Posted by walkah on January 10, 2008 at 10:43pm
27 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | API change, needs backport to D7 |
Issue Summary
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.
Comments
#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.
#23
Ah, the function can also be called with just 1 array as argument.
Fixed this in this patch.
#24
Subscribing
Patch needs at least a reroll for the whitespace fixes that are already fixed.
#25
Ok, let's try this again :)
Made a new patch without the already fixed whitespace fixes.
#26
If you're anything like me, you've added this to your D6 version for better performance. However I noticed my Views' ajax suddenly stopped working. I determined that because each setting is simply set with an ambiguous array, each key is thus 0. Because of this, confuses array_merge_recursive_distinct. I have added a special if statement to detect if the key is ajaxViews and then it simply uses the normal array_merge_recursive function. Not sure if this will help anyone, but I also figured this might give everyone some food for thought and let someone find a more dynamic solution :)
<?php
function array_merge_recursive_distinct() {
// Holds all the arrays passed
$params = & func_get_args();
// First array is used as the base
$merged = array_shift($params);
foreach ($params as $array) {
foreach ($array as $key => &$value) {
if ($key == 'ajaxViews') {
$merged[$key] = array_merge_recursive($merged[$key], $value);
}
elseif (is_array($value) && isset($merged[$key]) && is_array($merged[$key])) {
$merged[$key] = array_merge_recursive_distinct($merged[$key], $value);
}
else {
$merged[$key] = $value;
}
}
}
return $merged;
}
?>
#27
@markcarver has pointed out a deficiency in the array_merge_recursive_distinct function from php.net: It treats numeric arrays the same as associative arrays, when really we want to add items with numeric keys and replace items with string keys. Fortunately, we don't need to fix it: effulgentsia has already written a version with the correct behavior. It was originally posted at #791860: array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead., but that issue was postponed -- and then at #823428: Impossible to alter node URLs efficiently, but that issue ended up going a different direction -- so I'm moving it here with his permission. It comes complete with tests. Please credit effulgentsia on commit.
I also added tests specifically for drupal_add_js() to confirm that we get the behavior we expect. They fail on HEAD and pass with the patch. I love when that happens. :)
#28
It should come as no surprise that I'm +1 for this :)
Thanks, all, for making the case for why this is needed, as my prior attempts to make that case have failed.
#29
I wish I could RTBC this, but given that much of #27 came from code I wrote, that wouldn't be playing by the rules. What else is needed for this to be RTBC?
Once this is in, I hope we can make module_invoke_all() also use the drupal_array_merge_deep() function in #27. We now have two core functions: rdf_get_namespaces() and file_download(), and maybe others I'm not aware of, needing to work around the annoyance of array_merge_recursive() turning string values into arrays when multiple modules return values for the same key.
#30
#27: 208611-27-drupal-add-js.patch queued for re-testing.
#31
Reviewing.... stay tuned.
#32
The patch works as expected, is well documented, and has tests. If we are going this route the code is RTBC.
The only down side is this causes a small hit to performance (.7%) in my limited testing. As more modules add settings this number would grow. I used a small number of settings and saw the performance hit. If a bunch of modules add settings this would be worse.
While the patch works, do we want our own (user space) recursive function(s) or should we expect developers to only call drupal_add_js() for a setting once to avoid the problems?
#33
Setting to RTBC. Dries or webchick, should we commit this? If so, it's ready to go.
#34
My instinct is not to sacrifice correctness here. If we don't make this change, then drupal_add_js() is idempotent when used to add files, but not when used to add settings. That's a WTF that will be hard to document or use correctly.
#35
#27: 208611-27-drupal-add-js.patch queued for re-testing.
#36
This looks to me like a straight-up bugfix, but it does technically change the API a bit, so I am adding the "API change" tag out of an abundance of caution :) In other words, it would be good to get this one in as soon as possible.
#37
Yes, this is crucial.
#38
Ok. I think the case has been made for this breaking functionality, going all the way back to D5, and this patch manages to work around PHP's deficiencies in a way that doesn't break APIs. (And comes with juicy and delicious tests! Mmmm. Tests...)
The 0.7% slowdown is concerning, especially if it increases with the number of calls. But neither sun nor I could figure out another way that this function could be written faster. And, if it could, it would end up being a non-API-breaking change in the function body which could be committed as a follow-up.
Committed to HEAD.
#39
If this API change breaks BC, please let me know how and what to do about it and I'll announce.
Thanks,
-Randy
#40
It changes the API in that if someone was working around the existing bug, they no longer have to do so. :) Personally I don't think it needs to be announced. But the basic summary is that drupal_add_js() is now idempotent when used to add settings. If you call
drupal_add_js(array('myModule', array('key' => 'value')), 'setting');drupal_add_js(array('myModule', array('key' => 'value')), 'setting');
the second call has no effect (as you would expect), and you get the following in Drupal.settings:
{myModule: {key: value}}Before, you would have gotten
{myModule: {key: [value, value]}}Anyone who was counting on that buggy behavior will have to change their code. But I'm guessing if they wanted an array, they would have written their drupal_add_js() call to add an array in the first place.
#41
Automatically closed -- issue fixed for 2 weeks with no activity.
#42
#27 changed drupal_get_js() from array_merge_recursive() to drupal_array_merge_deep(), but the same change is also needed in ajax_render(). Easy 1 line change there, but tests are also needed. Will post a patch when I have time, or if someone else wants to, even better :)
#43
Although there's no concrete bug report here, the remaining piece is clearly a major oversight, so let's keep this major and get a patch to replace that array_merge_recursive() in http://api.drupal.org/api/drupal/includes--ajax.inc/function/ajax_render/7 ASAP
#44
d'oh, sorry.
#45
Clarifying what needs to be done here.
#46
I'm not able to write tests for this, but it would be super awesome to see this committed.
I just wasted 2 hours on this.
#47
drupal_get_js() calls drupal_array_merge_deep_array() directly without cufa().
#48
Would be good to get rid of call_user_func_array here ;-).
#49
Sure thing.
#50
Marking needs work for tests.
#51
Can anyone give me a hint on how to write those tests or where similar tests are located?
I'm a total AJAX noob, and all the more I want to get this committed as it's virtually impossible to debug this for someone like me.
#52
@effulgentsia writes about tests for ajax in #789186-26: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage (and the whole issue, where testing possibility was created). And of course for anybody who's unfamiliar with simpletest in general, I'll point to the Simpletest Tutorial.
#53
#40 contains a unique and precise flow including expectations that can be turned into a test.
#54
FYI: I just tested the patch in #49 on a Drupal 7 installation where we were having problems with AJAX callbacks, and our (clientside validation) settings were added twice.
Blocking #1309504: Integrate in clientside validation
#55
As a follow-up, I've opened #1356170: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep().
#56
I was looking for the origin of drupal_array_merge_deep() and git blame led me here. I'm going to kidnap the patch in #49 (which no longer applies anyway) and deal with it in #1356170: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep().
Resetting to fixed as it was in #38, and adding a more descriptive title than "drupal_add_js includes settings twice".
#57
Automatically closed -- issue fixed for 2 weeks with no activity.
#58
People here might be interested in #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings) which changed the behavior in D8 even further to prevent duplicates of array values by emulating the integer key preservation of jQuery.extend().
#59
As said in #58, the work done here was extended in #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings).
Both were included in the change notice over at http://drupal.org/node/1911578.
#60
For those familiar with this issue, should we be removing this block of docs in ViewsFormBase.php?
// With the below logic, we may end up rendering a form twice (or two forms// each sharing the same element ids), potentially resulting in
// drupal_add_js() being called twice to add the same setting. drupal_get_js()
// is ok with that, but until ajax_render() is (http://drupal.org/node/208611),
// reset the drupal_add_js() static before rendering the second time.
#61
Yes. For D8, I believe all ways of adding settings are now fully idempotent, so that can be removed.