Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B

Actually, this is more general than just fields, so re-titling & re-categorizing.

Possible approaches for use cases of "creation/update of ConfigEntity A triggers creation/update of ConfigEntity B"
- Limit this to UI operations
i.e. only auto-create 'body' field on node types created through the UI
- Have a way to somehow identify that a ConfigEntity is being imported
i.e. do not auto-create 'body' field on import of a node type - whatever config changes that should happen are supposed to be part of the import set already.

bit shorter

Generalize

I can think of a couple other cases in core right now where we cleanup entries in entity B when entity A gets deleted. Didn't investigate whether this specific case also has its own set of race conditions, but this smells too.

"No cascade modifications of config entities during import, the import set is the only authority" sounds like a good practice, now that we don't support partial imports. Import means "the final result is supposed to be identical to the import set".

Ideally the import process would enforce this (same as UpdateModuleHandler enforces no hook is fired during updates), but I guess that's not really doable ? Import triggers ConfigEntity CUD APIs (and rightfully so...), we cannot really control what happens in there.

So short of that, yes, a flag on a ConfigEntity indicating it's being created/updated/deleted as part of import, and the code reacting to CUD needs to check the flag before acting on other ConfigEntities...

Conclusion from the "CMI hard issues" discussion in DC Prague:

- Add config_synching flag (like cron_running) -- ConfigEntityBase::isSynching(), plus a setter/getter, not exported
- It’s the responsibility of runtime code (MyConfigEntity::postSave(), hook_entity_save()...) to check the flag and not trigger separate config changes if the flag is present
- Also add a global state flag
- Check the state flag and throw an exception if a configuration entity is saved without the local sync flag when the global state flag is there.

more accurate

Wondering:
It might be worth promoting that "is syncing" flag to a new param in ConfigEntity::preSave()/postSave() / hook_entity_save(), to make it more obvious that this case needs some special consideration.

Cool! There are (probably a lot) more places in core where we'll need to check for the isSyncing flag (including "delete instances when field is deleted" and reversedly, in Field / FieldInstance classes - we'd need #2020895: Move save() / delete() logic in Field / FieldInstance to [pre|post]Save(), [pre|post]Delete() in first), but this looks like a good start.

What do you guys think of #7 ?

It might be worth promoting that "is syncing" flag to a new param in ConfigEntity::preSave()/postSave() / hook_entity_save(), to make it more obvious that this case needs some special consideration.

"Do not try to trigger further config changes when reacting to an import" is going to be an important rule that contrib will need to understand and apply correctly, I'm trying to think of ways to have the APIs make the question more obvious.

@beejeebus: it's nothing about fields specifically, it's about adding an $is_syncing param (defaults to FALSE) to EntityInterface::preSave()/postSave()/preDelete()/postDelete() and hook_entity_save()/_delete()... so that implementors are aware that "is syncing" is an important aspect they need to accomodate.

However, that additional flag would make sense only for config entities, while those methods / hooks are defined for all entity types, so maybe that's not a good idea after all.

As mentioned in the summary of the Prague discussion in the OP, it was agreed to raise an exception to explicitely prevent writing config changes while processing the import of a config entity. When we do that, we kind of achieve that effect of raising awareness, since, well, bad code will break on import instead of silently succeeding and having weird effects...

Minor nits :

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -35,6 +35,14 @@
   +   * Whether the config is being created, updated or deleted through the
   

   s/the config/the config entity/ ?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
   @@ -76,6 +84,14 @@ public function setStatus($status);
   +   * Returns whether the configuration entity is created, updated or deleted
   +   * through the import process.
   

   s/is/is being/ ?
   (also, I think we commonly say "config entity" in core comments ?)

3. +++ b/core/modules/node/lib/Drupal/node/Entity/NodeType.php
   @@ -103,19 +103,6 @@ class NodeType extends ConfigEntityBase implements NodeTypeInterface {
   -   * Indicates whether a Body field should be created for this node type.
   -   *
   -   * This property affects entity creation only. It allows default configuration
   -   * of modules and installation profiles to specify whether a Body field should
   -   * be created for this bundle.
   -   *
   -   * @var bool
   -   *
   -   * @see \Drupal\node\Entity\NodeType::$create_body_label
   -   */
   -  protected $create_body = TRUE;
   

   Why do we need to remove this ?

Other than that, I really think we should enforce that with an exception as mentioned in the OP (otherwise it's just too easy to overlook). But the moment we do that, it also means we need to fix all the places in core that will start raising that exception, so that's possibly best left to a separate followup ?

Also, this is at least major.

Sure, I mean why does the code replace the existing check instead of simply adding a new condition ?
if ($this->get('create_body') && !$this->isSyncing()) { ?

Agreed, I was about to RTBC myself :-)

Opened #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter for the "raise exception" followup.