Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating)

Wondering if skipUpdate() as a method name would suffice, instead of setSkipUpdate()...

I spent some time thinking about how Migrate would actually interact with this and I really wasn't pleased with adding any ChangedItem specific checks to the migration system. So I thought how we could make this more generic. I don't think the "skip-update" functionality is something that is generically applicable. Instead I think we should provide a Migrate-specific interface that various objects can optionally implement if they want to provide migrate-specific logic. The attached patch is a proof-of-concept of how I think that could work. The integration on migrate side could probably be solved a bit more elegantly, but I wanted to get some feedback before burning huge amounts of time on this.

Thoughts?

Ahem.

Discussed this with @dawehner a bit. He wasn't particularly fond of making the Entity system aware of the Migration system, and that's a fair point. So we discussed adding a special "don't try to be smart" mode to the entity system itself, and have ChangedItem use that. The Migration system can then simply enable that mode.

Here's a patch for that. I called the thing "preset mode". I'm not too happy with that name, but I couldn't come with anything better.

Oh ViewUI....

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -42,6 +42,15 @@
+  protected $presetMode;

Should probably default to FALSE.

I'd like to hear some thoughts on this, though, before re-rolling.

Especially naming suggestions!!!

Code looks fine and yes, should definitely default to FALSE.

I'm trying to come up with better names, but no clue either, we need some native english speakers in this issue :)

How about readOnly ? Or maybe even locked ?

Yeah, I had readOnlyMode when initially creating the patch but then changed it because of #12. Again, totally not happy with presetMode though. This is a tough one.

Several entity types already have the concept of "locking" already, so that'd also be problematic.

What about freeze() (or setFrozen()) and isFrozen()?

I think the reason it's so hard to find a good name for this is that what this issue is after is partial immutability, and even then it's opt-in for individual fields, the developer cannot specify the partialness of the immutability: it's essentially a blind step "in good faith". Otherwise naming would be simple: setImmutable().

Due to the above, perhaps it is better to put the burden of dealing with this on Migrate?

Oohh, I like the frozen metaphor. It makes it clear that the entity is not to participate in dynamic value assignment (i.e. in some way frozen == !dynamic). People might still assume that frozen means that in fact nothing at all can change (which is not the case), but we can clarify that with documentation I think.

Wim Leers++

Here's a patch for that. Just for the record: I like dedicated, small interfaces a lot more than huge, monolithic ones. That's not what's currently prevalent in core, though, so presumably this is less controversial than something like #5.

No interdiff because I created this from scratch.

@tstoeckler: actually, we have EntityOwnerInterface and EntityChangedInterface already, so it's really not that controversial. You could then even make EntityChangedInterface extend EntityFrozenInterface: that'd make it clear that freezability is required.

Ok, adapted patch for that.

Re #18: Yes, good point! Thanks. Will fix in next patch.

+++ b/core/modules/block_content/src/BlockContentInterface.php
@@ -9,11 +9,12 @@
+interface BlockContentInterface extends ContentEntityInterface, EntityChangedInterface, EntityFrozenInterface {

Why this optionality? Why not do EntityChangedInterface extends EntityFrozenInterface?

@Wim Leers: Maybe someone else has some input on this as well, but my opinion (!) is the following:
While the concrete implementation of the implementors of EntityChangedInterface depends on and is related to EntityFrozenInterface, the interface itself does not. It is possible that someone provides a getChangedTime() method on their entity (perhaps ConfigEntity?) that works completely different than the core implementations. Now, if there were an EntityChangedTrait (*cough* #2209971: Automatically provide a changed field definition *cough) I would certainly promote using EntityFrozenTrait from there, but that - at least to me - is a different story, as traits are specifically about implemenation.

Bump

Hmmm,

when #2428795: Translatable entity 'changed' timestamps are not working at all gets committed, the already existing function ContentTranslationMetadataWrapperInterface::setChangedTime() will become functional again. I assume that this one could simply be used to solve the issue here.
Alternatively something like $entity->set('changed', $timestamp); should work as well.

With that related patch applied the "automation" wihtin an instance of ChangedItem will act like this:

• If the changed timestamp has been set explicitly, use this value.
• If not, set it to REQUEST_TIME, but only on these translations that have been changed really.

This algorithm also works for non-translatable entities.

The patch introduced in #2428795: Translatable entity 'changed' timestamps are not working at all by @mkalkbrenner works when setting the changed timestamps manually during migration on translations, but not when initially creating the entity.

I am attaching a small patch fixing this, which should be applied after the patch from @mkalkbrenner.

I see the problem with new entities but the proposed solution doesn't seem to work because it turns off the normal functionality for non-translatable entities which aren't new.

I missed that one.... An improved version is attached...

I think this one is the easier solution.

but like this you remove the condition check if not translatable?

but like this you remove the condition check if not translatable?

This check was just a shortcut to skip the algorithm below for non-translatable entities.

This algorithm also works for non-translatable entities.

To achieve that, the same shortcut has to be removed either.

I included #28 in #2428795: Translatable entity 'changed' timestamps are not working at all. Can you give it a try?

https://www.drupal.org/node/2453153#comment-10060032 will definitely fix this issue here without any additional patch required.

There it has been solved only for new entities, but we still need a solution for existing entities e.g. currently you can't resave an entity with changes without updating the changed timestamp.

Going though the issue over again I like the solution from #19.

A patch on top of #2985297: Generalize the concept of entity synchronization. Release for testing as soon as the referenced issue is committed.

Quoting @hchonov in #2985297-28: Generalize the concept of entity synchronization:

Right after the patch here is committed, we can add the test coverage for the already provided patch in #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating). I think this would be enough for the core.

That patch is now committed, so let's move this forward :)

I've queued the patch for testing. Let's see what happens :).

So, this patch is expected to solve the 'changed' field freeze in this way?

$entity->set('some_field', ...);
$entity->isSyncing(TRUE);
$entity->save();

Then, even if will achieve the goal, the ->isSyncing() naming is not self-explanatory. We are just using other functionality (sync flag) to resolve this particular case. I think we should have a dedicated method/flag.

What about adding a new property to ChangedItem, along with value? Something like preserve (bool, default FALSE). Then our code would look more meaningful:

$entity->set('some_field', ...);
$entity->changed->preserve = TRUE;
$entity->save();

EDIT: PasswordItem also uses some additional properties to act on save.

EDIT2: Or even a new method: ChangedItem::setReadOnly($flag = TRUE); that sets the flag:

$entity->set('some_field', ...);
$entity->get('changed')->setReadOnly();
$entity->save();

Till this will land, I've built a tiny module that resolves the freeze of fields of type changed when explicitly is required: https://www.drupal.org/project/preserve_changed. IMO, the way this is done $node->changed->preserve = TRUE is self-explanatory compared to $entity->isSyncing(TRUE); which has no meaning in this context.

I'd agree with @claudiu.cristea that ->isSyncing() doesn't make much sense for this use case.

ChangedItem::setReadOnly($flag = TRUE); probably makes the most sense.

Over in #2803717: Allow 'syncing' content to be updated in content moderation without forcing the creation of a new revision which is also proposing using the isSyncing flag I proposed something like so:

Should we consider expanding setSyncing to work with bitwise flags instead of booleans?

$entity->setSyncing(EntityInterface::SYNC_OPERATION_MIGRATION);

/// And then.

$entity->isSyncing(EntityInterface::SYNC_OPERATION_MIGRATION);

Then we can move away from it being a binary flag?

I like the SynchronizableInterface approach, since it means callers can rely on syncing semantics without having to inspect or understand if an entity type has a ChangedItem field or what that field is named. I've spun off #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities to try and make it clear based on the interface docs that it's actually a good choice for this exact issue.

IMO, #50 is a good arguments against a solution specific to the changed field type..

We started using the syncing semantic to prevent creations of new revisions, updating specific states and so on. We just assume that when the entity is synching then it is not in the regular state of updating. There might be a lot of cases why the entity is in a synching state, but no matter the reason it is not going to be a regular user interaction.
In all that cases the changed field should not be updated, because if we skip creating a new revision then we'll be manipulating the real time of the change performed by the user.

I was having a look at this. I think this will probably be easier to get in if the migration changes are split and moved into: #3052115: Mark an entity as 'syncing' during a migration update.

I think it makes sense add specific tests for migrations as well as tests that check ChangedItem in isolation, so splitting will probably reduce some complexity.

Straight reroll, may look into #53 shortly.

I went to go write some tests for this, but on the whole the testing for ChangedItem seemed complicated and brittle, so I've logged #3064727: Remove the REQUEST_TIME constant from ChangedItem and CreatedItem and delete ChangedTestItem in favour of a mock programmable time service in the meantime.

Patch on top of #3064727: Remove the REQUEST_TIME constant from ChangedItem and CreatedItem and delete ChangedTestItem in favour of a mock programmable time service, which delegates the migration work to another issue and adds test coverage for ChangedItem.

I think this will probably be easier to get in if the migration changes are split and moved into: #3052115: Mark an entity as 'syncing' during a migration update

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

+1

With this, we need to allow the user to be able to choose if the date needs to be changed on user cancellation (i.e. content reassignment for Anonymous user) or not.

UI could be done from contrib then. I don't think the UI for this should be provided from core as this is nothing a normal editor needs.

Ah sorry, on user cancellation and content reassignment, yes of course, there it makes sense.

I thought a global checkbox under the account settings or a maximum per content type and not on the entity edit page.

EDIT: Ehh you were faster.

The https://www.drupal.org/project/drupal/issues/3064727 was a duplicate of https://www.drupal.org/project/drupal/issues/2809515, so I've done an update after the second issue.
I've also extracted the base of this issue for easier updates later on (it's in the do not test patch).

Great I've uploaded a 0-byte file...

I re-rolled the patch.

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. any suggestion?

Any suggestions are:

1. Read through this issue
2. Try out the patches
3. Provide feedback

Thank you

Re-rolled the patch from #71 for 8.9.x.

Re-rolled the patch from #71 and committed to the issue fork.

Rerolled #71 for 9.3.x.

Needed because of #3131281: Replace assertEqual() with assertEquals()

Also replaced a reference to \Drupal::time() with $this->time() to improve testability - hopefully it doesn't break the test.

Tested the patch locally, it works perfectly. So I next went to review this in detail:

1. 👍 95% of this patch is to ensure everything in ChangedItem is using the time service instead of the REQUEST_TIME global. That is necessary to allow for testability.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -41,13 +43,16 @@ public function preSave() {
   +      if (!$entity instanceof SynchronizableInterface || !$entity->isSyncing()) {
   

   👍 This is the only actual functional change!

   It's very simple: it wrap the existing logic in this if-statement. The existing logic already ensured we only update the changed timestamp when updating.

   This addition ensures that we only update the changed timestamp when not syncing.

   The prime example of syncing (content) entities in Drupal core are migrations.

3. +++ /dev/null
   @@ -1,46 +0,0 @@
   -class ChangedTestItem extends ChangedItem {
   

   👍 This becomes obsolete thanks to ChangedItem no longer using REQUEST_TIME.

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
   @@ -472,6 +504,30 @@ public function testRevisionChanged() {
   +  /**
   +   * Tests the changed functionality when an entity is syncing.
   +   */
   +  public function testChangedSyncing() {
   +    $entity = EntityTestMulChanged::create([]);
   +    $entity->save();
   +    $changed_time_1 = $entity->getChangedTime();
   +
   +    // Without the syncing flag the changed time will increment when content is
   +    // changed.
   +    $this->time->advanceTime();
   +    $entity->name = 'updated content';
   +    $entity->save();
   +    $changed_time_2 = $entity->getChangedTime();
   +    $this->assertTrue($changed_time_2 > $changed_time_1);
   +
   +    // With the syncing flag, the changed time will not change.
   +    $entity->name = 'updated content again';
   +    $entity->setSyncing(TRUE);
   +    $entity->save();
   +    $changed_time_3 = $entity->getChangedTime();
   +    $this->assertEquals($changed_time_2, $changed_time_3);
   +  }
   

   👍 This is proving that what we aim to do is working.

   Without the first piece of code, the last assertion here will fail: even when syncing, the changed timestamp will have changed!

5. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +  public function advanceTime($seconds = 1) {
   +    $this->requestTime += $seconds;
   +    return $this;
   +  }
   

   👍 This is the crucial piece in this dummy/test-only Time service: it allows us to control time for testing purposes 🤓

… this patch is perfect! 🤩

It is hugely disruptive for migrations that involve content that has been updated upstream — i.e. this patch is pretty much essential for any migration of a relatively big Drupal 7 site.

Let's get this committed!

Created a change record: https://www.drupal.org/node/3250104

Just a few quick nits.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/ChangedItem.php
   @@ -41,13 +43,16 @@ public function preSave() {
   +          if ($this->value == $original_value && $entity->hasTranslationChanges()) {
   

   Do we really need to do loose comparison (==) or can we use strict (===)? I guess that isn't a change from previous code, so maybe we can ignore it.

2. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +   * @see \Drupal\Tests\search_api\Kernel\TestTimeService::getRequestTime()
   

   Nit: copy/pasta.

3. +++ b/core/tests/Drupal/KernelTests/TestTime.php
   @@ -0,0 +1,49 @@
   +  public function advanceTime($seconds = 1) {
   

   Can we type hint this as an int?

1. Content Entity API is not type safe, never use === with field values unless you explicitly cast them yourself.

@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. Without this fixed, that migration cannot be made to work correctly.

Unless I'm missing something, I think the patch in referenced issue #3052115, which marks an entity as "syncing" during a migration update, is needed for this patch to work (#78) in that use (i.e. when updating previously migrated nodes with e.g. "drush mim upgrade_d7_node_article --update").

With that caveat, it seems to work for me.

It also seems to me that the title for this issue (Allow the ChangedItem to skip updating when synchronizing) could be read to mean that we want to skip updating the entity in its entirety when syncing. In fact we just want to skip updating the "changed" field for that entity. right? I've revised the title to clarify.

This and #3052115 have not been enough for my site to fix the problem. I opened #3280140 in migrate_tools as a sort of "meta" issue because I suspect a lot of people are running into this problem because of that module.

Linking issues from #87 :)

Fixed 2. and 3. from #80. (1. can/should not be fixed per #81.)

Didn't quite grok what's going on with the merge request so posting an updated patch.

Looking into the tests, I believe some of this data was covered up by using incorrect values in the source database.

For example, in drupal7.php node 1 has the following in the "node" table:

->values(array(
  'nid' => '1',
  'vid' => '1',
  'type' => 'test_content_type',
  'language' => 'en',
  'title' => 'A Node',
  'uid' => '2',
  'status' => '1',
  'created' => '1421727515',
  'changed' => '1441032132',
  'comment' => '2',
  'promote' => '1',
  'sticky' => '0',
  'tnid' => '0',
  'translate' => '0',
))

For comparison, "node_revisions" has the following:

->values(array(
  'nid' => '1',
  'vid' => '1',
  'uid' => '1',
  'title' => 'A Node',
  'log' => '',
  'timestamp' => '1441032132',
  'status' => '1',
  'comment' => '2',
  'promote' => '1',
  'sticky' => '0',
))

This data is consistent - the original creation was 1421727515, which corresponds to values in both "file_managed" and "taxonomy_index", and it was then updated at 1441032132.

However, after the migration runs the "created" and "changed" values are set to 1529615790, which come from "entity_translation" and "entity_translation_revision". That points to errors in the source data - the time values in "entity_translation" and "entity_translation_revision" should be the same as what "node" and "node_revisions" contain.

An example of what would be necessary to fix the test coverage:

diff --git a/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeCompleteTest.php b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeCompleteTest.php
index 381513ed0a..c2c99e7785 100644
--- a/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeCompleteTest.php
+++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeCompleteTest.php
@@ -231,8 +231,8 @@ protected function expectedNodeFieldDataTable() {
           'status' => '1',
           'uid' => '2',
           'title' => 'An English Node',
-          'created' => '1529615790',
-          'changed' => '1529615790',
+          'created' => '1421727515',
+          'changed' => '1441032132',
           'promote' => '1',
           'sticky' => '0',
           'default_langcode' => '1',
@@ -475,8 +475,8 @@ protected function expectedNodeFieldRevisionTable() {
           'status' => '1',
           'uid' => '2',
           'title' => 'An English Node',
-          'created' => '1529615790',
-          'changed' => '1529615790',
+          'created' => '1421727515',
+          'changed' => '1441032132',
           'promote' => '1',
           'sticky' => '0',
           'default_langcode' => '1',

That would correct the expectations of what node 1's data should look like after migration.

Correction - a single run migration wouldn't trigger this, the problem is when an existing entity is updated, and I don't think core's upgrade path cares about that. However, the discrepancy in the test data is still confusing.

I opened a separate issue for the data inconsistency problems: #3303925: D7 upgrade migration test shows incorrect node created, updated values are migrated

the problem is when an existing entity is updated, and I don't think core's upgrade path cares about that.

… but it should, shouldn't it? 🤔

Exactly same as #90, just re-rolled for 9.4.x

Again, re-rolled for 9.4.9

Thank you, @junaidpv! 😊

What is in the way of an RTBC here? 😅 @heddn's nits from #80 were addressed by @tstoeckler in #90 … which means this actually has been ready for >6 months! 🫣

Actually, looks like this needs a reroll for 10.1.x (this won't land in 9.5.x most likely).

added patch against #97 in 10.1.x

Thanks!

PHPStan is complaining about various errors in #101.

It's complaining because this replaces a lot of REQUEST_TIME usages, so the ignore file needs to be updated.

Got hit again by this issue during migration. Worked on re-roll.

Test core/tests/Drupal/KernelTests/TestTime.php is missing in patch #101.

Rerolled #101 against 11.x and added missing test.

    Ignored error pattern #^Call to deprecated constant REQUEST_TIME\:                                          
     Deprecated in drupal\:8\.3\.0 and is removed from drupal\:11\.0\.0\. Use                                    
     \\Drupal\:\:time\(\)\->getRequestTime\(\); $# in path                                                       
     /var/www/html/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ChangedTestItem.php  
     was not matched in reported errors.           

These errors need to be removed from the baseline to prevent failures.

Seeing a lot of changes for $this->value = REQUEST_TIME; and reading the proposed solution not super clear how they are connected. If that could please documented.

Updating the proposed solution to document the changes of $this->value = REQUEST_TIME;, see #79.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Related:#3052115: Mark an entity as 'syncing' during a migration update landed 👍

Seems #111 is a false positive as patch applies cleanly against 11.x in #108.

Setting again for needs review

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

patch failing to apply is legit. the green test was two weeks ago, requeued one that failed like the bot.

Rerolled #108 against latest 11.x Head.

Tested by using

  $node = Node::load(1);
  $node->set('title', 'A');
  $changed_original = $node->getChangedTime();
  echo ('Old changed date:' . $changed_original);
  echo ('<br>');
  $node->save();
  $changed_current = $node->getChangedTime();
  echo ('Current changed date:' . $changed_current);

Verified date changed
Applied MR 4203

  $node = Node::load(1);
  $node->set('title', 'B');
  $changed_original = $node->getChangedTime();
  echo ('Old changed date:' . $changed_original);
  echo ('<br>');

  $node->setSyncing(TRUE);
  $node->save();
  $changed_current = $node->getChangedTime();
  echo ('Current changed date:' . $changed_current);

Verified date is the same.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/CreatedItem.php
@@ -16,14 +16,34 @@
-    $this->setValue(['value' => REQUEST_TIME], $notify);
+    $this->setValue(['value' => $this->time()->getRequestTime()], $notify);

This seems out of scope, I don't think we need to change CreatedItem in this patch.

A lot of the test changes seem out of scope as well.

To be able to update the change time during syncing the EntityChangedConstraintValidator must also be updated

Wrong patch

Updated the MR, fixed conflicts, and added in the change to the validator in the patch in #123 Still need to address @longwave's comments in #119

Re-rolling the #116 for 10.2.5

Probably too late to say this, but as much as I understand the reasoning in #50, I still think a solution like in #45 would still make sense *additionally*. IMHO there should be a chain: isSyncing() should internally rely on sth. like #45. Even in the issue summary it says "f.e.(!) when migrating". Now, the solution is ONLY about migrating. What if I just want to skip updating the changed date? I don't really know what setSyncing() might do now or in the future. I still think there's a valid reason to just want to skip changing the changed date. With the solution outlined here (AFAIU) I would have to pretend/claim that I'm syncing sth, when in fact I'm not.

@rgpublic: I totally agree.

We often find ourselves in the situation of wanting to update entity field values in a deploy hook. Changed assumptions, bad data entry, whatever.

Or admins need a way to generate and save a reference to an external api entity that failed to create (flaky api, whatever).

In both cases we don't want to mess with the changed timestamp (that just confuses editors), and we are not "syncing" a migration.