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
Comment | File | Size | Author |
---|---|---|---|
#62 | 2923004-62.patch | 8.02 KB | tedbow |
#62 | interdiff-57-62.txt | 1.19 KB | tedbow |
#57 | 2923004-57.patch | 7.71 KB | tedbow |
#57 | interdiff-55-57.txt | 679 bytes | tedbow |
#55 | 2923004-55.patch | 7.79 KB | tedbow |
Comments
Comment #2
bircherComment #3
bircherOk actually tests are simple to add...
Comment #4
geek-merlinYes 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:
=>
Comment #5
bircherThanks 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.
Comment #6
borisson_I agree, a test-only patch doesn't make a lot of sense.
I only could find some a nitpick:
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.
Comment #7
alexpottComing 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...Comment #8
bircherYea that would be practical too.
I think
getOverrides($key = '')
makes most sense, since we have to merge the arrays anyway to check the keys andisOverridden($key = NULL)
is basically then justempty(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.
Comment #9
alexpottNice 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.
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.
Comment #10
bircher1) 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.
Comment #11
geek-merlinCorrect 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:
...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.
Comment #12
alexpottI 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.
Comment #13
tedbowre #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 toConfig
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
assomething like:
But it seems like that could an follow up if needed.
Comment #14
geek-merlin> getValueByStringKey()
Yes please!
Comment #15
alexpottre #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:
Comment #16
tedbowre #15 updated the patch to use the onliner everyone in Config and ConfigBase.
Also added the related issue where I hope to use this.
Comment #17
Wim LeersNit: "that apply to" -> "applied to"
(I think?)
There's no test coverage yet for this documented edge case.
s/overwritten/overridden/
Comment #18
tedbow\Drupal\Tests\Core\Config\ConfigTest::overrideDataProvider
Comment #19
Wim LeersSee #2910353-4: Prevent saving config entities when configuration overrides are applied + #2910353-6: Prevent saving config entities when configuration overrides are applied. AFAICT this is a blocker for that issue.
Comment #20
Wim LeersI think this is RTBC. But this still needs:
Once those exist, I'll RTBC.
Comment #21
tedbowComment #22
Wim LeersIS + CR look good.
One last look at the patch revealed one tiny nit:
Nit: s/configuration/configuration object/
(For consistency with all other method docs.)
Comment #23
tedbow@Wim Leers thanks, fixing the last nit.
Comment #24
borisson_Nit was fixed, this all looks very good now.
Comment #25
alexpottI 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.
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.
Comment #26
bircherActually 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 simplyforeach(\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
Comment #28
bircherups.. I guess I should have actually ran this locally first...
Comment #29
alexpottI 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 butsystem.site:page.403
andsystem.site:page.404
are not then:Comment #30
alexpottAlso 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.Comment #31
alexpottSo pondering some more of #30 maybe we are asking the wrong thing if configuration is overridden - maybe we need to ask the ConfigFactory?
Comment #32
geek-merlin> 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:
Any requirements left?
Comment #33
bircherThese 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.
Comment #34
alexpott@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.
Comment #35
tedbowOk here is a try at #29 by adding
hasOverrides()
instead ofgetOverrides()
I trying to follow the comments past that but not sure I see all the nuances that this can won't cover.
re #32
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 1configFactoryOverride
overrode the value of another this would not matter forhasOverrides()
.Would it make sense to add
hasOverrides()
now and then additionally in follow look into @axel.rutz's idea in #32 to implement something likegetOverrides()
when you need to know values also?Comment #36
borisson_/s/Determinse/Determine/
I think it makes sense to do this first, so we can solve the followups.
Comment #38
tedbow\Drupal\Tests\Core\Config\ConfigTest::testOverrideData
which caused test to fail.Comment #39
tedbowChanging 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. ButhasOverrides()
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.
Comment #40
Berdir$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.
Comment #41
BerdirComment #42
alexpott+1 to #40.
Comment #43
tedbow@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 doinggetOverrides()
in a follow up(if needed)Do you think we should do
getOverrides()
still instead? @alexpott same question since you were +1 on #40Comment #44
alexpott@tedbow I think what @Berdir was saying is that we should support sub keys - i.e
hasOverrides('foo.bar')
Comment #45
BerdirYes, 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()
Comment #46
samuel.mortensonThis patch adds sub key support per #40, and adds test coverage for this behavior.
Comment #47
tedbow@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.
So the comment for
\Drupal\Core\Config\Config::hasOverrides
still needs to be updated.I was working on this issued I was going with
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
Comment #48
tedbowFixing items in my review in #47
\Drupal\Core\Config\Config::hasOverrides()
$this->assertFalse($this->config->hasOverrides($non_overridden_key));
and$this->assertFalse($this->config->hasOverrides($nested_non_overridden_key));
keysComment #49
xjmThis 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?
Comment #50
tedbow@xjm thanks for the review
Updated the @param comment and change the default value to NULL.
Comment #51
alexpottWell 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:
so the empty and argument logic is consistent across hasOverrides(), get() and getOriginal().
Comment #52
tedbow#51 sounds good to me fix.
Fixing.
The only thing different from the suggestion is instead of:
if (!empty($key)) {
I changed it to
Since
\Drupal\Core\Config\Config::get()
and\Drupal\Core\Config\Config::getOriginal()
both haveif (empty($key)) {
and then a return statement.
Comment #53
tim.plunkettLooks pretty good to me!
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
These could be keyed with a plain English string explaining what exactly is being tested for each.
Not sure repeating these each time is helpful, but can't hurt
Asserts
Comment #54
Wim LeersIndentation of the second and third lines is off.
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!
I think making the second parameter not having a default value would result in clearer/less brittle test coverage.
"combined key" is not the correct term here. How do existing config tests/code describe this? We should describe it the same way here.
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.
Comment #55
tedbow@tim.plunkett and @Wim Leers thanks for the reviews
#53
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 checkis_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.#54
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 thathasOverrides()
produces the correct result.Comment #56
alexpottI 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.
Comment #57
tedbowOk @alexpott thanks for reviewing again. fixed #56
Comment #58
alexpottThanks @tedbow.
Comment #59
alexpottAfaics 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.
Comment #60
alexpottI've updated the change record to be about the new solution.
Comment #61
larowlanShould we document that a presence of a . in the key is taken to mean nesting?
On
::get
we have thisShould we document similar here so that it is clear?
Comment #62
tedbow@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".Comment #63
alexpottThe updated docs look good to me.
Comment #65
larowlanCommitted as 21b9f7d and pushed to 8.5.x.
Published change record.