Problem/Motivation
On module/theme/profile install, extension-provided configuration is assigned a hash representing the configuration as provided. Example:
_core:
default_config_hash: iQ350J-hr8qwdSGYvxmWOBBGtcyTwcZXMEMo4Z3lT_o
When you update configuration in the UI, the hash remains unchanged (this is an observation -- see comment #6, where this is tested). Also, configuration added via the UI, such as adding a new View or a new Search Page to the site, doesn't have a config hash (see comment #12, where this is tested).
Therefore, in this module, when we revert config (which is basically an operation that means "set the configuration to its current default value), we should make sure the config hash is retained, so that it's similar to having changed it in the UI to a new value (which happens to be the current provided value from the module). And when we import configuration in this module, we should not give it a config hash.
Testing (again see #6 and #12) revealed that for entity-based configuration, the module is already doing the right thing for revert, and for all configuration, it is doing the right thing for import. But for simple configuration items being reverted, the config hash is lost. We need to fix that.
Proposed resolution
a) In ConfigReverter::revert()
, make sure that simple config's hash is retained. Currently it isn't.
b) Write tests for both simple and entity-based config to test that upon reversion, the config hash is retained.
c) Write tests for both simple and entity-based config to test that upon import, the config hash is missing.
Remaining tasks
See Proposed Resolution.
User interface changes
None.
API changes
Config hashes will be retained on revert, and empty on import.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#15 | 2829506-config-revert-hashes-code-and-test.patch | 4.64 KB | jhodgdon |
| |||
#13 | 2829506-config-revert-hashes-TEST-ONLY.patch | 3.06 KB | jhodgdon |
Comments
Comment #2
jhodgdonGood catch! Sounds like something we need to do.
Comment #3
nedjoHmm, tested manually and it seemed the
_core
key is retained in a revert. Steps:Edit the
node_search
search page.Using the Configuration Update Report UI, revert
node_search
.Visit admin/config/development/configuration/single/export/search_page/node_search. Result: the
_core
key is present with adefault_config_hash
value.The attached test also passes for me locally--let's see what the test bot makes of it.
Comment #4
jhodgdonIt looks like the test passes... so we should probably (a) figure out why and (b) commit this test?
Comment #5
nedjoYes.
I'm curious about two things:
_core
is retained, woulduuid
also be retained? If so, can we remove the code currently copying overuuid
inConfigReverter::revert()
. If not, um, why not?_core.default_config_hash
represent a hash of the config as originally installed, or has it been refreshed to reflect a potentially changed state? Example: I installed module X, then customized (overwrote) config item Y, then updated module X to a new version that included a changed version of config item Y, then reverted. Has the hash been updated? If not, does it matter? Code in core seems to just detect the presence of the hash, without actually evaluating its value. Contrib might evaluate its value, for example, to determine if a new version of the configuration is available. (In fact, we might consider doing that.) Should we ourselves be updating the hash, even if a (potentially outdated) hash is present?But so far I'm evidently not curious enough to actually check any of this ;)
Comment #6
jhodgdonI looked into this a bit today. Here's what I did:
a) I had an existing dev site. Running 8.3.x, installed a few weeks ago. I turned on the Config Update Base and Reports modules.
b) I went to the Config Update report page and verified that the node_search Search Page config had not been modified.
c) I went to the single config export page and exported this config. The hash says:
d) I went to the Search settings page and modified the configuration for node search. Ran report, verified it came up as modified, looked at diffs on diffs page to verify it changed. Went to single config export page. The hash is the same, while other values have changed.
e) I used the Revert link to revert it. Went to single export page again. Config hash is still the same; other values are as they were originally.
So... why? Let's look at the Revert code. It does two different things, depending on if it's simple config or a config entity. The case I was looking at here was a config entity... in which case, this is what it does:
So it's calling the updateFromStorageRecord() method, which is
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!Entity!Con...
What that method does is an $entity->set() on each value that is in the passed-in data. So, it is not overriding anything that isn't in the passed-in data, meaning it doesn't override the __core value. Or uuid for that matter.
So that explains why this isn't a problem:
1. When you use the UI to update a config entity, the hash is not updated (this is an observation of what happens on a Search Page anyway).
2. When you call our Revert code from this module, it also doesn't override the hash.
We should also test on a simple config, because that has different code... the revert method in that case is calling
It looks like that could be a problem, because instead of going through and doing an entity set on the array keys/values, the method just puts $value in as the data for the config item.
So, let's see what happens in practice. I followed the above steps, except I looked at the simple config item called search.settings instead of the search page item called node_search.
In this case, before any changes are made, the config hash looks like this:
As before, editing the search settings in the UI didn't change the config hash. But reverting has removed the config hash.
So. We do need to fix this, but only for simple (non-entity) config. Probably all we need to do is add a few lines to the revert() method on ConfigReverter that will preserve the value of __core() from the existing config. And we need to add a few lines of testing for it too.
By the way, it looks like simple config doesn't have UUID values. I guess that makes sense, as they are not entities.
Anyway, setting to Needs Work, because the existing patch isn't sufficient to test the problem. By the way I think the test in the patch should be modified: it should not only verify that the UUID and core and hash text are present, but also verify that their values haven't changed. Then for the simple config, it should just check for the hash, not the UUID, since simple config doesn't apparently have UUID values.
Comment #7
jhodgdonComment #8
jhodgdonSo I'm also wondering if we should actually redo the config hash for both simple and entity config, rather than retaining its old value... the reason being that we cannot guarantee that the current file from the module/theme whose values we are "reverting" to is the same as what had been used to calculate the hash. I think that is actually the right thing to do: recalculate the hash in both cases.
Comment #9
jhodgdonI take back #8. Reverting should be like changing config through a UI. It should preserve the hash value that was there. So I think only simple config is problematic currently in this module.
Comment #10
jhodgdonI'm going to work on this today...
Comment #11
jhodgdonGuess I should start by updating the issue summary.
I realized in the process of doing this that we need to test and possibly fix the Import operation as well. I'll do the testing in a separate comment.
Comment #12
jhodgdonOK, I tested this similarly to comment #6 but for importing, using the User Search page:
a) Exported the User Search page to see what it looks like before any manipulation. It has a config hash:
b) Went to Search settings and deleted the User Search page.
c) Went to the config reports, ran the report for search pages, and imported the User Search page.
d) The imported config has no config hash. That seems wrong... but...
e) I deleted it again from the Search settings UI, and then used the Add feature of the Search settings UI to add a user search page. When I went to export that item, it also doesn't have a default config hash in it.
So... I think our Import code is OK. When we import using the button on the report, I think it should behave similarly to adding new config in the UI, and it does: neither one ends up with a config hash.
I'll update the issue summary with this conclusion.
Comment #13
jhodgdonHere's the test-only patch. Should have one failure (fails as expected locally, for the case of reverting a simple config item).
Comment #15
jhodgdonGood, that failed as expected.
Now here's the patch with the tests (see previous patch, no changes since then) and code change (in the ConfigReverter class). Tests now pass locally. If the test bot agrees, I'll commit this patch shortly.
Note: I also simplified the entity revert code, since (see comment #6) I realized that the uuid special casing wasn't necessary. And since we now have a test to verify that the uuid and hash do not change, we can see that this change doesn't break the code.
Comment #17
jhodgdonLooks good, committed!
Comment #18
nedjoThanks jhodgdon for tracking down and fixing these subtle issues. The complexity here is part of why module's API is a sound basis for others that have similar needs. See e.g. #2829512: Retain uuid and _core keys when replacing configuration.
Comment #19
jhodgdonHopefully the API is enough for most modules that need a "do stuff with configuration" API. If not, maybe those other modules will add issues here so we can add to our base API. :)