The import of new node types currently only works only because 'field.*' files are imported before 'node.*' files.

Scenario:
0) Dev & Prod environments, exact same state and config
1) Create a node type "foo' on Dev.
- This adds node.type.foo.yml.
- node_add_body_field() (called by NodeType::postSave($update = FALSE)) and _comment_body_field_create() (called by comment's hook_node_type_insert()) create instances of respectively 'body' and 'comment_body':
field.instance.node.foo.body.yml
field.instance.comment.comment_node_foo.comment_body.yml
each with fresh UUIDs.
2) Import all of this to Prod:
What happens in HEAD is:
- thanks to alphabet ordering, the field.* files are imported first. The 'foo' node type doesn't exist yet, but FieldInstance doesn't check that the bundle actually exists (was done of purpose for similar cases in the upgrade path) --> the instances are created.
- node.type.foo.yml is imported. This triggers both NodeType::postSave() & hook_node_type_insert()
We're lucky, both node_add_body_field() & _comment_body_field_create() are wise enough to check and do nothing if the instances already exist (I'm not even sure why they do it). All is good.

If import happened the other way around:
- node.type.foo.yml is imported, node_add_body_field() & _comment_body_field_create() create *new* instances with their own UUIDs
- field.* files are imported - Import crashes because the instances already exist with different UUIDs than in the files (the UUIDs that were generated on Dev).

So, we're lucky now, but this feels really brittle. If another entity type wants to implement similar "autopopulate new bundles an inititial set of field instances", and is provided by a module that starts with [a-e], this explodes...

Proposed solution

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.

Files: 
CommentFileSizeAuthor
#28 2069373-28-test-only.patch677 bytesswentel
FAILED: [[SimpleTest]]: [MySQL] 59,371 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#28 2069373-28.patch7.67 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,714 pass(es).
[ View ]
#28 interdiff.txt2.16 KBswentel
#22 2069373-22.patch7.64 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 59,258 pass(es).
[ View ]
#22 interdiff.txt946 bytesswentel
#20 2069373-20.patch6.72 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 59,265 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#20 interdiff.txt2.47 KBswentel
#8 2069373-8.patch5.02 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

yched’s picture

Title:Auto created 'body' and 'comment_body' field instances fragile on config import» ConfigEntities whose creation triggers the creation of other ConfigEntities play really bad with imports
Component:field system» configuration system

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.

yched’s picture

Title:ConfigEntities whose creation triggers the creation of other ConfigEntities play really bad with imports» ConfigEntities whose creation triggers the creation of other ConfigEntities mess with imports

bit shorter

yched’s picture

Issue summary:View changes

Generalize

beejeebus’s picture

i think i prefer the latter - we could certainly stick something on the entity in importCreate().

we can bikeshed the names later, but i support the idea.

yched’s picture

Title:ConfigEntities whose creation triggers the creation of other ConfigEntities mess with imports» Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B

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

mtift’s picture

Updating tags

yched’s picture

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.

yched’s picture

Issue summary:View changes

more accurate

yched’s picture

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.

swentel’s picture

Status:Active» Needs review
StatusFileSize
new5.02 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Status:Needs review» Needs work
Issue tags:-Configuration system, -Field API

The last submitted patch, 2069373-8.patch, failed testing.

mtift’s picture

Status:Needs work» Needs review

#8: 2069373-8.patch queued for re-testing.

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

The last submitted patch, 2069373-8.patch, failed testing.

beejeebus’s picture

import hooks run during module install (obviously, right?), which means that setSyncing(TRUE) stops default field creation for all the things, which is why so many random tests seem to fail after this change.

there's a patch over at #2095489: Separate out module install config code from import code to fix that.

swentel’s picture

Right makes sense. The body field for article and page are not created on install now, so getting the other one in will make a huge difference already.

yched’s picture

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’s picture

#7 seems like a good idea to me, but i've had very little to do with fields though, so i don't think my vote means much :-)

yched’s picture

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

alexpott’s picture

So here are all the config entities that are created automatically during standard profile install and enabling all the modules.

breakpoint.breakpoint.module.toolbar.narrow.yml
breakpoint.breakpoint.module.toolbar.standard.yml
breakpoint.breakpoint.module.toolbar.wide.yml
breakpoint.breakpoint.theme.bartik.mobile.yml
breakpoint.breakpoint.theme.bartik.narrow.yml
breakpoint.breakpoint.theme.bartik.wide.yml
breakpoint.breakpoint_group.module.toolbar.toolbar.yml
breakpoint.breakpoint_group.theme.bartik.bartik.yml
entity.display.comment.node__comment.default.yml
entity.display.comment.node__comment_forum.default.yml
entity.display.custom_block.basic.default.yml
entity.display.node.book.default.yml
entity.display.node.book.teaser.yml
entity.display.node.forum.default.yml
entity.display.node.forum.teaser.yml
entity.display.node.page.default.yml
entity.display.node.page.teaser.yml
entity.display.user.user.compact.yml
entity.display.user.user.default.yml
entity.form_display.comment.node__comment.default.yml
entity.form_display.comment.node__comment_forum.default.yml
entity.form_display.custom_block.basic.default.yml
entity.form_display.node.book.default.yml
entity.form_display.node.forum.default.yml
entity.form_display.node.page.default.yml
entity.form_display.user.user.default.yml
field.field.comment.comment_body.yml
field.field.custom_block.body.yml
field.field.node.body.yml
field.field.node.comment.yml
field.field.node.comment_forum.yml
field.field.node.taxonomy_forums.yml
field.field.taxonomy_term.forum_container.yml
field.field.user.user_picture.yml
field.instance.comment.node__comment.comment_body.yml
field.instance.comment.node__comment_forum.comment_body.yml
field.instance.custom_block.basic.body.yml
field.instance.node.article.body.yml
field.instance.node.article.comment.yml
field.instance.node.book.body.yml
field.instance.node.forum.body.yml
field.instance.node.forum.comment_forum.yml
field.instance.node.forum.taxonomy_forums.yml
field.instance.node.page.body.yml
field.instance.user.user.user_picture.yml
system.action.user_add_role_action.administrator.yml
system.action.user_remove_role_action.administrator.yml
swentel’s picture

Status:Needs work» Needs review
Issue tags:-Configuration system, -Field API

#8: 2069373-8.patch queued for re-testing.

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

The last submitted patch, 2069373-8.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new2.47 KB
new6.72 KB
FAILED: [[SimpleTest]]: [MySQL] 59,265 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This should at least run al tests.

Status:Needs review» Needs work

The last submitted patch, 2069373-20.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new946 bytes
new7.64 KB
PASSED: [[SimpleTest]]: [MySQL] 59,258 pass(es).
[ View ]

Should be green

yched’s picture

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 ?

yched’s picture

Priority:Normal» Major

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.

swentel’s picture

+++ b/core/modules/node/lib/Drupal/node/Entity/NodeType.php
@@ -170,8 +157,9 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
-      // Unless disabled, automatically create a Body field for new node types.
-      if ($this->get('create_body')) {

Re: removal of 'create_body': because we don't use that property then anymore ? see snippet above ? Or do we want to keep that, it seems redundant.

yched’s picture

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()) { ?

swentel’s picture

Hmm right, I see what you're getting at. Just because no code in core is explicitly setting that property to FALSE, it doesn't mean that it can be used to disallow creating a body field for a node type, for whatever reason. So yeah, we can keep the property. I was not thinking about that use case.

This needs tests too for syncing a node type without a body field I presume, I'll do that one tomorrow evening.

swentel’s picture

StatusFileSize
new2.16 KB
new7.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,714 pass(es).
[ View ]
new677 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,371 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reverted the create_body property. Added test as well.

alanburke’s picture

While trying out the config system, I did note that the body field kept getting added to newly imported content types, even if they didn't have the body field in their definition.
This patch sorts that out - thanks.

beejeebus’s picture

Status:Needs review» Reviewed & tested by the community

looks good to me, test to prove it works, RTBC.

yched’s picture

Agreed, I was about to RTBC myself :-)

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Nice!

Committed and pushed to 8.x. Thanks!

webchick’s picture

Issue summary:View changes

proposed approach

Status:Fixed» Closed (fixed)

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

xjm’s picture