Problem/Motivation

During an import secondary writes or deletes of configuration through hook implementations or event listeners are impossible to prevent. Raising an exception or silently failing both have completely unpredictable outcomes because the hook and event system make it impossible to completely lock down the config system in this way during an configuration import.

Proposed resolution

Our general approach is to let the secondary writes happen, then try to clean up afterwords in import-specific code. We will also try to complete the config import, rather than blowing up - we are guessing that this will lead to less broken sites than stopping half-way through an import.

List of scenarios and ways we will handle it below:

  • create config that already exists: this will just blow up, and is a bug the module author needs to fix
  • create config doesn't already exist, there are two scenarios
    • the config that was created exists in the source tree, so we will do a delete/create so that the config object looks like source
    • the config that was created doesn't exist in the source tree. we have two options, and are not decided yet. a) we clean up and delete the newly created object, so that the target tree looks like the source, or b) we don't do anything, so that the code that created the new entity doesn't break
  • delete config that has already been deleted - in this case we should do nothing.
  • delete config that exists in the source tree - we will try to fix this up by doing a create, so that the target tree looks like source. however, this is possibly already too late

#2175233: stop config import and log/display error and state information as best we can will explore if there is anyway we can report these events to the user.

Remaining tasks

Write tests.

User interface changes

None

API changes

?

Original report by @username

Followup of #2069373: Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B.

In Prague, it was determined that the import (create/update/delete) of a config entity through the config import process should not trigger secondary config writes (e.g: auto-creation of body field on creation of a new node type). During config import, the set of imported config files should be the only source.

The issue linked above added an "isSyncing" flag on config entities to allow the entity CUD callstack (preSave/postSave, preDelete/postDelete, hook_entity_OP()) to determine whether secondary config changes can be triggered.

It was also agreed to have the config system enforce this behavior by raising an exception when some code tries to write to config while a config entity is being processed (like we prevent hook invocations in updates - otherwise, isSyncing is just too easy to overlook). However, doing so means fixing all the places in core that currently do that, so this was deferred to a followup.

Files: 
CommentFileSizeAuthor
#65 2124535.65.patch42.17 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,683 pass(es). View
#65 62-65-interdiff.txt2.73 KBalexpott
#62 2124535.62.patch41.25 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,738 pass(es). View
#62 58-62-interdiff.txt12.74 KBalexpott
#58 2124535-secondary_config_create-58-interdiff.txt22.03 KBBerdir
#58 2124535-secondary_config_create-58.patch32.53 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,722 pass(es). View
#54 2124535-secondary_config_create-54-interdiff.txt15.73 KBBerdir
#54 2124535-secondary_config_create-54.patch38.35 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,630 pass(es), 2 fail(s), and 0 exception(s). View
#51 2124535-secondary_config_create-51-interdiff.txt10.07 KBBerdir
#51 2124535-secondary_config_create-51.patch29.7 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,104 pass(es). View
#48 2124535-secondary_config_create-48-fix-for-create.txt1.29 KBBerdir
#48 2124535-secondary_config_create-48-tests-only.patch22.81 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,983 pass(es), 2 fail(s), and 6 exception(s). View
#48 2124535-secondary_config_create-48.patch23.33 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,990 pass(es), 2 fail(s), and 4 exception(s). View
#44 interdiff-42-44.txt1.22 KBDésiré
#44 2124535-secondary_config_create-44.patch14.36 KBDésiré
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,821 pass(es). View
#42 interdiff-39-41.txt3.91 KBDésiré
#42 2124535-secondary_config_create-41.patch13.79 KBDésiré
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,811 pass(es). View
#39 2124535-secondary_config_create-39.patch15.53 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,482 pass(es), 43 fail(s), and 7 exception(s). View
#36 2124535.36.patch16.73 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,535 pass(es). View
#36 34-36-interdiff.txt9.42 KBalexpott
#34 check-syncing-2124535-34-interdiff.txt867 bytesBerdir
#34 check-syncing-2124535-34.patch7.61 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,069 pass(es), 320 fail(s), and 28 exception(s). View
#32 check-syncing-2124535-32.patch7.34 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#22 config-2124535-22.patch22.77 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 63,694 pass(es), 0 fail(s), and 1 exception(s). View
#18 2124535.18.patch22.4 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,942 pass(es), 0 fail(s), and 1 exception(s). View

Comments

alexpott’s picture

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!

xjm’s picture

alexpott’s picture

Issue tags: +beta blocker, +Configurables
catch’s picture

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.

beejeebus’s picture

can we please not do this?

pretty sure we're mixing up 'make it easy to spot bad code in development' with 'make CMI deployment more robust'. we should make it easier to spot code that does the wrong thing during import.

throwing an exception, at some indeterminate place during import (lots of people are going to use this without following best practice, so this will happen on live sites), seems like the worst possible way to do this.

yched’s picture

I thought that had been agreed in Prague ?

Thing is : you *really* shouldn't trigger secondary config writes during the sync of a config entity. That's a critical principle of config sync - we're syncing a config tree, so don't write to it while we're syncing or your writes will either:
- get overwritten during the rest of the sync (which will sometimes fail with a fatal because you created an entity that was supposed to be created differently later in the sync),
- or just silently overwrite data that got synced earlier in the process.
One or the other depending on delicious and hard-to-reproduce things like alphabetical order :-p; I predict epic hair pulls in contrib issue queues when that happen.

Then, second fact:
Our ConfigEntity API makes it *super* easy to trigger such secondary config writes without even noticing it. My bet is that 90% of contrib / custom code won't notice the isSyncing() flag and understand why it's here and how it should be used. And your syncs might work fine in your simple test cases. So you just release your code, and hairs get pulled.

If it shouldn't happen, then it's better to make it fail with a bang so that it just can't go unnoticed, rather just silently break things in subtle and hard to reproduce ways.

Not sure what other ways than raising an exception when something that shouldn't happen happens; I'd be fine with personnally mailing the code author, but well :-)

The exact same principle was adopted for "fail with an exception when invoking hooks within hook_update_N()". It might also break in production.
If something shouldn't happen because bad things ensue, and the supporting API can easily detect when it happens, then it's the responsibility of that API to make sure misbehaving code gets noticed early ?

beejeebus’s picture

i think it's wrong during hook_update_N() as well :-)

i'm ok to let this go, but my point still stands. if this problem is really going to happen often, we're choosing an approach which my gut tells me will thoroughly break more sites than the issue it is trying to resolve.

Berdir’s picture

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.

beejeebus’s picture

i'm all in favour of catching this in development, and if i could choose the way we behave here, and set it to 'throw exception' via settings.php in dev, that would be awesome.

but in production, i don't see why our choices are only 'silent bugs' vs 'blow up your import'.

if we can detect this to throw an exception, we can detect this to log an error, and show a big warning in drush/the import UI.

given the way our import process works, i'm just surprised people are more worried about possible issues from extra config writes vs almost certainly broken sites from an import that just stops half way.

mtift’s picture

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?

sun’s picture

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.

yched’s picture

@mtift:

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

Not at all, we have plenty of those in core:

- NodeType::postSave($update = FALSE) calls node_add_body_field(), which in turn creates Field and FieldInstance config entities
- CustomBlockType::postSave() does roughly the same when a new "custom block" bundle is created
- I'm pretty sure there's some code to autocreate a "comment" field on new node types

- NodeType::postSave($update = TRUE) calls entity_invoke_bundle_hook('rename') if the node type machine name is changed, which in turn will cause a ton of updates of other config entities.
- Similarly, ImageStyle::postSave() calls static::replaceImageStyle() on "image style machine name change" which updates EntityDisplay config entities for widgets / formatters
that use the image style
("Sync vs config renames" are still an unadressed quagmire...)

- entity_reference's implementation of hook_ENTITY_TYPE_update() updates field instances when some fields change (which looks really weird, actually)

-- Not even talking about deletes causing other deletes or updates, because those might be less problematic in terms of race condiftions during config sync if we're lucky.

That's after 10 minutes searching, I'm pretty sure there are more...

That's what I mean when I write "Our ConfigEntity API makes it *super* easy to trigger such secondary config writes without even noticing it". And its extremely open, thus hard to track: any config write that happens at any point in the callstack during a ConfigEntity::[pre|post][CRUD] or hook_entity_CRUD($config_entity).

------------------

Basically, the ConfigEntity API provided an extremely handy formulation for our notion of "config entries of the same 'kind' which might exist in arbitrary amounts on a site" (views, fields, node types...):
- we made them Entities, and gave them the same rich set of callstacks and hooks than regular entities for reacting to CRUD,
- then on config import / sync, easy, we just trigger the *exact same API action and code path* than for the "original" API action we're rerpoducing : $entity->create(), ->save(), ->delete().

This made it extremely easy to write and manipulate ConfigEntities.

But an API CRUD action on a config item triggers two sorts of things:
a) action on the db (create a table, clear a cache...)
b) action on the config (writing the config change itself + triggering secondary changes in other parts of the config tree)
And both a) and b) are written intertwined in the same places (Entity::pre|postCRUD() + hook_entity_CRUD()), and are "open buffet for everyone" since, well, hooks.

So, payback time : "do exactly the same steps when initially "doing" the API action and when "replaying" it on config sync" cannot work. Syncing config means replaying a) *without* b), because that causes race conditions. I can't be the exact same code paths.

That's why we added $entity->isSyncing(), so that code that wants to do b)-style stuff can check it and refrain if TRUE.

yched’s picture

To finish replying to @mtift #12:
No, faulty code can only be detected when performing a *config deploy*. Not when enabling the module, not when running it on your dev site. It's only when you sync config to another site that errors will happen.

yched’s picture

re @beejeebus / @sun:

Yeah, I guess we could probably log and report errors (even though I doubt we can easily detect who's the module at fault, due to nested calls).

But IMO it's fairly hard to decide what's the right "friendly non breaking behavior" to let the sync go through :
1) stop the secondary write from happening (just do not write it) ? Then we have code that expects some action has been done while it actually hasn't, hard to tell what may break then.
2) let the secondary write happen and hope the resulting race conditions won't cause fatals themselves or be otherwise too destructive ?

I guess 1) is the least evil. But still leaves the site in a state that's hard to trust.
So yeah, whatever we do, it has to look sufficiently serious that it gets noticed and reported immediately, so that awareness spreads across module authors.

I wish we could have made the isSyncing() flag more prominent that just a method hidden in the $entity, by adding it as an explicit param in ConfigEntity::pre|postCRUD() and hook_entity_CRUD(). But those methods and hooks are shared with content entities, for which such a param makes no sense and is not in the interfaces.

So other than that, afaics our best hope is "shun loud on faulty code and hope this makes it common knowledge".

yched’s picture

Another drastic approach (was discussed with @alexpott a couple weeks ago in IRC) would be that config sync calls dedicated ->syncSave() -> syncDelete() methods instead of the regular $entity->save() / ->delete(), .

I.e have different code paths by using different entry points rather than by using the same entry points + an "isSyncing" flag.

It's then up to each ConfigEntity to structure its code so that what needs to be shared (a in #12) is shared, and what needs to happen only in non-sync (b).

Not really sure what's best DX-wise, honestly. Also, not sure what this would mean for hooks.

alexpott’s picture

Priority: Major » Critical

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

beejeebus’s picture

i think the summary we came up with for this was:

- don't blow up, instead write logs/show error messages
- don't allow the secondary write, as the change should happen via the follow up sync process anyway

alexpott’s picture

Status: Active » Needs review
FileSize
22.4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,942 pass(es), 0 fail(s), and 1 exception(s). View

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!

Status: Needs review » Needs work

The last submitted patch, 18: 2124535.18.patch, failed testing.

yched’s picture

Yay, thanks for kicking this Alex.

Quick remark after a first read : it's a bit confusing that lock($config_name) actually means "only $config_name can be written, all *other* items are locked" ?
i.e :
ConfigLock->lock($name);
results in ConfigLock->isLocked($name) == FALSE ?

beejeebus’s picture

this issue raises a bigger question - is it better to stop half way, try to soldier on or try to rollback if we hit an exception of any kind during import?

i have NFI what the best answer is, because any of those options can leave you with a borkenated site. even on the same site, different options will be better depending on the type of fail, it's timing during the import, etc etc, so the write answer will vary import by import.

that feels like a bigger issue, so i created #2175233: stop config import and log/display error and state information as best we can.

xjm’s picture

Status: Needs work » Needs review
FileSize
22.77 KB
FAILED: [[SimpleTest]]: [MySQL] 63,694 pass(es), 0 fail(s), and 1 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 22: config-2124535-22.patch, failed testing.

Berdir’s picture

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.

xjm’s picture

Issue tags: +Configuration system
alexpott’s picture

Title: Raise an exception when a config write is triggered during import of a config entity » Prevent secondary configuration creates and deletes from breaking the ConfigImporter
Issue summary: View changes
Related issues: +#2175233: stop config import and log/display error and state information as best we can

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.

xjm’s picture

Issue tags: +sprint
alexpott’s picture

Status: Needs work » Postponed

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

xjm’s picture

Status: Postponed » Active

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

Berdir’s picture

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?

Berdir’s picture

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.

Berdir’s picture

Status: Active » Needs review
FileSize
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 32: check-syncing-2124535-32.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,069 pass(es), 320 fail(s), and 28 exception(s). View
867 bytes

Ups, this should install.

Status: Needs review » Needs work

The last submitted patch, 34: check-syncing-2124535-34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.42 KB
16.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,535 pass(es). View

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.

yched’s picture

Oops, I'm probably the one to blame for those bad names in FieldImportDeleteTest :-/

Other than that, IMO it would be much less confusing if the method names and code comments didn't say "config import" when they're about "config sync" ;-) Probably broader than the scope of this specific issue though, so maybe keeping the status quo is more reasonable for now. Just sayin'.

beejeebus’s picture

Issue summary: View changes
Désiré’s picture

FileSize
15.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,482 pass(es), 43 fail(s), and 7 exception(s). View

Reroll

Désiré’s picture

Assigned: Unassigned » Désiré

Status: Needs review » Needs work

The last submitted patch, 39: 2124535-secondary_config_create-39.patch, failed testing.

Désiré’s picture

Status: Needs work » Needs review
FileSize
13.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,811 pass(es). View
3.91 KB

Some minor fixes after the reroll.

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

Désiré’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
14.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,821 pass(es). View
1.22 KB
Désiré’s picture

Status: Needs review » Needs work

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.

alexpott’s picture

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

Berdir’s picture

Assigned: Désiré » Berdir
Issue tags: +Needs tests

Working on tests.

Berdir’s picture

Status: Needs work » Needs review
FileSize
23.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,990 pass(es), 2 fail(s), and 4 exception(s). View
22.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,983 pass(es), 2 fail(s), and 6 exception(s). View
1.29 KB

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.

Status: Needs review » Needs work

The last submitted patch, 48: 2124535-secondary_config_create-48-tests-only.patch, failed testing.

The last submitted patch, 48: 2124535-secondary_config_create-48.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
29.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,104 pass(es). View
10.07 KB

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.

alexpott’s picture

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

alexpott’s picture

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,630 pass(es), 2 fail(s), and 0 exception(s). View
15.73 KB

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.

Status: Needs review » Needs work

The last submitted patch, 54: 2124535-secondary_config_create-54.patch, failed testing.

alexpott’s picture

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.

alexpott’s picture

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

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
32.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,722 pass(es). View
22.03 KB

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.

beejeebus’s picture

this patch looks good overall.

+      case 'update':
+        if (!$target_exists) {
+          $this->logError(String::format('Update target "@name" is missing.', array('@name' => $name)));
+          return FALSE;
+        }
+        break;

should this also do $this->setProcessed($op, $name); ? seems like otherwise this op will never be removed?

alexpott’s picture

Status: Needs review » Needs work
+++ 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 :)

Berdir’s picture

Assigned: Berdir » Unassigned
alexpott’s picture

Status: Needs work » Needs review
FileSize
12.74 KB
41.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,738 pass(es). View
+++ 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)

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

i think this is ready if the bot comes back green.

Berdir’s picture

+++ 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 :)

alexpott’s picture

FileSize
2.73 KB
42.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,683 pass(es). View

@Berdir didn't know that...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • Commit 0b4bf3a on 8.x by webchick:
    Issue #2124535 by Berdir, alexpott, Désiré, xjm | yched: Prevent...
webchick’s picture

Status: Fixed » Closed (fixed)

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