I had a brief discussion with David about some of the reload issues this week at the d.o sprint here in Portland. Our talk was centered around the whole issue of 'what happens when something on prod has been overridden since last reload' but it is possible that it might help in the other issues as well.

Basically the idea is that whenever we reload config, we take that a snapshot of that config and save it away somewhere else. What this gives us at any point in time is the following:

1) The config in the files
2) The config in the active store
3) The config that was last reloaded

This means we can actually determine not only that the active store has drifted since last reload, but also in what way it has drifted. It allows us a little more detail too. If I change a setting on prod, then decide to revert it, the active store is still the same as the last reload even though it was at some point modified. We couldn't know this with the simple flag solution. This offers the opportunity for contrib to do some really cool 3-way diff stuff to determine how and where config has changed. While this does add some complexity to our solution, I really like it a lot.

CommentFileSizeAuthor
#87 83-87-interdiff.txt8.04 KBalexpott
#87 1515312-config-import-snapshots-87.patch12.44 KBalexpott
#83 81-83-interdiff.txt5.06 KBalexpott
#83 1515312-config-import-snapshots-83.patch11.87 KBalexpott
#81 79-81-interdiff.txt5.75 KBalexpott
#81 1515312-config-import-snapshots-81.patch9.09 KBalexpott
#79 1515312-config-import-snapshots-79.patch7.29 KBgdd
#77 1515312-config-import-snapshots-77.patch7.31 KBgdd
#75 1515312-config-import-snapshots-75.patch6.27 KBgdd
#73 1515312-config-import-snapshots-73.patch8.45 KBgdd
#68 1515312-config-import-snapshots-68.patch6.89 KBgdd
#63 1515312-config-import-snapshots-63.patch9.69 KBgdd
#63 interdiff-61-63.txt7.08 KBgdd
#61 drupal8.config-snapshot.61.patch12.5 KBsun
#61 interdiff.txt1.7 KBsun
#60 1515312-config-import-snapshots-60.patch12.24 KBgdd
#56 cmi-snapshot-1515312-56.patch12.36 KBAnonymous (not verified)
#53 1515312-config-import-snapshots-53.patch12.25 KBgdd
#52 1515312-config-snapshot.patch12.45 KBAnonymous (not verified)
#44 1515312-config-import-snapshots-44.patch259.12 KBgdd
#44 interdiff_38-44.txt1.49 KBgdd
#38 1515312-config-import-snapshots-36.patch12.26 KBgdd
#38 interdiff-35-36.txt7.79 KBgdd
#35 config-1515312-35.patch4.98 KBxjm
#32 config-1515312-32.patch4.87 KBxjm
#32 interdiff.txt751 bytesxjm
#30 1515312-config-import-snapshots-30.patch4.9 KBgdd
#28 1515312-config-import-snapshots-28.patch4.33 KBgdd
#24 1515312-config-import-snapshots-24.patch5.74 KBgdd
#16 1515312-16-snapshot.patch5.75 KBAnonymous (not verified)
#13 1515312-13-snapshot.patch5.87 KBrealityloop
#10 1515312-10-snapshot.patch5.8 KBAnonymous (not verified)
#9 1515312-9-snapshot.patch5.84 KBAnonymous (not verified)
#7 1515312-7-snapshot.patch5.03 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Issue tags: +Configuration system

.

gdd’s picture

Title: Add a flag to the active store to track differences from canonical storage » Add snapshots of last loaded config for tracking whether config has changed

A solution for this was proposed in #1447686: Allow importing and synchronizing configuration when files are updated that I think makes a lot more sense. Updated issue summary with such.

sun’s picture

Status: Active » Closed (duplicate)

This issue/discussion seems to duplicate #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs now.

The essential conclusion thus far is that configuration staging is more complex than diffing two or three snapshots (although that depends on the "type" of configuration), and that we even have huge troubles to determine the proper storage/location for the "second" snapshot in a two-snapshot diff.

Let's centralize the discussion in that issue.

gdd’s picture

Status: Closed (duplicate) » Active
Issue tags: +VDC

Now that the architecture has settled down I'd like to reopen this issue. The Views team is in particular quite interested in this functionality, and if it doesn't get in then it is a feature regression for them.

tim.plunkett’s picture

sun’s picture

Category: task » feature
Priority: Major » Normal

Given that we have #1741804: Implement a config import/staging directory and #1702080: Introduce canonical FileStorage + (regular) cache now,

I don't really get what a third snapshot would make possible that isn't possible today already?

There also seems to be quite some overlap with #1642062: Add TempStore for persistent, limited-term storage of non-cache data

(In light of this, let's demote this back to a feature; there's no reason to block other patches from landing)

Anonymous’s picture

Status: Active » Needs review
FileSize
5.03 KB

here's a first cut at a patch to get the discussion going.

Status: Needs review » Needs work

The last submitted patch, 1515312-7-snapshot.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.84 KB

same patch as #7, but make sure to create a snapshot dir for tests.

Anonymous’s picture

FileSize
5.8 KB

rerolled to chase HEAD.

realityloop’s picture

Issue tags: -Configuration system, -VDC

#10: 1515312-10-snapshot.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +VDC

The last submitted patch, 1515312-10-snapshot.patch, failed testing.

realityloop’s picture

Status: Needs work » Needs review
FileSize
5.87 KB

rerolled patch, untested

realityloop’s picture

Issue tags: -Configuration system, -VDC

#13: 1515312-13-snapshot.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system, +VDC

The last submitted patch, 1515312-13-snapshot.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.75 KB

rerolled to keep up with head, lets see what the bot thinks.

gdd’s picture

Issue tags: +feature freeze

I am assuming this would have to go in before feature freeze, tagging as such.

I'm curious why we use FileStorage for the snapshot instead of a database table. Seems like the latter would make more sense. This data never needs to be staged, and having a third directory in the config area could be confusing for users. This isn't a critical performance path or anything.

+++ b/core/includes/install.incundefined
@@ -262,6 +262,9 @@ function drupal_install_config_directories() {
+        CONFIG_SNAPSHOT_DIRECTORY => array(
+          'path' => 'config/snapshot_' . drupal_hash_base64(drupal_random_bytes(55)),

With #1821172: Hash the config directory name, unhash the active and staging directory names in place, this should be changed to be the hashed config directory . '/snapshot'

gdd’s picture

Status: Needs review » Needs work
mitchell’s picture

gdd’s picture

I think I can safely say we're not going to be adding revisions to FileStorage anytime soon.

mitchell’s picture

Would it be that hard to do, or is a GitStorage Controller, as described in #1703168-24, possible?

Will revisioning configs be a contrib add-on? Does this affect how diff will be used in the Config UI?

gdd’s picture

It will be available in contrib, and actually there is already one contrib sandbox which implements it.

gdd’s picture

No longer applies, working on a reroll

gdd’s picture

OK here's a reroll to HEAD. I talked to beejeebus about moving this to database storage and he said he's not against it, so I'll probably make that change. I was pondering adding some API functions around it (config_is_config_overridden($object) or something) however we might not need that at all because we can just use config_sync_get_changes() with $active and $snapshot storage. If it turns out we want something more as things get implemented we can add them in followups.

gdd’s picture

Status: Needs work » Needs review
mkadin’s picture

+1 for this feature, I'd love to see some fancy diffing of views and such.

Re-queuing as this patch is breaking the installer for me. Let's see...

mkadin’s picture

gdd’s picture

Here's a new version which runs on database storage and has a custom table for storing the snapshots. I started playing around with the idea of storing a hash of the data (for comparisons) in addition to the actual data (for diffs) but it wasn't worth the trouble. Once this is in we should figure out how/if we're going to surface it in the import UI.

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-28.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
4.9 KB

This is passing for me locally, however it appears the snapshot table isn't getting installed because system.module isn't getting enabled. Lets try this.

Note that I thought about putting table creation into config.install, but we need this table to exist even if someone uninstalls the Config UI (they can still do imports via drush and the like.)

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-30.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
751 bytes
4.87 KB

Does this work instead?

Status: Needs review » Needs work
Issue tags: -Configuration system, -VDC, -feature freeze

The last submitted patch, config-1515312-32.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +VDC, +feature freeze

#32: config-1515312-32.patch queued for re-testing.

xjm’s picture

FileSize
4.98 KB

I'm a dork.

gdd’s picture

I realized we should be creating an initial snapshot at install time, so this patch adds that.

Other than that this is all tests. I modified the tests in ConfigImportTest to check that the snapshot and active match at initial install as well as after a successful import.

I also created ConfigSnapshotTest which tests the following:

- We have a matching snapshot at install time
- That it doesn't match when we modify the active config
- That it doesn't match staging but still matches active when we change the staging config
- That active and snapshot match again after an import.

Status: Needs review » Needs work
Issue tags: -Configuration system, -VDC, -feature freeze

The last submitted patch, 1515312-config-import-snapshots-36.patch, failed testing.

mkadin’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +VDC, +feature freeze
webchick’s picture

Priority: Normal » Major

Seems like this is pretty key to a well-functioning configuration system, so escalating to major.

alexpott’s picture

I wonder whether we should be using the state system to store the snapshots instead of a new table?

moshe weitzman’s picture

If we are going to stick with just saving one snapshot, then I could see us moving this to state system instead of DB storage. State designed for name+blob storage like this. However, I expected to see yet another directory for this. That is, I expected FileStorage. That lets sysadmins use standard diff tools to look for changes. I'm curious why we went for DB storage.

I'd love to see core start saving multiple snapshots. And we use the upcoming diff feature to compare arbitrary snapshots. I'm OK with that being in Contrib for D8.

+++ b/core/includes/bootstrap.inc
@@ -2480,6 +2480,12 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+    // Register import snapshot configuration storage.

Personally, I would remove the word 'import' here.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.php
@@ -0,0 +1,78 @@
+    // Verify that active and snapshot match, and that staging doesn't match
+    // either.
+    $this->assertFalse(config_sync_get_changes($snapshot, $storage));
+    $this->assertTrue(config_sync_get_changes($snapshot, $staging));
+    $this->assertTrue(config_sync_get_changes($staging, $storage));

The code comment suggests to me that I would have seen two FALSE and one TRUE

gdd’s picture

My concern about adding a third directory is that it is going to cause confusion when people are staging their config. Having a staging and active directory is already more confusing than I'd like. However the state system does potentially make sense. We would need to write a StateStorage controller. I'm trying to think if there are scenarios when we would want snapshot to be a bit more permanent. Moving your site to a new server? As long as the state API is non-ephemeral it might be a better option, yes. However if we decided to save multiple snapshots, state probably wouldn't be appropriate anymore.

The code comment suggests to me that I would have seen two FALSE and one TRUE

config_sync_get_changes() actually returns FALSE when the two storages are the same, and an array of changes when they are different. I was confused at first too. I clarified that comment and the other one in this new patch.

sun’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.inc
@@ -2480,6 +2480,12 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+    $container
+      ->register('config.storage.snapshot', 'Drupal\Core\Config\DatabaseStorage')
+      ->addArgument(new Reference('database'))
+      ->addArgument('config_snapshot');

+++ b/core/modules/system/system.install
@@ -1454,6 +1454,26 @@ function system_schema() {
+  $schema['config_snapshot'] = array(

I agree that this should use the key/value storage.

The primary reason for that is that it would be wrong/invalid to stage a snapshot.

Furthermore, I cannot see why that storage would have to be tied to config in any way. It's just a new key_value $bin, nothing else.

In turn, that also eliminates the new database table.

+++ b/core/includes/config.inc
@@ -233,6 +235,53 @@ function config_import() {
+  foreach ($snapshot_storage->listAll() as $name) {
+    $snapshot_storage->delete($name);
+  }

Most storage controllers will be able to handle this in a more efficient way. We should add a deleteAll() method that has the same signature as listAll().

EDIT: The deleteAll() suggestion is obviously obsolete by switching to key/value.

+++ b/core/includes/config.inc
@@ -233,6 +235,53 @@ function config_import() {
+/**
+ * Resets configuration to the state of the last import.
+ */
+function config_restore_from_snapshot() {

I'm not sure whether the addition of this API function makes sense. I can't really see in which case you'd want to revert all configuration to a previous state.

+++ b/core/includes/config.inc
@@ -233,6 +235,53 @@ function config_import() {
+    // Flush all caches and reset static variables after a successful import.
+    drupal_flush_all_caches();

In any case, dfac() should not be contained in this API function.

The call to dfac() is the reason for why you had to change the test to a WebTest, so we can revert that change, too.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.php
@@ -37,6 +37,12 @@ function setUp() {
+    // After installation we enabled config_test.module, which changed the
+    // active configuration. Create a new snapshot to get us back to parity.
+    $active = drupal_container()->get('config.storage');
+    $snapshot = drupal_container()->get('config.storage.snapshot');
+    config_import_create_snapshot($active, $snapshot);

I do not understand what this comment tries to say, and why we need to manually create a snapshot.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.php
@@ -0,0 +1,78 @@
+class ConfigSnapshotTest extends WebTestBase {

This test should use DrupalUnitTestBase.

xjm’s picture

This test should use DrupalUnitTestBase.

See #30 through #35. If you can get it working using a unit test, then by all means, but otherwise, I see no reason not to use a web test. Slower? Sure. But it works.

gdd’s picture

I agree that this should use the key/value storage.

I do now as well, although there is still some dissent around whether it should use k/v or be stored in a third subdirectory in the file system. I'm not a fan of that option but I'd like to hear some more voices.

gdd’s picture

Most storage controllers will be able to handle this in a more efficient way. We should add a deleteAll() method that has the same signature as listAll().

Agreed, #1851180: Add deleteAll() function to Drupal\Core\Config\StorageInterface opened.

I spent some time attempting to implement this as keyvalue storage and I'm not sure it works properly as things currently stand. In order for this to make sense we really need to create a KeyValueStorage implementation of StorageInterface, so that it can be used in config_sync_changes() and config_sync_get_changes(). However implementing this is not a no-brainer. The biggest problem is that there is currently no way to implement listAll() for keyvalue storage, and the functionality doesn't really make sense for keyvalue anyways. We could just have it throw an exception, but then we have to rework the way ConfigTestBase works (it assumes a valid implementation of listAll() for all storage implementations.) A similar problem exists for rename(), which we could work around by just doing a delete/add operation, but it feels wrong.

Adding deleteAll() would remove the dependence on listAll() for this patch, but the greater questions are still worth pondering.

xjm’s picture

So I had @heyrocker explain #48 to me. If we create a k/v bin for each snapshot, and want to look for a particular configuration object or set or objects, then we have to load all the snapshots to look for them. That's a misuse of the k/v store I think.

gdd’s picture

Yes, to clarify, getting one object or all objects from keyvalue is easy. Its the querying by prefix that listAll() does which is the problem.

gdd’s picture

Given the above, I'd like to leave the table. I'm still against a file-based snapshot implementation. Anyone else have any thoughts here? I'd like to push this through.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
12.45 KB

here's a reroll to get this going again.

gdd’s picture

I'm not sure whether the addition of this API function makes sense. I can't really see in which case you'd want to revert all configuration to a previous state.

I agree and changed this API function to revert a single piece of configuration.

In any case, dfac() should not be contained in this API function.

This actually ended up getting reverted in another patch.

I do not understand what this comment tries to say, and why we need to manually create a snapshot.

I made this more explicit in the tests.

Other changes:
- Changed lock functions to use new API
- Now using new deleteAll() functionality for config storage
- Added restore functionality to tests

Having heard no more discussion about where to store the snapshots can we consider that topic shut?

I'm getting an odd test fail on this locally so I'm throwing it at the bot.

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-53.patch, failed testing.

Anonymous’s picture

looked at this, found that we're doing this in config_restore_from_snapshot():

  if (!lock()->acquire('config_import')) {
...
  lock()->release(__FUNCTION__);

fixed that so it uses CONFIG_IMPORT_LOCK, but the test still fails. still digging...

Anonymous’s picture

Status: Needs work » Needs review
FileSize
12.36 KB

turns out that calling drupal_static_resset() (which we do via drupal_flush_all_caches()) is not safe during a lock.

because lock():

function lock() {
  $lock_backend = &drupal_static(__FUNCTION__);

  if (!isset($lock_backend)) {
    $class_name = variable_get('lock_backend', 'Drupal\Core\Lock\DatabaseLockBackend');

    // Do not allow a WSOD here, if the class does not exists use the default
    // one.
    // @todo We should log failed class loading for debugging, but for that we
    //   need an early watchdog function that logs into a file if the database
    //   is not present.
    if (class_exists($class_name)) {
      $lock_backend = new $class_name();
    }
    else {
      $lock_backend = new DatabaseLockBackend();
    }

    drupal_register_shutdown_function(array($lock_backend, 'releaseAll'));
  }

  return $lock_backend;
}

will go ahead and get a new lock backend, so we end up with a different $lockId, and lock()->release() will always fail. because the test takes a lock for config_restore_from_snapshot(), but doesn't release it, config_import() never actually runs.

the quick fix in the attached patch is to move the drupal_flush_all_caches() call after the lock()->release(). i think we probably need another issue to deal with the lock stuff.

gdd’s picture

Thanks for tracking that down! I personally think this is ready but someone else should pull the switch.

tim.plunkett’s picture

+++ b/core/includes/config.incundefined
@@ -235,6 +237,69 @@ function config_import() {
+  if (!in_array($op, array_keys($config_changes))) {

Not that it really matters, but if (!isset($config_changes[$op])) { does the same thing with no function calls.

+++ b/core/includes/config.incundefined
@@ -235,6 +237,69 @@ function config_import() {
+  if (!lock()->acquire(CONFIG_IMPORT_LOCK)) {
+    return FALSE;

Couldn't this be moved to the top, before even the drupal_container() calls?

+++ b/core/includes/config.incundefined
@@ -235,6 +237,69 @@ function config_import() {
+  $success = TRUE;
+  try {
+    $remaining_changes = config_import_invoke_owner($config_changes, $source_storage, $target_storage);
+    config_sync_changes($remaining_changes, $source_storage, $target_storage);
+  }
+  catch (ConfigException $e) {
+    watchdog_exception('config_restore_from_snapshot', $e);
+    $success = FALSE;

For readability, I'd have $success = FALSE at the top, then $success = TRUE in the try{}. This way, it seems like we're saying it succeeded before even trying.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -7,12 +7,12 @@
-class ConfigImportTest extends DrupalUnitTestBase {
+class ConfigImportTest extends WebTestBase {

:( Why did this need to be done?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined
@@ -0,0 +1,78 @@
+ * Definition of Drupal\config\Tests\ConfigSnapshotTest.

Contains \Drupal\...

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined
@@ -0,0 +1,78 @@
+class ConfigSnapshotTest extends WebTestBase {
+  public static function getInfo() {

Missing a blank line before the method

---

Except the switch away from a Unit test, this is great.

xjm’s picture

:( Why did this need to be done?

See #30 through #35 and #45 / #46. The dfac() is still there, I think. Someone could try reverting the class change and seeing if it works.

gdd’s picture

Someone could try reverting the class change and seeing if it works.

Actually the call to drupal_flush_all_caches() that was causing problems is gone, so we can in theory change to DrupalUnitTestBase. However, when I just tried that, I discovered that the initial config snapshot never gets created, because that happens in install_finished(), which never gets called in unit tests. Note that this snapshot has to get created *after* all other modules are installed, otherwise it will be incomplete. If someone knows a better place to put this, let me know.

Couldn't this be moved to the top, before even the drupal_container() calls?

I think this is a bad idea, since we want the code that executes after the lock is acquired to be as small as absolutely necessary. Talked to timplunkett about this in IRC and got his agreement.

All other issues addressed in attached patch. No interdiff since I also merged head, sorry.

sun’s picture

Reverted ConfigImportTest back to DUTB. The schema of the {config_snapshot} table just had to be installed.

That said, I'm really not sure whether the snapshot assertions actually belong in there. Remember, these are supposed to be unit tests — mixing arbitrary other functionality into an existing unit test is not really good.

Also, I was not able to understand the test plan in the new ConfigSnapshotTest. I think the code comments, but also the variable names being used need some clarification on what is actually going on in there.

Lastly, I wasn't really able to make sense of the {config_snapshot} table/schema... it only has name/data columns — no snapshot identifier? So there can only be a single snapshot, and whenever you do one, and whenever you import configuration, you overwrite a possibly existing snapshot?

gdd’s picture

That said, I'm really not sure whether the snapshot assertions actually belong in there. Remember, these are supposed to be unit tests — mixing arbitrary other functionality into an existing unit test is not really good.

Since successfully creating a snapshot during import operations is an important functionality, I thought they made sense to be added to those tests. On the other hand, the main snapshot tests cover an import operation already, so I'd be OK removing them from ConfigImportTest.

Lastly, I wasn't really able to make sense of the {config_snapshot} table/schema... it only has name/data columns — no snapshot identifier? So there can only be a single snapshot, and whenever you do one, and whenever you import configuration, you overwrite a possibly existing snapshot?

That is correct, it can only be used to compare for changes since the last import. I think that for core this is fine, contrib can do tracking of multiple snapshots if necessary, although that would require them to maintain their own tables or storage. So hrm. I can see adding a snapshot identifier for future growth purposes, but I still don't think core should maintain more than one snapshot out of the box.

Will improve comments in the test in next roll, hopefully by tomorrow morning. Would be nice to get this in.

gdd’s picture

New patch removes snapshot tests from ConfigImportTest and attempts to clarify ConfigSnapshotTest. I held off on the change to the schema of {config_snapshot} pending further comment.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed this before and all suggestions including mine have abeed addressed. Lets go RTBC on this. I think we could have aimed higher here (preserve multiple snapshots), but this will do for now.

moshe weitzman’s picture

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system, +VDC, +feature freeze

The last submitted patch, 1515312-config-import-snapshots-63.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
6.89 KB

Reroll, just moving code around. Should go back to RTBC if the bot comes back green.

gdd’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I don't want to be a party pooper but think this patch needs more work because

  • Reverting a snapshot will have some very interesting effects if modules have been enabled since the last snapshot as snapshotting only takes place on import. For example, a module can be disabled without any disable hook firing. And should a module be disabled or uninstalled?
  • There are no tests. Especially of the config_restore_from_snapshot() function.

I would like to see #1808248: Add a separate module install/uninstall step to the config import process done before this... and therefore should get done before to #1890784: Refactor configuration import and sync functions which would significantly change this code as snapshotting should be done on firing on the synchronise event. And the restore from snapshot code would be a lot simpler too.

gdd’s picture

Talked this over with alexpott in IRC. I had also been having some misgivings about the restore process, as I'm not sure we can get single-config-restore to work consistently at all. There are some functions in Drupal that are inherently not restorable (think field changes) and the interactions between config changes that may or may not have happened makes this area inherently complex. We both agreed that it is best to simply punt on restore entirely for this iteration of the patch, and if we do that then this is fine to move on. In the next followup we can expose this in the import ui as a warning (HEY YOU MIGHT BE OVERWRITING DATA DANGER DANGER) and link to a diff of the changes so you can sort it out if you need to.

He also pointed out that the tests are missing in the last patch, which I can fix easily enough.

I'll do that this afternoon.

moshe weitzman’s picture

If we had validation, then a field would reject such a restore. See http://api.drupal.org/api/drupal/core%21modules%21field%21field.crud.inc...

gdd’s picture

Status: Needs work » Needs review
FileSize
8.45 KB

I'm happy to integrate validation into this conversation when it exists. I think a lot of this stuff can be iterated going forward, but I really want to get the basics in place first in case none of the dependencies end up going forward.

Attached patch re-adds the test and removes config_restore_from_snapshot() and hopefully we're done here then.

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-73.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
6.27 KB

The upgrade path failures are random bot fails, the other is me being stupid.

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-75.patch, failed testing.

gdd’s picture

Status: Needs work » Needs review
FileSize
7.31 KB

Note to self: Actually run tests after changing them. (CKEditor test fail is unrelated and is happening all over the place today.)

alexpott’s picture

Real minor nit...

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigSnapshotTest.phpundefined
@@ -0,0 +1,64 @@
+}
\ No newline at end of file

Missing a new line at end of file

Also the test should probably be a DrupalUnitTestBase but this is non trivial as we'd need to create the services on the container or just use the classes to create objects. Happy to do this as a follow up.

gdd’s picture

Based on the nit this is RTBC if the bot comes back green (which may not be until Sunday at this rate.)

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-79.patch, failed testing.

alexpott’s picture

The reason for the test failures in #79 is the simpletest overrides to system.file is leaking into the test. To mitigate this I've changed the test to DrupalUnitTestBase and I'm using the config provided by the config_test module not config provided by the system module.

In changing the tests to DrupalUnitTestBase I think I might have uncovered an issue with snapshots and module enables / disables / uninstalls as these activities do not touch the snapshot table. See the code from the test below:

    // Install the default config.
    config_install_default_config('module', 'config_test');
    // Although we have imported config this has not affected the snapshot and
    // no differences are show because: manifests are only compared if they
    // exist in the snapshot and simple config is only listed if it is changed.
    $this->assertFalse(config_sync_get_changes($snapshot, $active));

    // Update the config snapshot.
    config_import_create_snapshot($active, $snapshot);

    // The snapshot and active config should now contain the same config
    // objects.
    $this->assertIdentical($active->listAll(), $snapshot->listAll());

what is concerning is that if we moved $this->assertIdentical($active->listAll(), $snapshot->listAll()); it would fail because the snapshot would be empty but active would contain a number of config objects... and config_sync_get_changes($snapshot, $active)) is reporting there is nothing to sync...

alexpott’s picture

Status: Needs work » Needs review

Test please...

alexpott’s picture

After discussing this with heyrocker on IRC we decided that is was a bug that after importing config calling config_sync_get_changes() with the snapshot and active storages returned FALSE (as in no changes). This is a result of how config_sync_get_changes() assumes that basic config files can only be changed and config entities as managed using the manifest files.

One possible solution is to use a function similar to config_sync_get_changes() before the addition of manifests. The patched attached implements config_compare_storage_contents() to do exactly that.

Status: Needs review » Needs work

The last submitted patch, 1515312-config-import-snapshots-83.patch, failed testing.

alexpott’s picture

The following test failure does not occur locally...

Drupal\taxonomy\Tests\TermTranslationUITest	152	1	0
Message	Group	Filename	Line	Function	Status
Invalid values generate a list of form errors.	Other	EntityTranslationUITest.php	40	Drupal\translation_entity\Tests\EntityTranslationUITest->testTranslationUI()

Sending for a re-test

alexpott’s picture

Status: Needs work » Needs review
alexpott’s picture

Borrowed code from #1889620: config_install_default_config() overwrites existing configuration that adds an argument to config_sync_get_changes() as to whether to use manifests. If set to false then the function compares everything rather than using the manifests present in the source storage.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

#87 looks great to me. Given that the premise of this issue was RTBC already back in #69, and the implementation fixes since then look good, I think it's safe to re-RTBC this. Committers: please give this one at least 24 hours in case others more involved with this issue than I've been know of something still wrong with it.

Anonymous’s picture

this looks ready to fly.

sun’s picture

Also happy to move forward here. I guess this facility will undergo larger revamps anyway later on.

YesCT’s picture

webchick’s picture

Category: feature » task
Status: Reviewed & tested by the community » Fixed

Ok, since it's the 11th hour of feature freeze here, I did a review of this patch. There's nothing to see since this doesn't introduce any revert capabilities or whatever in the UI to interact with these snapshots; it only creates the table to store them in and adds an API/test coverage. I was told by Tim that this is required functionality for #1790398: Re introduce revert functionality for views using the config system, and since #1790398: Re introduce revert functionality for views using the config system is basically a regression in Views, I'm personally comfortable moving this to a task rather than a feature request. The other issue will have to see whether this becomes a core or contrib thing, but in any case we need this stuff for the functionality at all, it sounds like.

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.