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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

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.

alexpott’s picture

Title: Allow NestedArray::mergeDeepArray() to preserve integer keys » Allow NestedArray::mergeDeepArray() to preserve integer keys for the purpose of merging config data
FileSize
1.75 KB
5.35 KB

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.

alexpott’s picture

Status: Needs work » Needs review

go bot go!

alexpott’s picture

Issue tags: +Configuration system

Tagging...

Status: Needs review » Needs work

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

alexpott’s picture

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

alexpott’s picture

Status: Needs work » Needs review

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

Berdir’s picture

justafish’s picture

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

Berdir’s picture

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

justafish’s picture

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.

justafish’s picture

Patch adds additional documentation from #1 to #3

Berdir’s picture

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.

justafish’s picture

justafish’s picture

Status: Needs work » Needs review
sun’s picture

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:

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

alexpott’s picture

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
alexpott’s picture

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.

justafish’s picture

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
justafish’s picture

Assigned: Unassigned » justafish
FileSize
3.66 KB

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

Array
(
    [foo] => overridden
    [baz] => injected
    [0] => derp
)
justafish’s picture

Status: Needs review » Needs work
justafish’s picture

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.

penyaskito’s picture

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.

tim.plunkett’s picture

+1 for herpderp.

tim.plunkett’s picture

Issue summary: View changes

update broken html

justafish’s picture

Issue summary: View changes

Adding issue summary

webchick’s picture

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.

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

tstoeckler’s picture

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
FileSize
1.8 KB
1.18 KB

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.

tstoeckler’s picture

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

tstoeckler’s picture

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Documentation

Thanks for fixing the documentation @tstoeckler

  • catch committed 3d98fc4 on 8.0.x
    Issue #1825466 by justafish, tstoeckler, alexpott, sun: [docs follow-up...
catch’s picture

Title: [docs follow-up, then backport to D7] Allow NestedArray::mergeDeepArray() to preserve integer keys » Allow NestedArray::mergeDeepArray() to preserve integer keys
Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Committed/pushed to 8.0.x, thanks!

  • catch committed 3d98fc4 on 8.1.x
    Issue #1825466 by justafish, tstoeckler, alexpott, sun: [docs follow-up...

  • webchick committed af608d7 on 8.3.x
    Issue #1825466 by justafish, sun, alexpott: Allow NestedArray::...
  • catch committed 3d98fc4 on 8.3.x
    Issue #1825466 by justafish, tstoeckler, alexpott, sun: [docs follow-up...

  • webchick committed af608d7 on 8.3.x
    Issue #1825466 by justafish, sun, alexpott: Allow NestedArray::...
  • catch committed 3d98fc4 on 8.3.x
    Issue #1825466 by justafish, tstoeckler, alexpott, sun: [docs follow-up...

  • webchick committed af608d7 on 8.4.x
    Issue #1825466 by justafish, sun, alexpott: Allow NestedArray::...
  • catch committed 3d98fc4 on 8.4.x
    Issue #1825466 by justafish, tstoeckler, alexpott, sun: [docs follow-up...

  • webchick committed af608d7 on 8.4.x
    Issue #1825466 by justafish, sun, alexpott: Allow NestedArray::...
  • catch committed 3d98fc4 on 8.4.x
    Issue #1825466 by justafish, tstoeckler, alexpott, sun: [docs follow-up...