Problem/Motivation
Configuration files make use of keys such as "404". When ingesting these values into an array PHP will automatically cast these keys to integer types. The default behaviour of NestedArray::mergeDeepArray(), which is used to merge new and overridden data into a Config object, causes the following result:
thingies:
0: Apples
1: Bananas
404: 404-page
herp: derp
more thingies:
0: Apples
1: Bananas
404: new-page
herp: derp
result:
0: Apples
1: Bananas
2: Apples
3: Bananas
4: 404-page
5: new-pag
herp: derp
desired result:
0: Apples
1: Bananas
404: new-page
herp: derp
Proposed resolution
- Add a new parameter to NestedArray::mergeDeepArray() which will cause integer keys to be preserved.
- Add a new method onto the Config class to facilitate merging in data correctly
- Update override methods in the Config class to use mergeDeepArray(x, TRUE)
- Add tests for the upgrade path and overridden data that use integer keys
Remaining tasks
None.
User interface changes
None.
API changes
- NestedArray::mergeDeepArray() accepts a second parameter to allow the preservation of integer keys, set to FALSE by default.
- Config class has a new merge($data) method.
Original report by @justafish
Following on from an issue shown in #1824762: Convert admin_compact_mode to CMI mergeDeepArray() does not preserve integer keys similarly to array_merge_recursive, which is causing issues when merging config objects that contain numeric keys.
The attached patch adds the ability to preserve keys.
---
Updated as far as #25
Comments
Comment #1
sunBefore we add a new parameter, I'd like to know what actually breaks if we diverge from array_merge_recursive()'s behavior.
Comment #3
alexpottHere's a patch that creates a new method on config objects to merge data in correctly - ie. preserving numeric keys and uses this during
update_variables_to_config()
.I think it could be quite difficult to predict all the impacts of changing the default behaviour of
NestedArray::mergeDeepArray()
. The test in #1 shows that we have a test to ensure this method behaves the same way as PHP's array_merge_recursive and changing this just for config feels risky.Comment #4
alexpottgo bot go!
Comment #5
alexpottTagging...
Comment #7
alexpottDrupal\system\Tests\Entity\EntityFormTest does not fail locally. Going to retest #3
Comment #8
alexpott#3: 1825466.config-merge.patch queued for re-testing.
Comment #9
BerdirOpened #1825568: Random test failure in Drupal\system\Tests\Entity\EntityFormTest for the EntityFormTest failure.
Comment #10
justafishI agree with @alexpott, diverging from array_merge_recursive() seems unnecessarily risky.
Comment #11
BerdirWondering if it'd be easier to follow the code if these were two separate methods? We're calling it statically anyway..
Comment #12
justafishI don't think this is worth splitting into two functions, the extra parameter is very self explanatory and it's a minor alteration within the block.
Comment #13
justafishPatch adds additional documentation from #1 to #3
Comment #14
BerdirShould be prefixed with a \ according to the new coding standards.
Can we have a quick re-roll for that, then it looks ready to me.
Comment #15
justafishComment #16
justafishComment #17
sunWe need test coverage for NestedArray here, and sadly, I suspect that tests will reveal that this change would introduce a regression.
Test these potential configuration values:
You will obviously say that the entire point of passing TRUE is to not expect that $expected array.
However,
Config::merge()
unconditionally passes TRUE, so this applies to all data in all config objects.So while we're solving the
page.403
+page.404
merge case, we're introducing a regression for all other indexed arrays.A potential - but very fragile - solution might be to check the
$array
for whether it has a zero index (since all sequences start with a 0 index in PHP), before applying the special merge behavior; i.e.:As apparent from the snippet, we should definitely still limit this special behavior to an optionally passed flag, since NestedArray is used in many different areas throughout core.
Comment #18
alexpottI agree that this solution is for config only... but imagine I add a config object like this:
and I merge in new data like this:
I don't ever want the result to be:
i.e. config merge is a special case... perhaps we shouldn't even be using the NestedArray class here.
In the above case I would want this to be the result of the merge...
Comment #19
alexpottAlso... the only time we have to deal with this is during update_variables_to_config... so perhaps we could put the function in update.inc.
Comment #20
justafishPreserving integer keys seems like a fairly useful addition to NestedArray. If we only ever will deal with merging config arrays during the update process, then moving the merge() method into update.inc seems sensible.
The potential solution in #17 seems like it would break the case we're trying to fix if the array happened to contain a 0 index. It also adds a really weird caveat/quirk to a function that should be generically useful. To illustrate with @alexpott's example:
Comment #21
justafishPatch shows overrides fail too with integer keys, overrides will end up with something like
Comment #22
justafishComment #23
justafishPatches attached with tests and fixes for overrides, merging and upgrades with integer keys. I've left merge() in the tests only patch, but set preserve_integer_keys to false to show the failures.
Comment #24
penyaskitoI think that every comment here has been addressed, and the issue has proper tests.
The only thing that I'm not convinced about is using NestedArray for this, like @alexpott pointed out. However, not doing so would be duplicating code, and that could be even worse.
So marking as RTBC.
Comment #25
tim.plunkett+1 for herpderp.
Comment #25.0
tim.plunkettupdate broken html
Comment #25.1
justafishAdding issue summary
Comment #26
webchickWell that is very silly. :)
Since this is basically fixing a bug, changing to a task so it's not held up by issue thresholds.
Committed and pushed to 8.x. Thanks!
Comment #28
Wim LeersRelated: #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings).
Comment #28.0
Wim LeersUpdated issue summary.
Comment #29
tstoecklerI think this should be backported to D7. I just hit a use-case in contrib-land and was perplexed that there was no $preserver_integer_keys arguments... I was so sure it existed! :-) It's really a minor change, 100% backwards-compatible, so I personally see no reason to do that.
In wanting to backport the change, however, I noticed that the comitted patch added the docs to the completely wrong place, to mergeDeep() where the argument doesn't even exist (vs. mergeDeepArray()). So let's fix that first before backporting.
Before someone brings up test coverage: The $preserve_integer_keys functionality is not tested at all currently and drupal_array_merge_deep() is not tested at all in D7. So that should be a separate issue (which we should do, though!).
Attached is a patch for D8 and one for D7.
Comment #30
tstoecklerShortening title a bit. The config part is irrelevant for D7 anyway...
Comment #31
tstoecklerOh, and the contrib use-case: #2205823: Options element corrupts integer keys of allowed values for list fields
Comment #32
alexpottThanks for fixing the documentation @tstoeckler
Comment #34
catchCommitted/pushed to 8.0.x, thanks!