Allow ChangedItem to skip updating the entity's "changed" timestamp when synchronizing

Got hit 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: ✨ Mark an entity as 'syncing' during a migration update Needs work 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.

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

Wrong patch

mikelutz → changed the visibility of the branch 2329253-allow-the-changeditem to hidden.

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.

kksandr → made their first commit to this issue’s fork.

@longwave I opened a new merge request #8536, it has minimal changes.

All refactoring/optimization was removed, I also removed the validation omission during synchronization, this seemed unrelated to the problem.

But manually changing the request time instead of waiting in ChangedTestItem looked interesting, it might speed up testing a bit.

kksandr → changed the visibility of the branch 2329253-allow-the-changeditem-11.x to hidden.

Ran the test-only feature

1) Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testChangedSyncing
Failed asserting that 1719840325 matches expected 1719840323.
/builds/issue/drupal-2329253/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php:495
FAILURES!

Which shows the coverage

Applied a nitpicky change for the void return.

Issue summary appears fine to me

LGTM

I read the issue summary and the comments and the latest MR. The first thing I noted is that the proposed resolution is out or date, it no longer touches the time service. And then at the end of the comments a new MR was created that removed code. Unfortunately, I haven't found a way to compare 2 MR with the UI so it is difficult to confirm those changes.

There were discussion here of alternate ideas as well as wanting the solution to apply to more that just the 'changed' field. #45, #49. $51, #128, #129. Is a follow up needed for those? I am not sure.

I have updated the issue summary.

Leaving at RTBC

Updating issue credits

Committed to 11.x

Per https://www.drupal.org/about/core/policies/core-change-policies/allowed-... → this isn't eligible for backport to 11.0.x.

Glad to see this one resolved.

larowlan → closed merge request !8536