Prevent secondary configuration creates and deletes from breaking the ConfigImporter

We need to address #1851018: Improve breakpoint configuration implementation. before this gets in because the current breakpoint code makes this impossible as it creates config entities from simple config!

I personally wouldn't block beta on throwing the exception, that's just enforcing a best practice as opposed to a full API change. This should be critical if it really does block the beta though.

I'd say the point of throwing an exception is to make it fail during development of a module, when this is tested with a simple case. Instead of silently causing weird side effects on production that are irreversible.

Let me see if I'm following the logic here. The suggestion is to raise an exception (when a config write is triggered) during import of a config entity.

Now that #1851018: Improve breakpoint configuration implementation. is fixed, we do not have examples of this in core, so essentially this issue only concerns contrib and custom modules. And we have documentation that indicates how to create a config entity where we could make it very clear that secondary config writes are not allowed: https://drupal.org/node/1809494.

If module developers ignore core examples and the documentation, and we raise an exception, would the module author(s) see it during module development when the module is enabled? Presumably, they are not writing modules without first enabling them on a non-production site. Or is this about modules that create config entities that trigger secondary config writes after they are enabled?

In other words, is the problem that a module author could ignore -- or never see -- the exception, release their module as a contrib module, an unsuspecting user enables it on development site -- and also never sees the exception -- exports their full config, imports config on a production site, and borks their site?

I agree with @yched et al, an unexpected config write during synchronization of a config entity should be caught.

@beejeebus makes a fair point in that we also need to be concerned about incomplete/aborted synchronizations, which could leave a site in a non-functional state.

That said, I do not think that we are in a position to clearly evaluate the consequences of either way yet, so we don't know what's worse.

I'd suggest to move forward in a pragmatic way:

1. Figure out How to "write-lock config save except for $name until $name has been processed"
2. Find a way to record/log warning messages.
3. See whether current HEAD is bogus already.
4. Optionally and/or conditionally replace the warning message with throwing an exception.

This gets the functionality in place and allows us to evaluate the right course of action, possibly depending on other environment factors. Let's leave that debate for later.

We do not want to throw an exception immediately during Config::save(), since that would halt whichever operation being executed in the middle → possibly leaving the application in a completely broken state. So if we're going to throw an exception, we want to throw it after the malformed operation was performed, not in the middle of nowhere.

Bumping to critical as resolution of this is beta-blocking even if we decide to do nothing.

The patch attached implements the ability to lock the configuration system to only write to a specific config name and uses this during config sync.

I have not implemented the "not blowing up" part of #17 as I think we need to discuss exactly how we should not blow up!

The somewhat obvious place to catch the new LockedConfigException is in ConfigImporter - then we can log the error and report it etc. But the problem with this approach is that we have no idea if the creation of the configuration that we want to occur has actually occurred. The secondary write to config can happen in a presave hook after all!

So I think this leaves us with the following options:

1. Ignore this issue - permit secondary writes - document and make the isSyncing flag etc hard to ignore. The problem with this approach is that after every configuration object is imported we actually need to recalculate what we need to do next as a create can easily become an either update / recreate / rename
2. Do not throw exceptions at all - just don't write the configuration - the problem with this approach is that this silent fail might play havoc with assumptions made in the code after the write that appears successful but actually is not.
3. Consider turning the whole thing around - only permit writing to config when a write lock is taken out and only permit breaking the write lock if we are not syncing. The problem with this approach is that it makes using the configuration system more difficult.

Unfortunately in order to have safely importable configuration we have to have some rules... either enforced by convention or exceptions.

EDIT: For overuse of exclamation!

Naïve reroll, not addressing #20-21.

Not sure if this was already mentioned, but I just noticed image_entity_presave() when playing around with install profiles, which attempts to load the default image and change the config entity based on it.

This doesn't cause additional writes but it does change the config entity on the fly. It should also handle the case where that file doesn't exist but that's a different issue.

In light of #18

Further discussion has resulted in coming to the conclusion that we can not throw exceptions due to secondary writes during an import. This because we have no control of when the secondary writes occur or and what to actually do if they raise an exception. The other solution of just not writing and not raising an exception also poses problems since reporting a successful config write but not writing could cause all sorts of havoc if code that fires later assumes that the config exists.

Therefore we have to proceed with this solution.

Ignore this issue - permit secondary writes - document and make the isSyncing flag etc hard to ignore. The problem with this approach is that after every configuration object is imported we actually need to recalculate what we need to do next as a create can easily become an either update / recreate / rename

In fact reading that also makes me realise we have to take into account secondary deletes too.

Therefore I'm going to repurpose this issue to implement checking in ConfigImporter::importInvokeOwner and ConfigImporter::importConfig that the operation is still correct.

Postponing this on #2004370: Batch the configuration synchronisation process as that adds ConfigImporter::process() which would be the ideal place to do this.

#2004370: Batch the configuration synchronisation process is in now, so should this be unpostponed?

Uhm, not sure if this was already discussed, but it just occurred to me that hook_install() and hook_uninstall() and related hooks also affected by this once we're actually able to deploy modules with the config sync?

Also, just as a notice, #2157467: Fix type hinting for base entity interfaces turned into "Document isSyncing()", which was a non-trivial task, but we should at least have that properly documented with that issue.

Getting started, tried to find all examples of where we are doing secondary config writes and added isSyncing() checks there. Discussed a bit with @alexpott and that's something we want to do anyway, even if we allow out as we have no other choice.

One place that I'm not sure about are the hook_entity_bundle_*() hooks. field_entity_bundle_delete() for example deletes field instances there. Probably doesn't hurt much as by then, we should have all fields deleted already anyway, And we do need to do certain things there..

It's nice to be be able to access the flag directly on the entity you're working with, but this really is a global flag, isn't it? So maybe we should allow it to be access from somewhere globally? Then field_entity_bundle_delete() could check it on it's own? And hook_install() implementations could, if we're going to try and do something about them...

Ups, this should install.

Whilst working on #2030073: Config cannot be imported in order for dependencies I discovered that \Drupal\field\Tests\FieldImportDeleteTest is not testing what it thinks it is testing. I fixed the test but then FieldInstanceConfig::postDelete() started to delete all the fields before the importer. If the fields are deleted before the instances then FieldConfig::preDelete() deletes the instances.

I think we need to make the ConfigImporter robust enough to deal with secondary deletes in the above case. All we need to do is to mark the delete as processed instead of actually trying to do the delete. The question then becomes what about secondary creates? In this instance should the create just become an update? I think if we are going to try to update a file that has been deleted then we need to log an error.

Patch attached starts to address the points above.

Reroll

Some minor fixes after the reroll.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -260,6 +260,51 @@ protected function process($op, $name) {
+      case 'update':
+        if (!$target_exists) {
+          // Error?
+        }
+        break;

We have to log these errors. The config importer needs to store them and the config sync form needs to get and display them.

I'm sure that we'll need some test for checkOp() and for secondary writings. But currently I don't know how can we reproduce a secondary writing in a test.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -260,6 +262,53 @@ protected function process($op, $name) {
+        if ($target_exists) {
+          // Become an update. This is possible since updates are run after
+          // creates.
+          $this->storageComparer->swapOp('create', 'update', $name);
+          return FALSE;
+        }

I think actually we can not do this as we need to compare the UUID. If it does not match then an update is insufficient. We need to do a delete and create.

We can test this by using the ConfigTest entity to do secondary operations on import.

Working on tests.

Ok, here we go.

The patch adds tests for:

- Create a config entity when it was already created as a secondary write
- Update a config entity that was removed in a secondary delete

There are two versions of each test, I use dependencies to ensure the order in which the run and there's a test where the "secondary" config entity is actually synced first.

All of those tests fail with the previous patch, because of what @alexpott pointed out in #46. I changed that to doing a delete first directly in that method, which fixes the create case where secondary is written later. All other tests still fail, the update for removed in the right order is failing as expected with the exception, but the other two are outside of our control as they fail within ConfigTest::postSave().

Discussed a bit with alexpott, the only way out we see is introducing a way to catch and log those exceptions and display those errors at the end. There already is a follow-up to do something like that, I'm not sure how much of this task is beta blocking and what is bugfixing that can happen later...

Attaching a full with patch with tests and the additional fix, one with the test and the existing code and the interdiff between them.

Discussed the logging a bit with @alexpott, the idea is that the core UI will use a state/keyvalue backed logger that will allow us to print and log the messages at the end.

I created a *very ugly* proof of concept for that. Tests is now green but see @todo's. Not yet integrated into the UI, and the log messages are not persisted yet.

+++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -234,4 +234,12 @@ protected function getAndSortConfigData() {
+  /**
+   * {@inheritdoc}
+   */
+  public function swapOp($from, $to, $name) {
+    $key = array_search($name, $this->changelist[$from]);
+    unset($this->changelist[$from][$key]);
+    $this->addChangeList($to, array($name));
+  }

+++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.php
@@ -80,4 +80,18 @@ public function hasChanges($ops = array('delete', 'create', 'update'));
+  /**
+   * Moves a configuration name from one change list to another.
+   *
+   * @param string $name
+   *   The configuration object name.
+   * @param string $from
+   *   The current change operation. Either delete, create or update.
+   * @param string $to
+   *   The change operation to change to. Either delete, create or update.
+   *
+   * @return $this
+   */
+  public function swapOp($name, $from, $to);
+

Obslete

Otherwise this looks great. Config importing will be way more robust with this in place.

A few more nits...

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -12,6 +12,7 @@
   +use Guzzle\Common\Exception\ExceptionCollection;
   

   Unused

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -113,6 +121,17 @@ public function __construct(StorageComparerInterface $storage_comparer, EventDis
    
   +  public function setLog(ConfigImporterLogInterface $log = NULL) {
   +    $this->log = $log;
   +  }
   +
   +  protected function log($message, $severity) {
   +    if ($this->log) {
   +      $this->log->log($message, $severity);
   +      return TRUE;
   +    }
   +  }
   

   Doc blocks

3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -254,9 +275,73 @@ public function validate() {
       *   The name of the configuration to process.
       */
      protected function process($op, $name) {
   

   Needs an @throws

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -425,6 +426,9 @@ public function importCreate($name, Config $new_config, Config $old_config) {
   +    if (!$entity) {
   +      throw new ConfigImporterException(String::format('Attempt to update non-existing entity "@id".', array('@id' => $id)));
   +    }
   

   Lets document with @throws

Work in progress, added a State config logger but no idea how to use it as I can't access anything useful from the batch in finishBatch(), so for now, just using the array logger in the UI in each batch operation separately. I could just create a new object which would get it from the same state variable (that I need to clean up

Obviously doesn't make much sense in combination with finishBatch() as that will say it was successful when it wasn't.

Maybe simplify the logging and just handle it directly in ConfigImporter and get it from there again. And drop the severity stuff. Uploading for now, feedback welcome.

Yep maybe go back to the simple log array on ConfigImporter and then we could copy it to a protected property on ConfigSync in ConfigSync::processBatch() and then we can use it in ConfigSync::finishBatch()

The added complexity of ConfigImporterLogInterface and it's implementation looks like it might be unnecessary.

@Berdir pointed out that ConfigSync::processBatch() are both static :(

So, feeling much better about this.

Dropped the separate classes, keeping the errors in $context['results'] and then handling them on the completion page. Improved tests a bit and noticed that we were missing the checkOp() stuff in BatchConfigImporter.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -506,6 +548,63 @@ protected function processExtension($type, $op, $name) {
+            $this->logError(String::format('Deleted and replaced configuration entity "@name"', array('@name' => $name)));
...
+          $this->logError(String::format('Update target "@name" is missing.', array('@name' => $name)));

This has the possibility of appearing in the UI - should we not be translating this? I guess we need to inject string translation too :)

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -506,6 +548,63 @@ protected function processExtension($type, $op, $name) {
+      case 'delete':
+        if (!$target_exists) {
+          // The configuration has already been deleted. For example, a field
+          // is automatically deleted if all the instances are.
+          $this->setProcessed($op, $name);
+          return FALSE;
+        }
+        break;

This would not have worked since setProcessed() has become setProcessedConfiguration(). New patch fixes this and adds test for it.

New patch also resolves issues brought up in #60 and #59 (for which is also improves a test to cover)

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -585,10 +596,11 @@
+            $this->logError($this->translationManager->translate('Deleted and replaced configuration entity "@name"', array('@name' => $name)));

I think we need a t() method so that locale.module can detect the strings correctly as translatable.

Nice improvements :)

@Berdir didn't know that...

Overall this patch looks great, and the test coverage is really awesome.

+++ b/core/modules/image/image.module
@@ -387,6 +387,10 @@ function image_entity_presave(EntityInterface $entity) {
+  if ($field->isSyncing()) {
+    return;
+  }

This is the one thing that makes me go "hghghghghghg" ... module developers are forced to add this to what looks like at least most entity hooks. People are totally going to forget to do that, which will cause "phantom" un-synced stuff to show up, per the logic checks.

I talked this over with Alex Pott, and he suggested a follow-up to discuss whether we add an $isSyncing = FALSE argument to the config entity hook implementations so we could put this more "in yo face." Problem of course is that this would make them non-standard with content entity hooks which do not have a "sync" operation. Hrm.

Anyway, I'll spin that off in a second and we can talk.

In the meantime, let's get this in!

Committed and pushed to 8.x. Thanks!

Here's the follow-up: #2238787: Add $isSyncing as an argument to config entity hooks?