Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#93 | 208611-93-prevent-multiple-occurrences.patch | 942 bytes | solotandem |
#70 | 208611-70-ajax-render.patch | 593 bytes | mgifford |
#68 | 208611-68-ajax-render.patch | 599 bytes | mgifford |
#49 | 208611-49-ajax-render.patch | 593 bytes | tstoeckler |
#46 | 208611-ajax-render.patch | 611 bytes | tstoeckler |
Comments
Comment #1
Jody Lynn_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.
Comment #2
DougKress CreditAttribution: DougKress commentedHere is a slightly different approach than Lynn's.
Comment #3
dvessel CreditAttribution: dvessel commentedShouldn't this be applied to both settings and inline? And which is faster? The hash or array_search?
Comment #4
Jody LynnI 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.
Comment #5
drummI 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.
Comment #6
DougKress CreditAttribution: DougKress commentedHere's the HEAD patch with the suggested adjustments.
Thanks,
Doug
Comment #7
DougKress CreditAttribution: DougKress commentedForgot to change the status with my last post.
Comment #8
Dries CreditAttribution: Dries commentedMmm, I'm not sure about this solution. Why are we adding it with a different key? It's still in the array twice.
Comment #9
mfer CreditAttribution: mfer commentedThis is strange. I think the root of the problem is in drupal_get_js where we have:
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?
Comment #10
mfer CreditAttribution: mfer commentedThe method used in #6 will only work if it's the exact same setting call.
For example, it would catch this:
But not this:
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.
Comment #11
Dries CreditAttribution: Dries commented@#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.Comment #12
mfer CreditAttribution: mfer commented@dries - the actual behavior of:
is an array that looks like:
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.
Comment #13
lilou CreditAttribution: lilou commentedComment #14
p.brouwers CreditAttribution: p.brouwers commentedHad 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.
Comment #15
donquixote CreditAttribution: donquixote commentedI 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.
The result:
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.
Comment #16
p.brouwers CreditAttribution: p.brouwers commentedYes, array_merge_recursive() is buggy.
When using array_merge_recursive_distinct() the result is correct:
added a patch file for this.
Comment #17
RobLoachThis has got me a couple times.
Comment #18
p.brouwers CreditAttribution: p.brouwers commentedok, this patch needs a review.
Comment #21
p.brouwers CreditAttribution: p.brouwers commentedwhoops, wrong patch
Comment #23
p.brouwers CreditAttribution: p.brouwers commentedAh, the function can also be called with just 1 array as argument.
Fixed this in this patch.
Comment #24
casey CreditAttribution: casey commentedSubscribing
Patch needs at least a reroll for the whitespace fixes that are already fixed.
Comment #25
p.brouwers CreditAttribution: p.brouwers commentedOk, let's try this again :)
Made a new patch without the already fixed whitespace fixes.
Comment #26
markhalliwellIf 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 :)
Comment #27
ksenzee@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. :)
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedIt 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.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #30
effulgentsia CreditAttribution: effulgentsia commented#27: 208611-27-drupal-add-js.patch queued for re-testing.
Comment #31
mfer CreditAttribution: mfer commentedReviewing.... stay tuned.
Comment #32
mfer CreditAttribution: mfer commentedThe 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?
Comment #33
mfer CreditAttribution: mfer commentedSetting to RTBC. Dries or webchick, should we commit this? If so, it's ready to go.
Comment #34
ksenzeeMy 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.
Comment #35
tom_o_t CreditAttribution: tom_o_t commented#27: 208611-27-drupal-add-js.patch queued for re-testing.
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #37
sunYes, this is crucial.
Comment #38
webchickOk. 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.
Comment #39
rfayIf this API change breaks BC, please let me know how and what to do about it and I'll announce.
Thanks,
-Randy
Comment #40
ksenzeeIt 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
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.
Comment #42
effulgentsia CreditAttribution: effulgentsia commented#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 :)
Comment #43
sunAlthough 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
Comment #44
sund'oh, sorry.
Comment #45
catchClarifying what needs to be done here.
Comment #46
tstoecklerI'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.
Comment #47
sundrupal_get_js() calls drupal_array_merge_deep_array() directly without cufa().
Comment #48
RobLoachWould be good to get rid of call_user_func_array here ;-).
Comment #49
tstoecklerSure thing.
Comment #50
catchMarking needs work for tests.
Comment #51
tstoecklerCan 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.
Comment #52
rfay@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.
Comment #53
sun#40 contains a unique and precise flow including expectations that can be turned into a test.
Comment #54
attiks CreditAttribution: attiks commentedFYI: 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
Comment #55
tim.plunkettAs 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().
Comment #56
tim.plunkettI 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".
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedPeople 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().
Comment #59
Wim LeersAs 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.
Comment #60
mkadin CreditAttribution: mkadin commentedFor those familiar with this issue, should we be removing this block of docs in ViewsFormBase.php?
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedYes. For D8, I believe all ways of adding settings are now fully idempotent, so that can be removed.
Comment #62
attiks CreditAttribution: attiks commentedThe patch in #49 is still needed for Drupal 7
Comment #63
attiks CreditAttribution: attiks commented49: 208611-49-ajax-render.patch queued for re-testing.
Comment #64
attiks CreditAttribution: attiks commentedManually tested:
curl https://drupal.org/files/208611-49-ajax-render.patch | git apply -v
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 593 100 593 0 0 548 0 0:00:01 0:00:01 --:--:-- 1202
Checking patch includes/ajax.inc...
Hunk #1 succeeded at 292 (offset 1 lines).
Applied patch includes/ajax.inc cleanly.
Solves problems of masked_input
Comment #65
attiks CreditAttribution: attiks commentedComment #67
JvE CreditAttribution: JvE commented49: 208611-49-ajax-render.patch queued for re-testing.
Comment #68
mgiffordjust a re-roll.
Comment #70
mgiffordarg
Comment #71
Strae CreditAttribution: Strae commentedGot same problem, patch #70 solved it.
Thanks a lot!
Comment #73
Jelle_SSimple and straight-forward patch, works as expected. If it still applies this is RTBC.
Comment #74
Jelle_SComment #75
dsayswhat CreditAttribution: dsayswhat commentedI can also confirm it works as expected. Thanks!
Comment #76
thiemo CreditAttribution: thiemo commentedPatch works!
It solves a bug for references_dialog_insert as well (used in ERPAL).
Comment #78
joelpittetComment #81
joelpittetComment #82
zaporylieConfirmed - patch #70 works and you can commit it.
Comment #85
joelpittetComment #86
zaporylieI've run tests for patched D7 on local machine and have no errors. Any idea why that patch constantly fails tests?
Comment #89
joelpittetNo idea why it's failing tests.
Comment #90
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
The random test failures are #2246867: Intermittent "test did not complete due to a fatal error" failures when testing Drupal 7 patches.
Comment #93
solotandem CreditAttribution: solotandem commentedI tried this patch and found it a big improvement but still wanting; the duplication of array items still exists. My specific use case involves openlayers, similar to the summary in #1983096: Bring JS settings merging in ajax_render() in line with drupal_get_js(). While this patch was a big improvement, i.e. the map would again render, only with the attached patch was all functionality restored, e.g. ability to zoom the map.
Comment #96
xjm@solotandem, pleaseopen a new issue for that. Thanks!