JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings)

I tried to be too fast:

1. There apparently was a test for this. Hence the test failure in #3. I did not find this before.
2. I added two real world test cases. To make these testable in a sane way, I also added parsing of 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.)
3. I didn't explain this clearly enough, but it seems that so far the 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.

#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 get drupalSettings.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.

#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.

From #208611-34: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice, ksenzee:

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.

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:

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 drupalSettings:

{myModule: {key: value}}

Before, you would have gotten

{myModule: {key: [value, value]}}

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:

drupal_add_js(array('myModule' => array('otherKey' => array('foo', 'bar'))), 'setting');
drupal_add_js(array('myModule' => array('otherKey' => array('foo', 'bar'))), 'setting');

the second call has no effect (as you would expect), and you get the following in drupalSettings:

{myModule: {otherKey: ['foo', 'bar']}}

Before, you would have gotten

{myModule: {otherKey: ['foo', 'bar', 'foo', 'bar']}}

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:

persister.js:

drupalSettings.persister.IDsToPersist = [];

foo.module:

drupal_add_js(array('myModule' => array('persister' => array('IDsToPersist' => array('foo-bar', 'foo-baz')))), 'setting');

bar.module:

drupal_add_js(array('myModule' => array('persister' => array('IDsToPersist' => array('bar-foo', 'bar-baz')))), 'setting');}

Then you would get:

drupalSettings.persister.IDsToPersist = ['foo-bar', 'foo-baz', 'bar-foo', 'bar-baz'];

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:

foo.module:

drupal_add_js(array('myModule' => array('persister' => array('IDsToPersist' => array('foo' => array('foo-bar', 'foo-baz'))))), 'setting');

bar.module:

drupal_add_js(array('myModule' => array('persister' => array('IDsToPersist' => array('bar' => array('bar-foo', 'bar-baz'))))), 'setting');

Then you would get:

drupalSettings.persister.IDsToPersist = {foo: ['foo-bar', 'foo-baz'], bar: ['bar-foo', 'bar-baz']};
drupalSettings.persister.IDsToPersist = mergeKeyedArrays(drupalSettings.persister.IDsToPersist);
drupalSettings.persister.IDsToPersist = ['foo-bar', 'foo-baz', 'bar-foo', 'bar-baz'];

with:

function mergeKeyedArrays(keyedArrays) {
  var mergedArray = [];
  for (key in keyedArrays) {
    if (keyedArrays.hasOwnProperty(key)) {
      mergedArray = mergedArray.concat(keyedArrays[key]);
    }
  }
  return mergedArray;
}

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.

#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.

#31: Great!

Splitting it out in a separate method to be able to document it more cleanly & clearly makes a lot of sense — thanks.

Yes, we do!

I was awaiting conclusive feedback from you or others, sun, so now I can go ahead and create the change notice.

Finally 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.

#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.

Yeah, 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?

Change notice was posted at #37.

Updated issue summary.