Coming from #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter, though I think this was actually introduced in another issue originally.

That patch added a lot of lines like this:

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

This requires module developers to a) know about this method, b) know they're supposed to check it, c) know exactly when/why to do so. This would be a lot easier to communicate if it were an argument in the hook itself, because then we could just document it.

Thoughts?

Comments

yched’s picture

Finding a way to make the existence & importance of the isSyncing() flag more obvious to folks interacting with config entity CRUD has been a concern for the past months, yes.

And yes, adding it as a param to the hook would be a great way,
BUT the hooks are shared with content entities for which this notion doesn't exist.

We could choose to simply omit the param for hook_[ENTITY_TYPE]_[CRUD]() implementation for content entities, but this still leaves a high chance of people copy/pasting the wrong ones and ovelooking the $is_syncing thing completely.

In short, yep, +[a lot] on the problem description, but I don't really have new insight on the solution :-/

sun’s picture

A "synchronization" also exists for content entities, as soon as someone/anyone develops a working mechanism to stage/deploy content across sites.

What we're doing with config right now is merely a one-off "experiment of how staging could possibly work out." — Once the design works, it's going to be boilerplate.

Meanwhile, contrib will try to make the impossible possible. Contrib will need this parameter, in the same way as core needs it for config.

→ No reason to limit this to config entities. Let's forget about that concern.

It's not about config vs. content. It's about staging.

yched’s picture

@sun: possibly, but note that the isSyncing() flag only has meaning because config sync is a "full sync" of the complete config tree - hence the need to avoid secondary writes reacting to an entity write, that will overwrite or get overwritten as the sync process progresses.

Even if content entities get some level of deployability some day, it's not sure the challenges and constraints will map.

sun’s picture

the isSyncing() flag [is] only need[ed] to avoid secondary writes reacting to an entity write, that will overwrite or get overwritten as the sync process progresses.

Correct. Any attempt to stage content entities is guaranteed to run into the exact same problem of secondary writes.


While being there, let's change the name, please... "isSyncing" is 100% ambiguous — Does it sync the save? Sync the database? Sync the original user input? Oh, does it explicitly sync secondary writes? :P What does it sync? ;-)

yched’s picture

Yeah, it means the entity is being synced, actually.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.