Impossible to update an entity revision if the field value you are updating matches the default revision.

Rather than removing those lines of code I wonder if this might work:

diff --git a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
index d696a94..e2350fd 100644
--- a/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1182,7 +1182,7 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
       $vid = $id;
     }
 
-    $original = !empty($entity->original) ? $entity->original : NULL;
+    $loaded_revision = $this->entityManager->getStorage($entity_type)->loadRevision($entity->getLoadedRevisionId());
 
     // Determine which fields should be actually stored.
     $definitions = $this->entityManager->getFieldDefinitions($entity_type, $bundle);
@@ -1198,7 +1198,7 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
 
       // When updating an existing revision, keep the existing records if the
       // field values did not change.
-      if (!$entity->isNewRevision() && $original && !$this->hasFieldValueChanged($field_definition, $entity, $original)) {
+      if (!$entity->isNewRevision() && $loaded_revision && !$this->hasFieldValueChanged($field_definition, $entity, $loaded_revision)) {
         continue;
       }

Yes, was going to suggest the same. But yes, performance is definitely a problem, we either need #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() or some wrapper to re-use the same revision object.

Would this not be solved by #2833084: $entity->original doesn't adequately address intentions when saving a revision ? Seems related, at least.

Yes, if we decide that we change ->original to something else. But we'll have to review every usage of ->original in core as part of that issue either way and fix it.

@Berdir:

I read all these related issues and I think this is actually a duplicate of #2833084: $entity->original doesn't adequately address intentions when saving a revision. It seems the goal over there is fixing the same problem reported here. Maybe you had #2839195: Add a method to access the original property in mind as the issue meant to provide an alternative to $entity->original?

IMHO the only way to properly fix this is no longer use $entity->original and switch to dedicated accessors for the source/original revision and the original default revision.

I also think repurposing the $entity->original property to always contain the source revision is the kind of behavior change we are allowed to perform in a minor, so we could keep this issue to explore a non BC-compatible approach quickly fixing $entity->original and #2833084: $entity->original doesn't adequately address intentions when saving a revision for a proper fix?

What about this? It should limit the performance penalty to only when storing pending revisions. And once we have #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision(), there would be no difference...

That was almost too easy :) Should we also add an explicit method

As discussed, lets start a change notice for this (one that we might extend/update with the other two issues).

So you're proposing that we'd add a method for this in #2833084: $entity->original doesn't adequately address intentions when saving a revision, wondering how much more work it would be to do that here. Or what else would imply a "proper fix" exactly?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -369,6 +369,15 @@ protected function doPreSave(EntityInterface $entity) {
+    if (!isset($entity->original) && $this->entityType->isRevisionable() && !$entity->isDefaultRevision()) {
+      $entity->original = $this->loadRevision($entity->getLoadedRevisionId());

This includes a hidden change -
1. load an entity in the default revision
2. flag the entity as a non-default revision
3. save the entity

Now the appended original will be not the newest default revision as it used to be, but the revision that we've worked with. This introduces a different behaviour if meanwhile the entity has been saved by another process with a new default revision.

I think this is still the right behaviour. I just wanted to point it out :).

I think it would be nice if we introduce a new method - loadOriginalEntity(EntityInteface $entity) and define on the interface what exactly is loaded in each scenario i.e.

1. saving a non-default revision - original will be the revision we're currently using
2. saving a default revision - original will be the newest default revision

@Sam152, sure, we don't have to delay the issue on a new method, which could be easy introduced in a follow-up.

Like already mentioned in the comments we should do the same also for the default revision, but without cache this isn't good.

We have two options:

1. Commit the patch as is and create a follow-up to adapt this to default revisions as well when the issue for the revision cache is ready.
2. On entity load before the post load store a clone of the entity as property $original or something else and during the saving process execute post load on that entity and use it as original. I am not sure if we need to run the original through the post load - any thoughts on this? Also in ContentEntityBase::_sleep() we would have to prevent from returning the original for serialization. I think this approach not only solves the problem for both default and non-default revisions, but even spares us one DB query for retrieving the original entity. What do you think?

P.S. I kind of have deja vu about the second option, so I am sorry if we already have discussed it somewhere else.

This bug leads to data loss, so it can have major and even critical priority. I suggest to accept a simple change (like the attached patch) which doesn't create compatibility issues and can solve the problem until some more general solution is found. Drupal 8.5 and 8.6 should also be fixed.

Bumping this again as I just lost quite a lot of time debugging a paragraph field migration I was writing due to some revisions not being populated with values.

I just ran up against this in a data import that maintains commerce order address data (as profile revisions). If an update occurs that would change a revision's field value to match the current default revision, that change is lost and the original value is preserved. The result is stale address data in my case. Definitely not expected behavior.

I was able to work around this bug by doing the following (this is a simplified example):

$entity = $this->storage->loadRevision($revision_id);
$entity->original = $this->storage->loadRevision($revision_id);
// Update the entity revision as necessary.
$entity->save();

The main point being I had to manually set $entity->original to the unchanged entity revision. Then I could make my changes and they'd stick.

This bug seems like it has huge potential for data loss, especially on commerce sites.

HI all!

I will have to agree with @chrisolof (thanks for the workaround by the way it seems to work so far) and the others.
This should be considered, imho as an architect/developer a major issue if it can be reproduced consistently (saying that because I have erratic behaviour with the suggested workaround depending on which field I try to update, very strange).

We have been trying to use revisions in our project, in good part to try and get the goodies it's supposed to provide.
But we have many business constraints and process (including unattended updates and / or unattended massaged copies) that require the ability to have proper control over revision data.

I stumbled upon this thread because a mix of decent Google fu and sheer luck, it saved my day. Otherwise I could have spent days trying to understand why I wouldn't get expected behaviour (we are still struggling in general to update however we want when using revisions, while working on nodes "only" has been a breeze ;)).
Especially since the concept of "original Revision" is dubious in our view so far (we get the general idea -probably- but no understanding of how really Drupal manages the revision lifecycle).
And I'm not skilled enough to go dig into the furthest of Drupal Core. :)

If it cannot be resolved easily, the minimum steps imo should be to update documentation and more importantly annotations of methods involved so less experienced Drupal developers like me can understand how to properly use the revisions system.
On that note, is it something I could try as a git commit with pull request (adding a comment, not solving underlying the problem, currently far above my skill ^^) ? Sorry for the newbish question, contributing to the Drupal community is kinda new to me and the "Getting Involved" was not totally clear to me from where to start.

Regards,
Laurent

EDIT : Ok, after pushing some tests I'm not sure my comment is pertinent. After trying different contexts I managed in all tested cases to directly set and save values in a specified revision, with and without ->original alike.
So actually my problem was probably irrelevant with the one exposed, which may or not be still a thing.
Sorry for that. Feel free to remove the whole comment if desired as I sadly don't bring anything to the discussion. ^^

Patch #26 still applies cleanly to 9.1.x-dev and it seems like there is at least some agreement to commit this as is and follow it up with using the dedicated method.

Hi all,

I am updating multiple users with cron but I am getting the following exception -
Drupal\Core\Entity\EntityStorageException: Update existing 'user' entity while changing the ID is not supported. in Drupal\Core\Entity\ContentEntityStorageBase->doPreSave() (line 706 of /var/www/html/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

My Drupal version is 8.8.1.

Can anyone tell me is this patch suitable for this error ? or suggest me any solution.

Thanks in advance.

Hi all

I came accross this issue when working with the workspaces module. The above patch, though an improvement, did not fix the issue I had when publishing nodes using workspaces.

My implementation is a bit different because I chose an implementation where I was able to force save the fields to the table by using a property and setter / getter method on ContentEntityBase. It also contains a fix for the WorkspacePublisher so felt it was out of scope for this issue.

The 2 issues deal with the same place of code (storing in dedicated tables).

https://www.drupal.org/project/drupal/issues/3152820#comment-13705672

Any feedback would be most appreciated.

I encountered this issue when attempting to programatically update a field on the published version and on the latest draft.

If I update the published version first, then I'm not able to update the latest draft.

In this use case, the workaround was to save the latest draft first.

However this is far from intuitive, and took some debugging (and reading of issues here) to understand the issue and find that workaround.

Faced this issue in our site running on D9.3.9. Workaround from #29 helped to solve it. Thanks!

Queued for 10.1.x, the related using a bit different approach but has no test
Probably it could be closed as duplicate but then it needs to extend test case for workspaces

Reroll the patch #26 with Drupal 10.1.x

FYI, I just faced the same problem using Content Moderation and trying to update the latest draft revision programmatically, but without creating a new revision, so using setSyncing(TRUE). When no additional revision is created, we face the same problem that if the value we try to set is identical to the one stored in the default revision (last published one), then the value is not updated.
Setting $entity->original as suggested in #29 does prevent the issue.

Given this can lead to data loss I think it's time we get this in.

I've opened a follow up #3343590: Add loadOriginalEntity function to address #20

Bumping to major as per #26

I think #2833084: $entity->original doesn't adequately address intentions when saving a revision already covers #20 so closed the new issue as duplicate of that.

Added credit from #3152820: Workspaces // Revision id in dedicated table not updated on publishing.

Committed b886bd7 and pushed to 10.1.x. Thanks!

Added a change record: https://www.drupal.org/node/3343661

And there's the mentioned #2839195: Add a method to access the original property , my plan for that is to make the replacement for $entity->original behave like the internal workaround here (that also exists in 2-3 other places already), which would then also make that other issue a duplicate.

Published the CR