Problem/Motivation

There is currently no convenient/good way to know if a config object is overwritten or not.
Simply comparing \Drupal\Core\Config\Config::getOriginal() with and without overrides does not reveal overrides when for example the override in settings.php happens to be of the same value.
Issues that showcase the need for it: #2408549: There is no indication on configuration forms if there are overridden values (which is marked as "Major" and "Bug report", hence the classification of this issue) or #2919373: Prevent Settings Tray functionality for blocks that have configuration overrides

Proposed resolution

Add a getOverrides() method on the \Drupal\Core\Config\Config object that can answer this question.

Remaining tasks

write patch add tests
review patch
commit
celebrate

User interface changes

none

API changes

New public method \Drupal\Core\Config\Config::getOverrides() that returns overrides applied to the configuration.

Data model changes

none

CommentFileSizeAuthor
#62 2923004-62.patch8.02 KBtedbow
#62 interdiff-57-62.txt1.19 KBtedbow
#57 2923004-57.patch7.71 KBtedbow
#57 interdiff-55-57.txt679 bytestedbow
#55 2923004-55.patch7.79 KBtedbow
#55 interdiff-52-55.txt6.93 KBtedbow
#52 2923004-52.patch7.23 KBtedbow
#52 interdiff-50-52.txt987 bytestedbow
#50 2923004-50.patch7.2 KBtedbow
#50 interdiff-48-50.txt966 bytestedbow
#48 2923004-48.patch7.07 KBtedbow
#48 interdiff-2923004-46-48.txt2.77 KBtedbow
#46 interdiff-2923004-38-46.txt2.11 KBsamuel.mortenson
#46 2923004-46.patch6.27 KBsamuel.mortenson
#38 2923004-38.patch5.39 KBtedbow
#38 interdiff-35-38.txt1.15 KBtedbow
#35 2923004-35.patch5.39 KBtedbow
#35 interdiff-26-35.txt10.04 KBtedbow
#28 2923004-28.patch9.14 KBbircher
#28 interdiff-2923004--23-28.txt2.34 KBbircher
#26 2923004-26.patch9.1 KBbircher
#26 interdiff-2923004--23-26.txt1.63 KBbircher
#23 2923004-23.patch9.17 KBtedbow
#23 interdiff-18-23.txt588 bytestedbow
#18 2923004-18.patch9.17 KBtedbow
#18 interdiff-17-18.txt1.66 KBtedbow
#16 2923004-16.patch8.89 KBtedbow
#16 interdiff-13-16.txt2.47 KBtedbow
#13 2923004-13.patch8.43 KBtedbow
#13 interdiff-10-13.txt2.4 KBtedbow
#10 2923004-10.patch9.64 KBbircher
#10 interdiff-2923004-8-10.txt2.35 KBbircher
#8 2923004-8.patch8.64 KBbircher
#8 interdiff-2923004-5-8.txt9.43 KBbircher
#5 interdiff-2923004-3-5.txt727 bytesbircher
#5 2923004-5.patch3.38 KBbircher
#3 2923004-3.patch3.52 KBbircher
#2 2923004-2.patch1.02 KBbircher
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bircher created an issue. See original summary.

bircher’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.02 KB
bircher’s picture

Issue summary: View changes
Issue tags: -Needs tests
FileSize
3.52 KB

Ok actually tests are simple to add...

geek-merlin’s picture

Status: Needs review » Needs work

Yes i can confirm this is major as of #2408549: There is no indication on configuration forms if there are overridden values.

If you add a test-only patch, everyone can see they work.

Code is trivial and straightforward.
That said, it can be simplified a lot:

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -141,6 +141,26 @@ public function setModuleOverride(array $data) {
+  public function hasOverrides() {
+    if (isset($this->moduleOverrides) && !empty($this->moduleOverrides)) {
+      return TRUE;
+    }
+    if (isset($this->settingsOverrides) && !empty($this->settingsOverrides)) {
+      return TRUE;
+    }
+    return FALSE;
+  }

=>

return !empty($this->moduleOverrides) || !empty($this->settingsOverrides);
bircher’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
727 bytes

Thanks for the review.
I updated the patch.

A test only-patch doesn't make a lot of sense in my opinion since the method is new. That said, the patch contains tests that assert the method returns the expected value in all cases.

borisson_’s picture

Status: Needs review » Needs work

I agree, a test-only patch doesn't make a lot of sense.

I only could find some a nitpick:

+++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
@@ -185,25 +185,38 @@ public function testOverrideData($data, $module_data, $setting_data) {
+    // Before overrides are set the configuration should indicate that.
+    $this->assertFalse($this->config->hasOverrides());

Not sure if this comment adds any value as it is right now.

I tried writing a comment to make the intention clear without duplication code and I don't know how. I'd suggest removing the comment.

alexpott’s picture

Coming from #2408549: There is no indication on configuration forms if there are overridden values where this is might be useful - I'm not sure that a hasOverrides() is exactly what we want. I think we want a getOverrides() that people can test for emptiness for the has case. This is because at least for #2408549: There is no indication on configuration forms if there are overridden values we need to know is specific parts of the config is overridden.

Maybe actually an isOverridden($key = NULL) is what we need. Where NULL would be the same as the hasOverrides() implementation atm but we'd also be able to determine this per sub key...

bircher’s picture

Title: Add hasOverrides() method to \Drupal\Core\Config\Config » Add method to check for overrides to \Drupal\Core\Config\Config
Status: Needs work » Needs review
FileSize
9.43 KB
8.64 KB

Yea that would be practical too.

I think getOverrides($key = '') makes most sense, since we have to merge the arrays anyway to check the keys and isOverridden($key = NULL) is basically then just empty(getOverrides($key)).

since the method would have been the fourth and third time the same code is repeated I made new protected methods for it.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigBase.php
    @@ -295,4 +283,33 @@ protected function castSafeStrings($data) {
    +  protected static function extractKeyedData(array $data, $key) {
    

    Nice use of a static. I like it here because it proves that we've making no changes to the object or its data. Also the way the PHP handles arrays - ie. it only copies on write means that there shouldn't be any memory or performance cost of this.

  2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -215,7 +227,17 @@ public function testOverrideData($data, $module_data, $setting_data) {
           $config_value = $this->config->getOriginal($key);
           $this->assertEquals($value, $config_value);
    +      $this->assertEquals($value, $this->config->getOverrides($key));
    

    This doesn't seem the best test of this because it looks like overrides are exactly the same as the original values? Ah getOriginal applies overrides... i see. It would be nice to have a test of $this->config->getOverrides($key) - where $key is in the config but not overridden.

bircher’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
9.64 KB

1) Thanks for liking the static method, I also think it is a good use for it.

2) Yes you are right that would be good, lets see if something breaks if we add more test data.

geek-merlin’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigBase.php
@@ -295,4 +283,33 @@ protected function castSafeStrings($data) {
+  protected static function extractKeyedData(array $data, $key) {
+    if (empty($key)) {
+      return $data;
+    }
+    else {
+      $parts = explode('.', $key);
+      if (count($parts) == 1) {
+        return isset($data[$key]) ? $data[$key] : NULL;
+      }
+      else {
+        $value = NestedArray::getValue($data, $parts, $key_exists);
+        return $key_exists ? $value : NULL;
+      }
+    }
+  }

Correct me if i'm wrong (when i mentally play all code paths i get to):
I think this is overly complex and boils down to:

protected static function extractKeyedData(array $data, $key) {
  return NestedArray::getValue($data, $key ? explode('.', $key) : []);
}

...which may or may not deserve its own function... ;-)

If yes, funny that this code copy-creeped all over the class in the first place.

alexpott’s picture

I think @axel.rutz is correct that the two code snippets are functionally equivalent. Nice find. Given #11 how about we file a patch to replace the complexity with the new snippet and get back to this one adding new functionality? Because I think once it is a one-liner then there is no need for a method on ConfigBase.

tedbow’s picture

re #12 if we are not going to add a method onto ConfigBase doesn't it become out of scope to change that class at all?

I think simplifying extractKeyedData() makes sense and then move it to Config because there it will be called 3 times.

I think if wanted to move the function elsewhere it seems it could live on \Drupal\Component\Utility\NestedArray as

getValueByStringKey()

something like:

  public static function getValueByStringKey(array &$array, $key = '', &$key_exists = NULL) {
    return self::getValue($array, $key ? explode('.', $key) : [], $key_exists);
  }

But it seems like that could an follow up if needed.

geek-merlin’s picture

> getValueByStringKey()

Yes please!

alexpott’s picture

re #13 I think it is odd that ConfigBase::get() and Config::get() don't use the same method to get the data. We could just change ConfigBase::get() to use the new one liner. In many ways I'd be happy just to replace the standard getting code with @axel.rutz's one liner and not have the new method on either ConfigBase or Config.

Wrt. to NestArray::getValueByStringKey() this could be explored in a separate issue. It should also have a delimiter argument so that it can be used by code like this:

          $parents = explode('][', $child['#group']);
          $group_element = NestedArray::getValue($form, $parents);
tedbow’s picture

re #15 updated the patch to use the onliner everyone in Config and ConfigBase.

Also added the related issue where I hope to use this.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -141,6 +129,27 @@ public function setModuleOverride(array $data) {
    +   * Returns overrides that apply to this configuration.
    

    Nit: "that apply to" -> "applied to"

    (I think?)

  2. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -141,6 +129,27 @@ public function setModuleOverride(array $data) {
    +   * Simply comparing the the data from getOriginal() with and without overrides
    +   * does not help to detect overrides if the override happens to have the same
    +   * value as the original data.
    

    There's no test coverage yet for this documented edge case.

  3. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -280,27 +283,28 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
    +   *   The overwritten data, unchanged if no overrides apply.
    

    s/overwritten/overridden/

tedbow’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.66 KB
9.17 KB
  1. I think your change is correct because it denotes that they are currently applied not could apply. Fixed
  2. Nice catch! Added test coverage for the overrides having the same by adding the test case to \Drupal\Tests\Core\Config\ConfigTest::overrideDataProvider
  3. Fixed
Wim Leers’s picture

I think this is RTBC. But this still needs:

  1. a change record to announce the API addition
  2. an updated issue summary to list the API addition

Once those exist, I'll RTBC.

tedbow’s picture

Issue summary: View changes
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs issue summary update

IS + CR look good.

One last look at the patch revealed one tiny nit:

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -141,6 +129,27 @@ public function setModuleOverride(array $data) {
+   * Returns overrides applied to this configuration.

Nit: s/configuration/configuration object/

(For consistency with all other method docs.)

tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
588 bytes
9.17 KB

@Wim Leers thanks, fixing the last nit.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Nit was fixed, this all looks very good now.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -141,6 +129,27 @@ public function setModuleOverride(array $data) {
+  public function getOverrides($key = '') {
+    $overrides = $this->mergeOverridesWithData([]);
+    if (empty($overrides)) {
+      return NULL;
+    }
+    return NestedArray::getValue($overrides, $key ? explode('.', $key) : []);
+  }

I think we might need a different behaviour if the key is empty and there are no overrides. Look at the following behaviour for an empty config object.

Psy Shell v0.8.0 (PHP 7.1.0 — cli) by Justin Hileman
>>> \Drupal::config('foo.bar')->getOverrides();
=> null
>>> \Drupal::config('foo.bar')->get();
=> []
>>> \Drupal::config('foo.bar')->get('foo');
=> null
>>> \Drupal::config('foo.bar')->getOverrides('foo');
=> null

In the instance of getting a root config value it can only be a mapping (or sometimes erroneously a sequence) so it might make more sense for it to be an empty array - so that it matches the behaviour of get(). Tricky. I can see the argument that NULL means no overrides being valid too.

I'm not going to un-RTBC this for this - just posting it here so I can mull on it a bit longer and for others to have a think too.

bircher’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.63 KB
9.1 KB

Actually at first I made it return NULL if not overwritten.
But from a DX perspective it makes sense to return an empty array.
After all to check that there are overrides you can simply do empty(\Drupal::config('foo.bar')->getOverrides()) but when looping over overrides one can simply foreach(\Drupal::config('foo.bar')->getOverrides() as $key => $override) {} which is better than having to check for empty first.

Also it makes it a one liner :D

Status: Needs review » Needs work

The last submitted patch, 26: 2923004-26.patch, failed testing. View results

bircher’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
9.14 KB

ups.. I guess I should have actually ran this locally first...

alexpott’s picture

Status: Needs review » Needs work

I was thinking about this some more and I'm not sure that the current implementation is quite right. The problem is that you might want to override something to a NULL value or an entire config object to an empty array. There I think we should implement hasOverrides($key) and return TRUE or FALSE is there is an applicable override. This should also work for sub keys. So for example, if system.site:page.front is overridden but system.site:page.403 and system.site:page.404 are not then:

$config = \Drupal::config('system.site');
$config->hasOverrides(); // returns TRUE;
$config->hasOverrides('mail'); // returns FALSE;
$config->hasOverrides('page'); // returns TRUE;
$config->hasOverrides('page.front'); // returns TRUE;
$config->hasOverrides('page.404'); // returns FALSE;
$config->hasOverrides('page.403'); // returns FALSE;
alexpott’s picture

Also I think there are complications around whether you are working with a configuration object where it's not being loaded with overrides. For example, if there is an override of system.site:page.front if you load the editable version of the configuration doing \Drupal::configFactory()->getEditable('system.site') to overrides are not going to be loaded - the configuration is not override aware are all in that instance.

alexpott’s picture

So pondering some more of #30 maybe we are asking the wrong thing if configuration is overridden - maybe we need to ask the ConfigFactory?

geek-merlin’s picture

> maybe we need to ask the ConfigFactory?

I agree that we cannot solve this only in Config...

Let's remember from the IS:

> Simply comparing \Drupal\Core\Config\Config::getOriginal() with and without overrides does not reveal overrides when for example the override in settings.php happens to be of the same value.

And it's even more complicated. Imagine we have to overrides that cancel each other.

Looking into the code, as \Drupal\Core\Config\ConfigFactory::loadOverrides does squash all overrides into one, that information is lost.

So i guess wee need to go deeper.
What if any (editable or immutable) configuration objects carries its overrides and can be queried for it
And let's dream matured overrides in code:

class Overrides {
  function add(Override $override): void
  function getAll(): Override[]
  function filterByKey(string $key): Override[]
  function applyTo(array $data):array
}
class Override {
  function __construct(string $source, array $data)
  function hasKey(string $key): bool
  function applyTo(array $configData):array
}

Any requirements left?

bircher’s picture

These are good points raised.

I would make sense to be able to get the overrides from the factory indeed because that would then also apply to config entities. (instead of doing $overrides = \Drupal::config($block->getEntityType()->getConfigPrefix() . '.' . $block->id())->getOverrides();)

However, when thinking about improving the overrides system and to answer 29. Maybe we should also have some way to then check the overrides validity. Or at least add some warning when the overridden config does not pass the schema. For example overriding a config object with an empty array may break things. And just to give another argument that we should do that then in a followup is that overrides are sometimes cached, for example overrides to config for plugins. But maybe something to keep in mind for future iterations.

alexpott’s picture

@bircher well I don't think we should be validating the overrides that's just not possible - overrides can be dynamic - it's the responsibility of whatever is doing the overriding to make sure it is valid. The problem is that I don't think we should mix getting an overridden value with determining if something is overridden. Because NULL, empty anything, and FALSE are all valid configuration values.

tedbow’s picture

Status: Needs work » Needs review
FileSize
10.04 KB
5.39 KB

Ok here is a try at #29 by adding hasOverrides() instead of getOverrides()

I trying to follow the comments past that but not sure I see all the nuances that this can won't cover.

re #32

And it's even more complicated. Imagine we have to overrides that cancel each other.

Looking into the code, as \Drupal\Core\Config\ConfigFactory::loadOverrides does squash all overrides into one, that information is lost.

I see that information could be lost as far as far override values but is it true that would not lose which keys had any override value.

If that is true then I think hasOverrides() would be good enough for the related issues.

#2408549: There is no indication on configuration forms if there are overridden values and #2919373: Prevent Settings Tray functionality for blocks that have configuration overrides

I think in both those cases we just need to know if key is being overridden and if in \Drupal\Core\Config\ConfigFactory::loadOverrides if 1 configFactoryOverride overrode the value of another this would not matter for hasOverrides().

Would it make sense to add hasOverrides() now and then additionally in follow look into @axel.rutz's idea in #32 to implement something like getOverrides() when you need to know values also?

borisson_’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -303,4 +303,20 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
+   * Determinse if there are overrides applied to this configuration object.

/s/Determinse/Determine/

I think it makes sense to do this first, so we can solve the followups.

Status: Needs review » Needs work

The last submitted patch, 35: 2923004-35.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
5.39 KB
  1. Fixed spelling in #36
  2. Update @covers in \Drupal\Tests\Core\Config\ConfigTest::testOverrideData which caused test to fail.
tedbow’s picture

Title: Add method to check for overrides to \Drupal\Core\Config\Config » Add method to check if any overrides are applied to \Drupal\Core\Config\Config

Changing the title to reflect the change to hasOverrides().

We can make a follow up to handle getOverrides() which is more complex re #32 but we should first determine if that would be needed for any of the related issues.

Would any of the current issues that will be blocked by this issue actually need getOverrides()?

#2919373: Prevent Settings Tray functionality for blocks that have configuration overrides only needs hasOverrides()

The current patch for #2408549: There is no indication on configuration forms if there are overridden values does require getOverrides() because if the user has the permission it would show the actual overridden keys. But hasOverrides() would provide at least some indication in configuration forms where there is none now.

I am not sure #2910353: Prevent saving config entities when configuration overrides are applied is far enough along to determine which one it would need.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -303,4 +303,20 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
+   * @param string $key
+   *   A string that maps to a key within the configuration data.
...
+    if ($key) {
+      return isset($this->moduleOverrides[$key]) || isset($this->settingsOverrides[$key]);
+    }

$key here doesn't have the same behavior as for other methods, shouldn't this also use NestedArray? Which in this case would mean NestedArray::keyExists()

So that you can use hasOverrides('foo.bar') just like with get()/set().

The description of the method also talks just about about configuration object overrides not of a specific key.

I'm thinking about #2910353: Prevent saving config entities when configuration overrides are applied and I think having hasOverrides() there would be enough, although I'm still not quite sure how to actually resolve that.

Berdir’s picture

Status: Needs review » Needs work
alexpott’s picture

+1 to #40.

tedbow’s picture

Status: Needs work » Needs review

@Berdir the code your referenced in #40 is not in the latest patches in #35 and #38 where I was suggesting(see #35 and #39 for reasoning) first in this issue doing hasOverrides() and doing getOverrides() in a follow up(if needed)

Do you think we should do getOverrides() still instead? @alexpott same question since you were +1 on #40

alexpott’s picture

@tedbow I think what @Berdir was saying is that we should support sub keys - i.e hasOverrides('foo.bar')

Berdir’s picture

Yes, I was talking about hasOverrides(), which also has a $key argument that should be consistent with other methods having a $key argument IMHO. Plus the documentation stuff.

And I'm pretty sure my code snippet is from the last patch, that code is in hasOverrides()

samuel.mortenson’s picture

This patch adds sub key support per #40, and adds test coverage for this behavior.

tedbow’s picture

Status: Needs review » Needs work

@Berdir @alexpott sorry about my comment in #43 I got throw off because I saw in the code snippet in #40
public function getOriginal
which wasn't changed method.

@samuel.mortenson thanks for the updated patch in #46 this look like what @Berdir wanted.

  1. The only other thing I see is @Berdir comment in #40

    The description of the method also talks just about about configuration object overrides not of a specific key.

    So the comment for \Drupal\Core\Config\Config::hasOverrides still needs to be updated.

    I was working on this issued I was going with

    Determines if overrides are applied to a key for this configuration object.

  2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -614,8 +634,12 @@
    +      foreach ($overridden_data as $key => $value) {
    +        // If there are nested overrides test a combined key.
    +        if (is_array($value)) {
    +          $key = $key . '.' . key($value);
    +        }
    +        $this->assertTrue($this->config->hasOverrides($key));
    

    This tests for overrides in nested keys 👍

    Should we also do this logic for the $non_overridden_keys?

    And add a test case with nested value that is not overridden?

otherwise looks good to me

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
7.07 KB

Fixing items in my review in #47

  1. Added the update function comment for \Drupal\Core\Config\Config::hasOverrides()
  2. Did #47.2 add test cases where there is only one type override and then neither. Then testing both $this->assertFalse($this->config->hasOverrides($non_overridden_key)); and $this->assertFalse($this->config->hasOverrides($nested_non_overridden_key)); keys
xjm’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -303,4 +303,28 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
+   * @param string $key
+   *   A string that maps to a key within the configuration data.
+   *
+   * @return bool
+   *   TRUE if there are any overrides for the key, otherwise FALSE.

This should probably indicate that if it's empty, we return TRUE if there are any overrides at all.

Also, any particular reason to use empty string rather than NULL?

tedbow’s picture

@xjm thanks for the review

Updated the @param comment and change the default value to NULL.

alexpott’s picture

Well the $key = '' was aligned with \Drupal\Core\Config\Config::get() and \Drupal\Core\Config\Config::getOriginal() which I think has some merit in being consistent. That said, I think once we get scalar typehints the correct signature for all of these methods would be along the lines of:
public function get(string $key = NULL) { but changing everything to that is probably a D9 change.

I think consistency should probably win out and we should do:

  public function hasOverrides($key = '') {
     if (!empty($key)) {

so the empty and argument logic is consistent across hasOverrides(), get() and getOriginal().

tedbow’s picture

#51 sounds good to me fix.

Fixing.

The only thing different from the suggestion is instead of:
if (!empty($key)) {
I changed it to

if (empty($key)) {
  return !empty($this->moduleOverrides) || !empty($this->settingsOverrides);
}

Since \Drupal\Core\Config\Config::get() and \Drupal\Core\Config\Config::getOriginal() both have
if (empty($key)) {
and then a return statement.

tim.plunkett’s picture

Looks pretty good to me!

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -303,4 +303,32 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
    +      return !empty($this->moduleOverrides) || !empty($this->settingsOverrides);
    ...
    +      if (isset($this->moduleOverrides) && is_array($this->moduleOverrides)) {
    ...
    +      if (!$override_exists && isset($this->settingsOverrides) && is_array($this->settingsOverrides)) {
    

    Super nit, but seeing !empty and isset checks mixed on the same variables makes me wonder why there's a difference. If the difference is important, please include a comment

  2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -449,6 +473,85 @@ public function overrideDataProvider() {
    +      [
    ...
    +      [
    ...
    +      [
    ...
    +      [
    

    These could be keyed with a plain English string explaining what exactly is being tested for each.

  3. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -449,6 +473,85 @@ public function overrideDataProvider() {
    +        // Original data.
    ...
    +        // Module overrides.
    ...
    +        // Setting overrides.
    

    Not sure repeating these each time is helpful, but can't hurt

  4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -545,4 +648,38 @@ public function testSafeStringHandling() {
    +   * Assert that the correct keys are overridden.
    

    Asserts

Wim Leers’s picture

Status: Needs review » Needs work
Related issues: +#2862248: UnitTestCase::getConfigFactoryStub() doesn't accept dotted keys
  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -303,4 +303,32 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
    +   *   (optional) A string that maps to a key within the configuration data. If
    +   *    not supplied TRUE will be returned if there are any overrides at all for
    +   *    this configuration object.
    

    Indentation of the second and third lines is off.

  2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -449,6 +473,85 @@ public function overrideDataProvider() {
    +          'a' => [
    +            'b' => 'originalValue'
    
    @@ -545,4 +648,38 @@ public function testSafeStringHandling() {
    +    foreach ($non_overridden_keys as $non_overridden_key) {
    +      $this->assertFalse($this->config->hasOverrides($non_overridden_key));
    +      // If there are nested overrides test a combined key also.
    +      if (is_array($overridden_data)) {
    

    AFAICT this doesn't work for arbitrarily nested config yet, only for two levels.

    If this also used NestedArray just like the actual code, then that would work.

    I suggest adding one obscenely nested test case, with … 10 levels of nesting?

    This is the one untested edge case that the code now supports since the last RTBC. Hence I'm observing this gap in the test coverage. If everybody else thinks this isn't worth testing: fine by me!

  3. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -545,4 +648,38 @@ public function testSafeStringHandling() {
    +  protected function assertOverriddenKeys(array $data, array $overridden_data = []) {
    

    I think making the second parameter not having a default value would result in clearer/less brittle test coverage.

  4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -545,4 +648,38 @@ public function testSafeStringHandling() {
    +        // If there are nested overrides test a combined key.
    

    "combined key" is not the correct term here. How do existing config tests/code describe this? We should describe it the same way here.

  5. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -545,4 +648,38 @@ public function testSafeStringHandling() {
    +    $non_overridden_keys = array_diff(array_keys($data), array_keys($overridden_data));
    

    This only returns non-overridden top-level keys.

Tangentially related:: Hah, #40 reminds me of #2862248: UnitTestCase::getConfigFactoryStub() doesn't accept dotted keys, which is an RTBC issue I encountered earlier today. That's touching nearly identical logic.

tedbow’s picture

Status: Needs work » Needs review
FileSize
6.93 KB
7.79 KB

@tim.plunkett and @Wim Leers thanks for the reviews

#53

  1. Changing the return statement in the case of empty($key) to:
    return (!empty($this->moduleOverrides) && is_array($this->moduleOverrides)) || (!empty($this->settingsOverrides) && is_array($this->settingsOverrides));
    I didn't notice that this class alwasy checks if these are arrays. I don't want to check isset() because we would still have to check is_array() and !empty(). I don't know what these could be if they were arrays but since the classes assumes they arrays to get the overrides if they are non-empty non-arrays when can't get determine overrides so this should return false.
  2. Removing these extra test cases
  3. No longer adding this test cases see #54.2 fix below
  4. Fixed

#54

  1. Fixed
  2. This made look at the test cases this patch added. I think we actually don't need new test cases exactly just the same just cases we already have with nested values.

    So I added loop to the end of overrideDataProvider() and for each of the existing tests cases I duplicated them except with all the values being nested 5 levels down.

    Then in assertOverriddenKeys() I added a loop that goes down through every level of the array and checks to make sure that hasOverrides() produces the correct result.

  3. Fixed
  4. Removed b/c of changes for 2)
  5. Now it goes down to all levels after getting the top level keys
alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -303,4 +303,32 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
+      return (!empty($this->moduleOverrides) && is_array($this->moduleOverrides)) || (!empty($this->settingsOverrides) && is_array($this->settingsOverrides));

I think the is_array() checks are a bit superfluous here. These properties are either NULL or an array. These values can only be set by setSettingsOverride() and setModuleOverride() both of which only accept arrays. For me it is easier to understand:
return !(empty($this->moduleOverrides) && empty($this->settingsOverrides))

Otherwise this looks great.

tedbow’s picture

Ok @alexpott thanks for reviewing again. fixed #56

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @tedbow.

alexpott’s picture

Afaics all the feedback on issue has been addressed and we've got a solution that works for the use-cases we have so far. Crediting all the reviewers because everyone has added to the direction of this patch.

alexpott’s picture

I've updated the change record to be about the new solution.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -303,4 +303,32 @@ public function getOriginal($key = '', $apply_overrides = TRUE) {
+   *   (optional) A string that maps to a key within the configuration data. If
+   *   not supplied TRUE will be returned if there are any overrides at all for
+   *   this configuration object.

Should we document that a presence of a . in the key is taken to mean nesting?

On ::get we have this

  *   @code
   *   array(
   *     'foo' => array(
   *       'bar' => 'baz',
   *     ),
   *   );
   *   @endcode
   *   A key of 'foo.bar' would return the string 'baz'. However, a key of 'foo'
   *   would return array('bar' => 'baz').
   *   If no key is specified, then the entire data array is returned.

Should we document similar here so that it is clear?

tedbow’s picture

@larowlan ok. that makes sense I copied the @param comment from \Drupal\Core\Config\ConfigBase::get(key) and adjusted it, changing, "would return" to "would map to".

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

The updated docs look good to me.

  • larowlan committed 21b9f7d on 8.5.x
    Issue #2923004 by tedbow, bircher, samuel.mortenson, alexpott, Wim Leers...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 21b9f7d and pushed to 8.5.x.

Published change record.

Status: Fixed » Closed (fixed)

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