Problem/Motivation
- Q: What if a setting like
drupalSettings.foo.bar.uiConfig = [ 'button A', 'button B']
gets added twice to the page (throughdrupal_add_js()
or#attached
)? - A: The result is NOT
drupalSettings.foo.bar.uiConfig = [ 'button A', 'button B']
like you would expect, butdrupalSettings.foo.bar.uiConfig = [ 'button A', 'button B', 'button A', 'button B']
!
According to nod_, that's also the ajaxPageState
is using objects with keys and useless values.
This has been problematic for some contrib developers, because it effectively prevents one from using arrays in drupalSettings
if it's possible that that array gets added multiple times during the page generation process. With the coming of D8's new rendering pipeline (everything as blocks + subrequests), this problem will be noticed by many more developers.
Proposed resolution
Preserve integer keys. The funny part is that the Config system had the exact same use case and already fixed it for us: #1825466: Allow NestedArray::mergeDeepArray() to preserve integer keys. Thus, the fix is a single-line change:
- $element['#value'] = 'var drupalSettings = ' . drupal_json_encode(NestedArray::mergeDeepArray($item['data'])) . ";";
+ $element['#value'] = 'var drupalSettings = ' . drupal_json_encode(NestedArray::mergeDeepArray($item['data'], TRUE)) . ";";
We have to ensure of course that this doesn't break anything, but, personally I think it should be discouraged to rely on it since it can cause so much confusion: you put an array into the settings, multiple times, and you get an unpredictable result (it also depends on the processing order and with blocks/subrequests, that becomes hard to guarantee). _nod seems to agree:
most often I've seen problems with it more than using it as a "feature"
Remaining tasks
- Reviews. Particularly: ensure this doesn't break anything else. From my testing, this was not the case.
- Simplify
ajaxPageState
.
User interface changes
None.
API changes
- True API changes happened over at #1825466: Allow NestedArray::mergeDeepArray() to preserve integer keys.
- The Drupal JS settings merging behavior changes to allow for JS array literals to be used in
drupalSettings
.
Comment | File | Size | Author |
---|---|---|---|
#47 | JS-settings-merging-behavior-1875632-47.patch | 486 bytes | RdeBoer |
#42 | prepend_settings-1875632.patch | 575 bytes | quicksketch |
#39 | prepend_settings-1875632.patch | 574 bytes | quicksketch |
#31 | 1875632-drupalSettings-merge-followup-31.patch | 7.03 KB | effulgentsia |
#27 | 1875632-tabledrag_broken-27.patch | 1.3 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
jessebeach CreditAttribution: jessebeach commentedThe change seems fine. We'll want a unit test with this patch rather than a followup.
Comment #4
Wim LeersI tried to be too fast:
drupalSettings
instead of checking whether some string exists somewhere in the generated<script>
tag or not. (Note that this should probably be used everywhere in this test, but I first want confirmation that people agree this is a good path to take.)drupalSettings
API was PHP-oriented. This makes some sense. But really, when you think about it, it makes far more sense to make it JS-oriented. This is intended to allow PHP (module) developers to pass settings to whatever JS lib exists out there. JSON/drupalSettings
is about array literals and object literals. This reroll makes that very explicit in the tests.Attached is the patch with updated & improved tests.
Comment #5
Wim LeersComment #6
jessebeach CreditAttribution: jessebeach commentedThe merging that occurs with this patch is one of matching indices, not matching values. Merging in this way will produce the following behavior. I want to make sure the two scenarios I'll present are not valid use cases.
Let's say we have a property in a settings array like this:
I add a new setting with a property cloud later in the code.
With this patch, I get the merged result
My naive understanding of merging told me to expect
['1','2','3','4']
, not the result that does occur['4', '2', '3']
.In another scenario, let's say I have three settings arrays for a Editor toolbar that define the buttons that should appear in the toolbar. Each occurs later in the code than the one before it.
When these three settings arrays are merged down, we will get:
Is it the intent of this patch to have settings arrays under that same key to be merged by index rather than value?
Comment #7
Wim Leers#6 scenario 1:
If you pass settings for some JS, you want to make sure that whatever you pass, actually arrives correctly. The example I'm starting from is the CKEditor toolbar configuration example. It is possible that different blocks in the page add the same settings in D8, because blocks will be rendered independently. That means we must be able to rely on setting merging to work in a non-mind-breaking, understandable manner. The CKEditor toolbar has this kind of configuration:
drupalSettings.foo.bar.toolbar = [ 'Source', 'separator', 'strong', 'em', 'u', '\' ]
. Now if this gets added multiple times on the same page, with the old merging behavior, we'd getdrupalSettings.foo.bar.toolbar = [ 'Source', 'separator', 'strong', 'em', 'u', '\', 'Source', 'separator', 'strong', 'em', 'u', '\']
. This will result in two toolbars.So, argument 1: we should be able to reliably pass an array through
drupalSettings
from the server to the client side; and in D8 this means that it must still be the same array after merging when it gets added N times on the same page load.Now let's move to your example. First of all: note that the behavior you one is *conflicting* with the one I described above. Secondly: why do I believe this is bad? A) it's impossible to add an array setting multiple times on the same page load, see above; B) the only valid use case I can think of for the "auto append" (aka old) JS settings merging behavior is to pass identifiers of things on the page that must be processed by JS (i.e. you cannot have global knowledge of the things present on the page, hence you want to register them one by one and then you of course don't want the second one to overwrite the first one etc.); since these things have unique identifiers in the first place, it's easy enough to use associative arrays/object literals for this instead. So in your example:
'cloud' => array('1' => array(), '2' => array(), '3' => array())
instead of'cloud' => array('1', '2', '3');
this will not cause 'auto append' (since the keys of a PHP associative array or a JS object literal don't have a specific order), but will cause all of the unique identifiers to be present (in an undefined order).#6 scenario 2:
Why would you be setting three different arrays? That sounds like three different configurations, meaning that each of them should live under a different key, otherwise you would *never* be able to properly use/access each individually, also not with the old merging behavior, then you'd have gotten
array('strike', 'b', 'u', 'b', 'u', 'u')
.Rerolled patch to extend the tests to support the above claims.
Comment #8
nod_That's a WTF, the patch fixes the problem and the tests added makes sense and pass. Nothing to add here :)
Comment #9
nod_Need a change notice
Comment #10
Wim LeersComment #11
webchickCommitted and pushed to 8.x. Thanks!
Setting back to active for the change notice. Although isn't this just fixing the behaviour to be the same as in D7? If so, I'm not sure why we need a change notice for that.
Comment #12
xjmFor reasons I do not pretend to understand, the commit from this issue breaks the block administration page in #1535868: Convert all blocks into plugins such that the weight column always shows up, regardless of how the tabledrag show/hide weight setting is toggled. Help over there would be appreciated.
Comment #13
nod_ok tabledrag issue. It relies on the old behavior since drupal_add_tabledrag will call drupal_add_js directly instead of building the whole setting object and adding it only once.
The problem will be visible on blocks UI and apparently when using groups for filters in views UI.
if the tabledrag api was a bit smarter this wouldn't happen. Now the question is what to do. The tabledrag rewrite is still nowhere and will take some time. At this hour of the night I don't have a quick and dirty idea on how to solve the issue with the admin screen of the blocks ui. Sleep may help.
Comment #14
sunHrm. Let's roll this back, please.
#12 is only one of many consequences here.
Unless I'm mistaken, there is an existing issue for this in the queue for quite some time already, and which discussed all the consequences and problems to a much greater extent.
Comment #15
tim.plunkettI agree this should be rolled back, if only because the changes to the assertions look a little odd and haphazard.
Comment #16
Wim Leers#15: I don't see what's wrong with the assertions?
#14: What is that issue, then? :) I couldn't find it.
nod_ told me he's going to work on tabledrag this weekend, which would solve #12. If he hasn't fixed it by Monday, then I'll work on this on Monday.
Comment #17
nod_Ugly code needs ugly solution.
This fixes #12
@sun: never run into it. It's been either closed for more than a year or not tagged properly.
Comment #18
nod_Comment #19
sunThe committed changes essentially resolved one WTF by introducing a new WTF and bug.
You're no longer able to enhance a list of items with additional items now, because the implementation assumes that there are no non-indexed arrays at all anymore. That's wrong.
Comment #20
nod_How do you solve the issue in the OP without this behavior?
Can you spell out the bug for me, I'm not seeing it. Is it what jesse was talking about in #6?I guess you meant the tabledrag stuff. At lest the WTF is not a hidden behavior, I mean do we really want to use the behavior tabledrag was using to do what it needed? I'm happier with the wtf in the patch than the pre-commit wtf.Comment #21
nod_From another angle, we're talking about settings here this is just getting js settings merging to behave like CMI. Consistency++
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedMy initial reaction to this was the same as #19: the change made by this issue is a total WTF to PHP programmers, because PHP programmers expect
array('classes' => array('foo', 'bar'))
andarray('classes' => array('baz'))
to result inarray('classes' => array('foo', 'bar', 'baz'))
when merged, not inarray('classes' => array('baz', 'bar'))
, which is the new behavior as of #11. Note that module_invoke_all() uses PHP's array_merge_recursive(), which does not preserve integer keys, and we rely on that well known PHP behavior in the editor_element_info() implementation in #1833716-140: WYSIWYG: Introduce "Text editors" as part of filter format configuration to add an extra callback, rather than replace whatever other one happens to be at index 0.However, #4.3 makes a good point:
jQuery.extend()
does preserve integer keys. As someone used to the PHP way, I dislike that, but JS programmers who are used to that probably consider it to be the correct way and dislike the PHP way. So, for drupalSettings, matching the JS convention rather than the PHP convention might make sense.If we decide to keep it like this, we certainly need a change notice. This does not fix a bug (except as a side effect: there would have been other ways to deal with the uniqueness problem that prompted this) or match D7 behavior. It's a decision to change the API of merging settings from something that makes sense to PHP programmers to something that makes sense to JS programmers.
It's an interesting side note that CMI also changed to the JS way. I don't know why that was done, but it could be an indication that PHP programmers are mixed on what makes more sense.
Comment #23
effulgentsia CreditAttribution: effulgentsia commented#14 might be referring to #208611: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice, which fixed the problem of scalar values being turned into arrays when added twice, but didn't fix arrays getting duplicates when added twice.
Comment #24
nod_Bah, I love you Alex, that's so much clearer explained like that. Thanks for finding the issue too.
Comment #25
sunThe following is no longer possible:
persister.js:
foo.module:
bar.module:
Comment #26
nod_Doesn't mean you can't acheive the same things differently. I'd have a specific method doing something to hold the values and it'd be the responsibility of the module using the persister setting to add it to the js after everything was added on it. As smarter version of what drupal_add_tabledrag does in the #17 patch essentially.
I'm not seeing the benefit of exposing and manipulating the raw data here beside the fact that it used to work. I'd argue that it shouldn't be done this way to begin with.
Comment #27
Wim LeersFrom #208611-34: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice, ksenzee:
This issue is essentially trying to do the same, but for a different WTF.
First: what was already fixed in another issue.
The WTF that was solved in #208611: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice (and which is likely what sun alluded to in #14) is this (again explained succinctly by ksenzee, this time from #208611-40: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice:
The use case there was that a module may be adding the same setting twice, because e.g. a formatter/block/view/whatnot is added twice, and it doesn't make sense to generate a unique identifier for each occurrence. E.g. if a setting applies to a text format, then you only key by text format, because it doesn't make sense to somehow add a unique identifier to each location/instance/occurrence where the text format is used, only to allow you to add the same setting many times just for the sake of working around Drupal API limitations.
Second: what was fixed in this issue.
The WTF that was solved here:
The use case there was that a module may be adding the same setting twice, because e.g. a formatter/block/view/whatnot is added twice, and it doesn't make sense to generate a unique identifier for each occurrence. E.g. if a setting applies to a text format, then you only key by text format, because it doesn't make sense to somehow add a unique identifier to each location/instance/occurrence where the text format is used, only to allow you to add the same setting many times just for the sake of working around Drupal API limitations.
Conclusion
1. The closing paragraphs for both solved WTFs are indeed identical, because they solve the same problem, but for different types of values.
2. The idempotency aspect is vitally important, especially when we have ESI (blocks rendered via subrequests, with each block specifying its attachments — also see #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]). Then it becomes an absolute necessity.
3. The only reason we were using arrays differently before is to be able to "just append stuff and have it show up magically". Whenever we just want a bucket of things, we now have two options: 1) give them some key (a unique identifier or even a random identifier that the JS knows how to deal with), 2) build an array that contains the bucket of things, then set this array once.
4. Before this patch, it was effectively impossible to pass an array of settings, with the intention of using that array from
drupalSettings
, because the "feature" (3.) that is now gone prevented the idempotency (2.) that was required to be able to pass such an array. It was only accidentally possible as long as the same setting wouldn't be added twice on the same page load, and with the coming of ESI/subrequests/attachments to every individual part, many settings are going to be added many times on the same page load.I hope this clarifies the problem.
#25:
Well, this relies on the (AFAICT "broken by design") behavior that I explained in the above conclusion to be bad. I do understand and appreciate that this patch has complicated another use case, but it has done so for the (just) cause of idempotency.
With this patch committed, you'll have to do something like this:
That's annoying for this use case, but idempotency trumps annoyance in my book.
In attached patch: alternative fix for the broken tabledrag; this fix is more explicit/less magical than the one in #17. nod_ has already reviewed it and thinks it's better.
Comment #28
Dave ReidI'm pretty sure the Media module also relies on this "broken by design" behavior as well. Likely many more contribs possibly too.
Comment #29
tim.plunkettFullCalendar depends on the previous behavior, and will be broken by this.
Comment #30
Wim Leers#28 & #29: We can fix those as well, of course. I do understand that it's annoying to change the code, but that doesn't help solve the underlying problem. Please comment on the reasoning and the committed fix instead — if you disagree with how it works, that's fine, but then please also post an alternative. I don't see an alternative.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedI agree with Wim, despite needing to mentally adjust to the change. Here's a cleanup that includes the #27 patch plus ensures that all places where settings are merged are using the same logic, and documents it.
Comment #32
Wim Leers#31: Great!
Splitting it out in a separate method to be able to document it more cleanly & clearly makes a lot of sense — thanks.
Comment #33
nod_Agreed, that's a pretty nice way of handling it. Patch works.
Comment #34
webchick#31 is a reasonable follow-up to what's already in HEAD, so...
Committed and pushed to 8.x.
However, it's not clear to me whether the concerns of Dave, tim, sun, etc. have been addressed. Marking fixed for now, but we may need to further discuss.
Comment #35
sun@eff is effin' convincing, as effin' usual. ;)
However, we definitely need a change notice for the changed behavior, but also the new utility function here. :)
Comment #36
Wim LeersYes, we do!
I was awaiting conclusive feedback from you or others, sun, so now I can go ahead and create the change notice.
Comment #37
Wim LeersFinally wrote the change notice for this. It's also a change notice for #208611: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice at the same time.
See http://drupal.org/node/1911578.
Comment #39
quicksketchThis patch broke attachment of behaviors.
You'll notice that previously, there was a second parameter TRUE passed into the addCommand method. Now there's only one (the command itself). The second TRUE told addCommand() to PREPEND the the settings array, but now it's being APPENDed. This means that all new HTML that has been added to the page doesn't have the corresponding JS settings needed to properly attach their behaviors.
In practice, this is difficult to reproduce because I can't think of any places in core where we use AJAX to add new behaviors to a section of the page. I just ran into this in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms that my behaviors weren't getting added properly.
This patch restores the second parameter to fix AJAX attachment functionality.
Comment #40
quicksketchI suppose this should be needs review for test bot.
Comment #42
quicksketchSigh, missing trailing return. Testing again.
Comment #43
Wim Leers#39: HAH! Irony wants that this precise bug was introduced at #1812866: Rebuild the server side AJAX API and reported by me. I pointed the people who introduced that into the right direction by debugging it. Adding test coverage for this was deferred to #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended.
Even more ironic is that effulgentsia accidentally introduced this bug in #31, and he's the one who rolled #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended to prevent that from happening again.
I'm marking this issue as fixed again, because #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended contains the test coverage, which I just extended significantly to prevent this regression from ever occurring again: #1848880-2: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended.
Comment #44
quicksketchHey Wim, thanks for the update.
Not to be too picky, but could you avoid referencing the same issue more than once in a single comment? With the title's of issues so long and descriptive, it makes reading as a sentence difficult. Even if you want to point to a particular comment, the entire issue title multiple times is text-overload.
Comment #45
Wim LeersYeah, I hate that aspect of it too :/ I just very much prefer the linking directly to comments, because that makes things a lot easier. I guess the filter that provides this should only show the full title the first time, and afterwards omit the title. Thoughts?
Comment #46
quicksketchSmart! Let's discuss in #1929290: Display node title only the first time in issue filter if it seems like that is a good idea.
Comment #47
RdeBoerAfter porting a module from D7 to D8 I found that
drupal_add_js()
is overzealous in its merging of JS settings.If you call these 2 statements
multiple times, with different values in the $settings variable, then at the end of it all, you'll only have the settings from the last call left, instead of an array of all of them.
This wasn't so in D7.
For me, the attached patch fixed it. It may not be the best solution, but at least I have the old D7 behaviour back.
Comment #48
Wim LeersChange notice was posted at #37.
Comment #48.0
Wim LeersUpdated issue summary.
Comment #49
jamix CreditAttribution: jamix commented