Mark an entity as 'syncing' during a migration update

Blocked only on #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) now. Adding blockers to related.

+1

Reroll from current 9.1.x HEAD

Edit: this works well to prevent content_moderation to create new revisions during migrations.
I didn't have time to look at those tests failures yet, and I'm not sure how to deal with some of those.

This really needs an issue summary update. ;-)

There is no reason to postpone it just because it needs issue summary update.

@hchonov, I set this to postponed because the IS says it is blocked on #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating)

I ran into the same problem and already created an issue here, #3195139: migrate:import --update creates new revisions, if content_moderation active.
(or rather different problem, same solution)
It is now a duplicate.

About the patch:
Would it not be simpler to call the ->setSyncing() in the ->save() method?
Do we need this for anything else? What is the effect e.g. on delete?

Also, for these temporary state changes I usually use try / finally, like so:

$syncing = $entity->isSyncing();
try {
  $entity->setSyncing(TRUE);
  $entity->save();
}
finally {
  $entity->setSyncing($syncing);
}

This makes sure the state is restored even if an exception occurs.

I could upload a new patch, but perhaps I am missing something essential.

Hi, I am also facing a similar issue updating the 1k volume of existing content.
But at the same time, I don't want to update the changed time. I am working
on the Drupal 8.9 Version. above patch #11 not working for me!! any suggestion?

I don't think this issue is currently blocked by the other.

Should this be behind a configuration flag, is there ever a use-case where one would like to keep the current "non-sync" behaviour?

I think it'd be nice if the migration destination plugins have some kind of "pre/post-migration-save" hooks which can allow third party modules to manipulate the destination entities before they're saved without having to extend and reimplement the destination plugins with the desired behaviours.

This should improve DX and avoid having to wait for core's release cycle to get new features in, allowing contrib modules like migrate_plus to introduce more helpful APIs to handle issues like this.

@huzooka and I are running into this same problem. He spent days pinning it down to this root cause! 🤯

Confirmation of the problem and clarifying it more 🔍

1. @huzooka has been working on https://www.drupal.org/project/workbench_moderation_migrate, whose goal is to migrate all of that per-revision-moderation-state-metadata into Drupal core's content_moderation module. Added Contributed project blocker tag for that. Also bumped to Major, because it's making migration of D7 contrib data into the succeeding D8|9 module in Drupal core impossible.
2. @donquixote's report in #17/#3195139: migrate:import --update creates new revisions, if content_moderation active is almost exactly the scenario we encountered, but slightly different. He reports it happens specifically when A) updating, B) using the drush migrate:import command.

   We can reproduce it even when not updating, just importing, and without using drush. All you need is:

   1. a node with two revisions, where the higher revision ID has a lower changed timestamp
   2. with d7_node_complete configured to set high_water_property for its source plugin: high_water_property: { name: changed, alias: n }.

   Both have the same root cause: a single migration plugin's source row is being imported multiple times.

   (Using high_water_property is essential if you want to have remotely acceptable incremental migrations on big Drupal 7 source sites — it'd take prohibitively long otherwise.)

3. @huzooka has an idea how he can write a failing migration test in Drupal core that showcases this problem in a crystal clear way. Stay tuned for a patch :)

Getting this moving forward again 🤞

So, I started looking at why this issue is actually marked Postponed on #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating). AFAICT that happened because this was originally split out from #2329253, to reduce the scope there. But per @hchonov in #2329253-59: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating):

@Sam152++, this totally makes sense. The patch is small, but that are two different issues. This separation also kind of shows that we want that to happen independent of the changed item.

… which confirms what I suspected: this issue can land independent of that other one!

Because then at least the migration can succeed, even though when updating already migrated entities, until #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) lands, their changed field will be set to the current time inappropriately. Still, that is better than a failing migration.

FYI: just RTBC'd #2329253 after a detailed review: #2329253-79: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating).

Here is a test-only patch.

I already have a local fix, but for first I want to check all the tests which were failing in #11.

AND: While I was checking all the destination plugins I noticed that the EntityShortcutSet plugin was created only to call setSyncing(TRUE) before the save!

1. 🥳
   

   00:29:35.039 Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7TestWit   0 passes   1 fails                            
   

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7TestWithContentModeration.php
   @@ -0,0 +1,80 @@
   +    \Drupal::service('module_installer')->install(['content_moderation']);
   

   Why do we need this if you also updated protected static $modules? 🤔

3. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7TestWithContentModeration.php
   @@ -0,0 +1,80 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getSourceBasePath() {
   +    return __DIR__ . '/files';
   +  }
   

   Nit: This is the same as the parent; can be removed.

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7TestWithContentModeration.php
   @@ -0,0 +1,80 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getEntityCountsIncremental() {
   +    return parent::getEntityCountsIncremental();
   +  }
   

   Nit: can also be dropped.

Addressing #26, and adding a similar test for Drupal 6 migrations.

We have to fight A LOT with the preexisting migrations since some of their destination entities (like Role) are doing some data petting in a presave: for example, Role sorts its permissions if the actual entity isn't syncing...

Are we sure that we want to do this for content and config entities?

config entity sync is something very different from content entity sync conceptually.

I would assume that the main benefit here is content entities and config entities is an edge case. We could consider that in a separate issue?

The existence of EntityShortcutSet destination plugin and thefact that in the Drupal 6 test there are 6 extra (duplicated ?) actions after the import prove that config entities are in scope.

But I don't say they shouldn't be fixed in a separate issue.

This is a content-only fix.

Test/proof of necessity review

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6Test.php
   @@ -206,6 +203,8 @@ public function testUpgradeAndIncremental() {
   +    $this->assertEntityRevisionsCount('node', 26);
   

   Before this patch:

   The number of node revisions is different than expected
   Failed asserting that actual size 80 matches expected size 26.
   

   😱

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php
   @@ -230,6 +230,8 @@ public function testUpgradeAndIncremental() {
   +    $this->assertEntityRevisionsCount('node', 19);
   

   Before this patch:

   The number of node revisions is different than expected
   Failed asserting that actual size 37 matches expected size 19.
   

   😱

→ proven beyond any doubt! 👍

Functional change review

1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -209,6 +210,9 @@ public function rollback(array $destination_identifier) {
   +      if ($entity instanceof ContentEntityInterface) {
   +        $entity->setSyncing(TRUE);
   +      }
          $entity->delete();
   

   🙏 Can you add a comment here explaining why we do this only for content entities, and not config entities?

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -244,6 +244,7 @@ public function validateEntity(FieldableEntityInterface $entity) {
   +    $entity->setSyncing(TRUE);
   

   Previous patches were doing a instanceof \Drupal\Core\Entity\SynchronizableInterface check first.

   But both content and config entities are always synchronizable since sue #2985297: Generalize the concept of entity synchronization. So … that makes sense.

   But I think it's still safer to have that instance check simply because EntityInterface does not implement this interface; it's theoretically possible that non-content, non-config entities are being migrated…

On the functional part. The second call is in the content entity specific destination, so that does not require an instanceof check. For the first part, maybe we could do that in that content entity specific implementation too? If we later on want to do it for config too then I can see the benefit of keeping it in the base class but we could still move it back if we want later?

The second call is in the content entity specific destination, so that does not require an instanceof check.

D'oh, of course! 😅

For the first part, maybe we could do that in that content entity specific implementation too? If we later on want to do it for config too then I can see the benefit of keeping it in the base class but we could still move it back if we want later?

@huzooka just explained to me that he did that to address your feedback in #28 👍

He also just explained to me that we could do what you're describing but then he'd have to change it in both EntityContentBase (for e.g. d7_node) and EntityContentComplete (for e.g. d7_node_complete). But then there's a complication due to \Drupal\migrate\Plugin\migrate\destination\EntityContentComplete::rollback()'s implementation:

  public function rollback(array $destination_identifier) {
    // We want to delete the entity and all the translations so use
    // Entity:rollback because EntityContentBase::rollback will not remove the
    // default translation.
    Entity::rollback($destination_identifier);
  }

… so we'd need to copy/paste basically all of the code from Entity::rollback() into EntityContentComplete::rollback(), so … it'd end up not simplifying anything 😳

And there could be contrib modules too that extend Entity and not EntityContentBase or EntityContentComplete: for example \Drupal\entity_reference_revisions\Plugin\migrate\destination\EntityReferenceRevisions::rollback() uses the Entity one, because it chose to not extend EntityContentBase!

IOW: also for maximum compatibility, the current approach is best/safest.

Considering all that … I think this is good to go as-is 😊

As far as I'm concerned, this is RTBC if we update the issue summary to specify that this is only tackling the problem for content entities, not config entities; and to file a follow-up to also do this for config entities. What do you think, @Berdir?

Sounds good to me. I didn't mean to suggest that we should not do it for config entities, just that I suspect it's basically a 50/50 switch of intended and unintended consequences from a quick glance while content entities gives us big improvements (revisions and update fields together with the other issue) without known drawbacks, so it seems like a good idea to split it up.

50/50 intended and unintended consequences

😅 — that's exactly what @huzooka said! :D

So does that mean you also think this is RTBC as soon as the follow-up issue exists and the issue summary here has been updated?

Discussed at #3253983: [meeting] Migrate Meeting 2021-12-16 2100Z. mikelutz, jibran and myself. As already tagged, this needs and Issue Summary update and without that it makes this difficult to review. We thought this is a feature request with priority of normal. We didn't have time to read all the comments so tagging 'Needs subsystem maintainer review'.

Setting to Needs work for an issue summary update. It would help to include the justification for the suggested change.

Thanks.

Would it be useful to have more comments in the code (not the tests) to explain what the changes mean?

@DamienMcKenna, as a reviewer I would rather have the IS up to date so that I can then determine if the code is doing what it should and in this case how it fits in with the related issues. The IS implies that the changed timestamp is incorrect, is that for all migrations or just some? Nor does it explain how introducing syncing will fix it.

Patch 34 fails on 9.3.2. Attached is a reroll that applies.

Updated issue summary per comment #43 (https://www.drupal.org/project/drupal/issues/3052115#comment-14345378) in hopes of getting this much-needed patch moving again.

This patch was not enough on its own to resolve the problem of content revision timestamps being modified when a migration is reran (migrate_tools, drush migrate:import MIGRATIONNAME --update), so I'm testing to see if #2329253 also helps.

Just to mention it, the site I'm having this problem on uses d7_node_complete for the migration instead of d7_node, so it's hitting the exact problem Wim mentioned in #37.

@DamienMcKenna: Any chance you'd be willing to do a thorough review + test the patch so we can mark this RTBC and get this in? 🤞

reroll for 9.4

Format issue links in the IS.

+1 I can confirm that the latest patch from this issue, combined with the patch from #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) solves the most common underlying issue facing people that are running multiple migrations: #3280140: Running migrate:import for node entity with --update modifies CHANGED timestamp.

Note: we are not migrating revisions, so sadly I cannot do a complete review here.

+1, patch 53 combined with #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) fixes the migrated "changed" dates for us. Without these two patches, the changed date would be set to the migration date on the destination.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

reroll for 10.1.x-dev

The patch really works well for me, +1 RTBC.

Also, found one more issue related to the content syncing, but create a separate issue for that because it's related not only to the migration -
#3338260: Skip changing 'revision translation affected' field if the old revision is syncing

Also, I agree with other people that the patch from #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) is very useful in this case too.

@rclemings: Thanks for updating the issue summary. I am keeping the tag for that because I think it still needs work.

@HitchShock: Thanks for updating the patch (as a MR). When updating an issue that was previously RTBC but needs a "reroll", it is OK to mark the issue RTBC again. Otherwise, it is unusual to mark an issue RTBC when you have worked on the code.

I am setting the status back to NW for several reasons.

1. Is this issue a bug report?

   I think there is a pretty good argument that it is. If so, then the issue summary should have steps to reproduce. It looks as though the automated tests implicitly have those steps, but it helps to have them in the issue summary. @quietone already made this point. (See Comments #42, #43.) There may be enough in the existing comments (such as #22 and #35 from @Wim Leers) to make the case that this issue is a bug. See also #11, #55.

   If this issue is not a bug report, then we should not change the behavior unconditionally. We can add an option to the destination plugin to use the new behavior. I see that @codebymikey already made this point in #21.

2. Do not change the Entity base class.

   This is just a question of OO design. The base class should handle common functionality, and the child classes should handle more specific requirements. In this case, EntityContentBase::rollback() can in-line the parent method (2 or 3 lines) instead of modifying the parent method.

   I see from #36 to #38 that this has already been discussed, and it is more complicated than that. Still, I wonder if there is a better way.

3. Maybe I am reading too much into the phrase, ‘simply flags an entity as "syncing"’ in the issue description. It suggests to me that this issue will not have any effect itself, and follow-up issues will use that flag. @Berdir knows the effects of setSyncing(), but for the rest of us it would help to have an explanation in the issue summary.

Patch #47 works for me on drupal/core 9.3.22

Hi @benjifisher
About your points:

1. At first glance, this is an extension of the issue #2985297: Generalize the concept of entity synchronization. Only this time we allow marking of migrated entities.
   That is, it seems that this is not a bug fix, but the introduction of the new feature. A feature that will allow us to fix #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) and #3338260: Skip changing 'revision translation affected' field if the old revision is syncing in the future.
   But at the same time, this solution automatically solves the issue #3195139: migrate:import --update creates new revisions, if content_moderation active, or rather, the issue will be fixed by the issues #3052115: Mark an entity as 'syncing' during a migration update and #2803717: Allow 'syncing' content to be updated in content moderation without forcing the creation of a new revision.
   So, in the end, this is not just a new feature, but a new feature + a bug fix.
   This is also confirmed by the tests for content_moderation that were created as part of this issue.
   Not sure which category should be used in this case.
2. As for me, I agree that it's ok to change the base class of the destination plugin. The main goal is to mark all the entities that are migrated, so we need to provide this "flag" to all plugins that extend the base.
3. I don't know what exactly you mean, but it's pretty clear to me from the issue description and the related tickets.

Done:

• Added #3195139: migrate:import --update creates new revisions, if content_moderation active as the related issue and mentioned it in the description.
• Updated tests according to #3336578: Move migration tests to the Forum module

Also, the patch was tested on real projects.

It's RTBC for me.

I've been running this for some time along with the related patches in order to fix various issues with migrations; I think it's about time that this one lands in order to be able to move some of those other issues along - hopefully we can get those into 10.2.0 as well.

Removing the issue summary update tag as it's no longer required. Leaving the followup tag as I think #38/39 intend to discuss doing this for config entities as well?

Committed and pushed 68c1000f770 to 11.x (10.2.x). Thanks!

Reverted, because this broke HEAD: https://www.drupal.org/pift-ci-job/2693233

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6TestWithContentModeration::testUpgradeAndIncremental
LogicException: Missing bundle entity, entity type node_type, entity id forum.


/var/www/html/core/lib/Drupal/Core/Entity/EntityType.php:877
/var/www/html/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php:241
/var/www/html/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:71
/var/www/html/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:89
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:381
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:321
/var/www/html/core/modules/workflows/src/Entity/Workflow.php:114
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:528
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:483
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:609
/var/www/html/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6TestWithContentModeration.php:48
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

And similarly in the other new test

1) Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7TestWithContentModeration::testUpgradeAndIncremental
LogicException: Missing bundle entity, entity type node_type, entity id forum.

/var/www/html/core/lib/Drupal/Core/Entity/EntityType.php:877

@longwave
Updated MR from 10.1.x and there are no test errors atm.
Looks like it was some temporary issue. Sometimes I see that right after the merge, there may be errors in the tests, but a second run shows that everything is fine.

It looks like I accidentally committed the patch instead of the MR! I got the right one this time and ran the tests locally to double check.

Committed and pushed cd1b41fdbf to 11.x (10.2.x). Thanks!

Setting to NW for the followups.

I made the follow up #3425339: Mark a config entity as 'syncing' during a migration update

This seems like a meaningful change for sites with ongoing migrations and content moderation enabled. Yet I did not see this mentioned in the release notes for 10.2 (sorry if it is there and I missed it). As a result we lost expected revision records since we updated to 10.2 a few weeks ago, which makes it hard to see what the net change is per migration update. The change is not present in 10.1.

By the way I agree with the change. Just wish it had been called out. The easy fix to go back to how things had been working was a presave hook:

/**
 * Implements hook_entity_presave().
 */
function mymodule_entity_presave(EntityInterface $entity) {
  if (($entity->getEntityTypeId() == 'node') && $entity->isSyncing() && !$entity->isNew()) {
    $entity->setNewRevision(TRUE);
    $entity->isDefaultRevision(TRUE);
  }
}

I found myself in a similar situation as @john.oltman above.

I was relying on 10.1 behaviors to migrate entity revisions' moderation_state values and (I think) relying on ModerationHandler::onPresave() set publish state accordingly.

@john.oltman, thanks for the solution!

Hi! I created a change record for the 10.1/10.2 changed behavior. Is this needed? If so, could someone review it and then I'll publish it?

https://www.drupal.org/node/3426397