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

Files: 
CommentFileSizeAuthor
#29 1825466-29-nested-array-integer-keys-docs-d8.patch1.18 KBtstoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,719 pass(es).
[ View ]
#29 1825466-29-nested-array-integer-keys-d7-do-not-test.patch1.8 KBtstoeckler
#23 1825466-tests_config_merges_upgrades_overrides_integer_keys-23.patch7.5 KBjustafish
FAILED: [[SimpleTest]]: [MySQL] 47,999 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#23 1825466-fix_and_tests_config_merges_upgrades_overrides_integer_keys-23.patch11.38 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 47,990 pass(es).
[ View ]
#21 1825466-test_integer_key_overrides-21.patch3.66 KBjustafish
FAILED: [[SimpleTest]]: [MySQL] 48,000 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#15 1825466.nested-arrays-config-merge.15.patch6.23 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 46,437 pass(es).
[ View ]
#15 interdiff.txt688 bytesjustafish
#13 1825466.nested-arrays-config-merge.13.patch5.83 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 46,433 pass(es).
[ View ]
#13 interdiff.txt739 bytesjustafish
#3 1825466.config-merge.patch5.35 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 46,348 pass(es).
[ View ]
#3 1825466.config-merge.test-only.patch1.75 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 46,251 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#1 drupal8.nestedarray-mergedeep.1.patch1.06 KBsun
FAILED: [[SimpleTest]]: [MySQL] 46,302 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
nested.patch2.04 KBjustafish
PASSED: [[SimpleTest]]: [MySQL] 46,324 pass(es).
[ View ]

Comments

StatusFileSize
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] 46,302 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Before we add a new parameter, I'd like to know what actually breaks if we diverge from array_merge_recursive()'s behavior.

Status:Needs review» Needs work

The last submitted patch, drupal8.nestedarray-mergedeep.1.patch, failed testing.

Title:Allow NestedArray::mergeDeepArray() to preserve integer keysAllow NestedArray::mergeDeepArray() to preserve integer keys for the purpose of merging config data
StatusFileSize
new1.75 KB
FAILED: [[SimpleTest]]: [MySQL] 46,251 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.35 KB
PASSED: [[SimpleTest]]: [MySQL] 46,348 pass(es).
[ View ]

Here'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.

Status:Needs work» Needs review

go bot go!

Issue tags:+Configuration system

Tagging...

Status:Needs review» Needs work

The last submitted patch, 1825466.config-merge.test-only.patch, failed testing.

Drupal\system\Tests\Entity\EntityFormTest does not fail locally. Going to retest #3

Status:Needs work» Needs review

#3: 1825466.config-merge.patch queued for re-testing.

I agree with @alexpott, diverging from array_merge_recursive() seems unnecessarily risky.

Wondering if it'd be easier to follow the code if these were two separate methods? We're calling it statically anyway..

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

StatusFileSize
new739 bytes
new5.83 KB
PASSED: [[SimpleTest]]: [MySQL] 46,433 pass(es).
[ View ]

Patch adds additional documentation from #1 to #3

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -401,4 +401,19 @@ public function getStorage() {
+   * @return Drupal\Core\Config\Config
+   *   The configuration object.

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

StatusFileSize
new688 bytes
new6.23 KB
PASSED: [[SimpleTest]]: [MySQL] 46,437 pass(es).
[ View ]

Status:Needs work» Needs review

Issue tags:+Needs tests

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

<?php
$allowed_protocols
= array('http', 'https', 'ftp');
$new_allowed_protocols = array('gopher', 'mailto', 'ssh');
$result = NestedArray::mergeDeepArray(array($allowed_protocols, $new_allowed_protocols), TRUE);
$expected = array('http', 'https', 'ftp', 'gopher', 'mailto', 'ssh');
?>

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

if (is_integer($key)) {
  // If there's a zero index, we have a sequence; merge like array_merge_recursive().
  if (!$preserve_integer_keys || isset($array[0])) {
    $result[] = $value;
  }
  else {
    $result[$key] = $value;
  }
}
else {
  ...
}

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.

I agree that this solution is for config only... but imagine I add a config object like this:

thingies:
  0: Apple
  1: Banana
  2: Pear

and I merge in new data like this:
thingies:
  0: Apple
  1: Banana
  2: Pear
  3: Apricot

I don't ever want the result to be:

thingies:
  0: Apple
  1: Banana
  2: Pear
  3: Apple
  4: Banana
  5: Pear
  6: Apricot

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

thingies:
  0: Apple
  1: Banana
  2: Pear
  3: Apricot

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

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

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-page
  herp: derp
desired result:
  0: Apples
  1: Bananas
  404: new-page
  herp: derp

Assigned:Unassigned» justafish
StatusFileSize
new3.66 KB
FAILED: [[SimpleTest]]: [MySQL] 48,000 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Patch shows overrides fail too with integer keys, overrides will end up with something like

Array
(
    [foo] => overridden
    [baz] => injected
    [0] => derp
)

Status:Needs review» Needs work

Assigned:justafish» Unassigned
Status:Needs work» Needs review
StatusFileSize
new11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 47,990 pass(es).
[ View ]
new7.5 KB
FAILED: [[SimpleTest]]: [MySQL] 47,999 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

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

+1 for herpderp.

Issue summary:View changes

update broken html

Issue summary:View changes

Adding issue summary

Category:feature» task
Status:Reviewed & tested by the community» Fixed

Well 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!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Updated issue summary.

Title:Allow NestedArray::mergeDeepArray() to preserve integer keys for the purpose of merging config data[docs follow-up, then backport to D7] Allow NestedArray::mergeDeepArray() to preserve integer keys for the purpose of merging config data
Status:Closed (fixed)» Needs review
StatusFileSize
new1.8 KB
new1.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,719 pass(es).
[ View ]

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

Title:[docs follow-up, then backport to D7] Allow NestedArray::mergeDeepArray() to preserve integer keys for the purpose of merging config data[docs follow-up, then backport to D7] Allow NestedArray::mergeDeepArray() to preserve integer keys

Shortening title a bit. The config part is irrelevant for D7 anyway...