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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nedjo created an issue. See original summary.

jhodgdon’s picture

Good catch! Sounds like something we need to do.

nedjo’s picture

Status: Active » Needs review
FileSize
964 bytes

Hmm, 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 a default_config_hash value.

The attached test also passes for me locally--let's see what the test bot makes of it.

jhodgdon’s picture

It looks like the test passes... so we should probably (a) figure out why and (b) commit this test?

nedjo’s picture

we should probably (a) figure out why and (b) commit this test?

Yes.

I'm curious about two things:

  • Since apparently _core is retained, would uuid also be retained? If so, can we remove the code currently copying over uuid in ConfigReverter::revert(). If not, um, why not?
  • Does the resulting value of _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 ;)

jhodgdon’s picture

Status: Needs review » Needs work

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

_core:
  default_config_hash: 97tvtzGOa8_flb22CzSjgtm_YkiGMHvEBO-6q2K9V_U

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:

      $value = $this->extensionConfigStorage->read($full_name);
      $entity = $entity_storage->load($id);
      $uuid = $entity->get('uuid');
      $entity = $entity_storage->updateFromStorageRecord($entity, $value);
      $entity->set('uuid', $uuid);
      $entity->save();

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

      $this->configFactory->getEditable($full_name)->setData($value)->save();

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:

_core:
  default_config_hash: hvVxL1G-ZCxaq32IZws0YsfuhvaDiQE_np-0g35KjUk

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.

jhodgdon’s picture

Title: Regenerate default_config_hash on reverting » Simple config doesn't retain config hash when reverted
Category: Task » Bug report
jhodgdon’s picture

Title: Simple config doesn't retain config hash when reverted » Config hash wrong or missing after a revert

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

jhodgdon’s picture

Title: Config hash wrong or missing after a revert » Simple config doesn't retain config hash when reverted

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

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

I'm going to work on this today...

jhodgdon’s picture

Title: Simple config doesn't retain config hash when reverted » Simple config doesn't retain config hash when reverted
Issue summary: View changes

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

jhodgdon’s picture

Issue summary: View changes

OK, 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:

_core:
  default_config_hash: k3aUaZXGDuhkek2TZIee0PApOPTvYZLadziekdyHA5A

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.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
3.06 KB

Here's the test-only patch. Should have one failure (fails as expected locally, for the case of reverting a simple config item).

Status: Needs review » Needs work

The last submitted patch, 13: 2829506-config-revert-hashes-TEST-ONLY.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
4.64 KB

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

  • jhodgdon committed 332e1e3 on 8.x-1.x
    Issue #2829506 by jhodgdon, nedjo: Simple config does not retain config...
jhodgdon’s picture

Status: Needs review » Fixed

Looks good, committed!

nedjo’s picture

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

jhodgdon’s picture

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

Status: Fixed » Closed (fixed)

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