Store revision id in originalRevisionId property to use after revision id is updated

If you load an arbitrary revision, then save it as the default revision (for example a revert), then it's correct to load the current default revision in $entity->original.

$entity->original exists so it's possible to compare the before/after state - this needs to be between what was published vs. what is about to be published. For example something that reacts to a change in $entity->status.

Can we maybe add previous as a method so we have a way of documenting it? Doing that for origin would be good as well but probably not in this issue.

We tried to remove it (origin) and have a separate $original argument passed to hooks but that issue never got in, which is one reason (more like an bad excuse) why it is not documented at all right now.

Shouldn't they be on \Drupal\Core\Entity\RevisionableInterface?

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -266,6 +273,7 @@ public function setNewRevision($value = TRUE) {
   +      $this->originalRevisionId = $this->getRevisionId();
   

   I would rather set this property in the constructor instead when the method setNewRevision is called.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -284,6 +292,29 @@ public function setNewRevision($value = TRUE) {
   +    $revison_id = $this->originalRevisionId;
   +    if (!$revison_id) {
   +      $revison_id = $this->getRevisionId();
   

   This could be turned into a single line :
   $revision_id = $this->originalRevisionId ? : $this->getRevisionId();

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -284,6 +292,29 @@ public function setNewRevision($value = TRUE) {
   +  public function loadUnchangedRevision() {
   

   This currently does not have a default return value (null) if the original revision id is not present.

You also have to ensure that $originalRevisionId is updated with the new revision id in the postSave method.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -284,6 +292,29 @@ public function setNewRevision($value = TRUE) {
+      $storage->resetCache([$this->id()]);

As we are caching only the default revision, the cache should be reseted only if you want to load the unchanged currently default revision.

Oh and in createDuplicate you have to unset the $originalRevisionId as well :)

You should not alter the RevisionableInterface as this is an API change, instead you could provide a new interface and make ContentEntityBase implement this new interface and after this issue is committed you could open a new one to merge this new interface into the RevisionableInterface.

Oh sorry, I've forgot to mention that I have already this logic implemented in #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load().

You could take the most of it from there :).

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -305,6 +305,7 @@ protected function doPostSave(EntityInterface $entity, $update) {
        // The revision is stored, it should no longer be marked as new now.
        if ($this->entityType->isRevisionable()) {
   +      $entity->resetOriginalRevisionId();
          $entity->setNewRevision(FALSE);
   

   If we add a new interface, would it make sense to test for that here?

2. +++ b/core/lib/Drupal/Core/Entity/OriginalRevisionIdInterface.php
   @@ -0,0 +1,30 @@
   +   * After calling ::setNewRevision, the revision identifier will be unset to
   +   * enforce the creation of a new revision identifier on save and
   +   * ::getRevisionId will not be able to return the revision identifier, for
   +   * which the entity has been loaded, but ::getOriginalRevisionId will.
   

   wondering if that's too much internal implementation details? Might be enough to just state that it will return the original revision ID the entity was loaded with even if it was changed (in any way).

3. +++ b/core/lib/Drupal/Core/Entity/OriginalRevisionIdInterface.php
   @@ -0,0 +1,30 @@
   +   * @return
   +   *   The original revision identifier of the entity, or NULL if the entity
   +   *   does not have a revision identifier.
   +   */
   +  public function getOriginalRevisionId();
   

   missing return type.

4. +++ b/core/lib/Drupal/Core/Entity/OriginalRevisionIdInterface.php
   @@ -0,0 +1,30 @@
   +   * After the entity has been saved, the storage should call this function to
   +   * set the original revision identifier to the new one, the entity received
   +   * after it has been saved.
   

   should => must, I think?

   Which is another nice example that shows how insanely hard BC is. So we have a new optional interface, everything is backwards compatible, yay! Except... if an existing storage won't call this method, it wont' work anymore?

   We need a change record for this anyway I think, that should then specfically highlight that the storage must call this and when in the process exactly.

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,77 @@
   +
   +  public function testPreviousProperty() {
   

   missing docblock.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,77 @@
   +    // entity, not the same as the loaded entity (which won't have a revision id
   +    // yet).
   

   ... which *does not* have a revision a revision ID yet, I think?

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,77 @@
   +    $this->assertTrue($loaded->getRevisionId() === NULL);
   

   assertIdentical(NULL, $loaded->getRevisionId()) might work too, not sure what's better?

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,77 @@
   +    // In hook_update the original revision id was stored in state. This should
   

   this should reference the actual function name, with (), there is no hook_update() ;)

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,77 @@
   +    // After saving the original revision id set in hook_update, and returned
   +    // from the entity should be the same and the entity's revision id.
   

   same re hook_update

   I think this comment could be clarified a bit. The comment just repeates the code but doesn't tell me why. The why is that the save above, unlike the first, updates the existing revision.

Also, I think I just found that this issue is a blocker for #2640496: Revision ID in {node} and {node_revision} can get out of sync, which is critical. The check/logic there doesn't work when updating a non-default revision as we're comparing with the default revision. Will explain more there, but this likely means that this issue is critical too.

@catch confirmed in IRC that this makes this issue critical. We should be close.

1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -28,7 +28,11 @@ protected function setUp() {
   +   * Tests the original revision id is set and returned at the right point in
   +   * the entity process.
   +   */
   +  public function testOriginalRevisionId() {
   

   not sure about rules for first line being a single line in docblocks for tests.. I usually try to word it so it is <80 characters but not sure about what the standards say exactly.

   But I'd say better change it than having it set back to NW because of that.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -37,17 +41,17 @@ public function testPreviousProperty() {
   +    // In entity_test_update the original revision id was stored in state. This
   
   @@ -68,8 +72,9 @@ public function testPreviousProperty() {
   +    // After saving, the original revision id set in entity_test and returned
   

   the second reference is just the module. And also still missing (), which I think they should have so it is linked and clickable.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -163,6 +163,13 @@
   +   * The original revision id before the new revision was set.
   

   id -> ID.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -230,6 +237,9 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   +    // Store the original revision identfier the entity has been loaded with to
   

   identifier ;)

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -230,6 +237,9 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
   +    $this->originalRevisionId = $this->getRevisionId();
   

   We should wrap this in a isRevisionable() check.

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -305,6 +305,9 @@ protected function doPostSave(EntityInterface $entity, $update) {
   +      if ($entity instanceof OriginalRevisionIdInterface) {
   +        $entity->resetOriginalRevisionId();
   +      }
   

   Why can't we do this in ContentEntityBase::postSave()? It would also make the new method unnecessary I think..

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -263,6 +275,8 @@ public function setNewRevision($value = TRUE) {
 
+    $this->originalRevisionId = $this->getRevisionId();
+

I'm not sure about this.

->setNewRevision(TRUE)
->setNewRevision(FALSE)

And then the revision ID is gone :)

maybe only do that assignment if we actually have a revision to assign?

Also, it at least needs a comment IMHO

Unfortunately I did not had the time to take a look at the changes made since the patch was cut off from #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load(), but I hope that they are compatible. I will try to review it until tomorrow.

I'm not entirely sure what I prefer, the code now in setNewRevision() or the explicit method from before. This means we don't have to worry about other storages not calling the new methods, that's certainly a plus.

I think is ready, has had lots of reviews and nitpicks. Lets unblock the issues that are blocked on this :)

I somehow do not like relying on ContentEntityStorage::doPostSave calling setNewRevision(FALSE) to update the original revision id. This is something I would do explicitly during the post save.

ContentEntityBase::createDuplicate has been forgotten - there the original revision id has to be unset and we need a test for this.

And yes I would say we have to update the original revision id explicitly in postSave as from now on we are working with an updated entity object.

I still think we should update the original revision id explicitly in postSave instead as a part of setNewRevision, actually when we set the original revision id in the constructor and then in the postSave there is no need of keeping this logic in setNewRevision at all.

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -414,15 +414,6 @@ function entity_test_entity_test_insert($entity) {
- * Implements hook_entity_update().
- */
-function entity_test_entity_update(EntityInterface $entity) {
-  if ($entity instanceof ContentEntityInterface) {
-    \Drupal::state()->set('entity_test.originalRevisionId', $entity->getOriginalRevisionId());
-  }
-}

->original is available in hook_entity_update(), I'm not convinced that not making this available is a good idea.

I can absolutely see use cases for this, for example when you need the original *and* the new revision ID, then this would be the only place to get that.

If we do, we should at least convert it to use presave instaed of update and not just delete that test coverage?

Sorry for doing this again but we have to break the reference of the variable when cloning the entity. See #2772979: Enforcing a cloned entity translation to be new propagates to the original entity and #2828073: Cloning an entity with initialized translations leaves $entityKeys and $translatableEntityKeys pointing to the old entity object as an example. A test is needed for this as well.

And we also have to make a reference in ContentEntityBase::initializeTranslation.

@effulgentsia, @catch, @cilefen, @alexpott, and I discussed this issue today and agreed that it was critical as a blocker for #2640496: Revision ID in {node} and {node_revision} can get out of sync, which is already triaged as critical. (If it were not a hard blocker, this issue would just be major, but we decided to follow @Berdir's recommendation in #2640496-31: Revision ID in {node} and {node_revision} can get out of sync.

That said, it would be good to put the information for how this is a hard blocker in the summary of this issue.

Thanks all!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -818,6 +818,7 @@ protected function initializeTranslation($langcode) {
     $translation->translationInitialize = FALSE;
     $translation->typedData = NULL;
+    $translation->originalRevisionId = NULL;
 

I'm not sure about this. the revision ID is not translatable, why should we unset it here?

Imagine this:

$node = Node::load(1);
$translation = $node->getTranslation('de');
$transation->save();

That *needs* to have access to the original revision ID as well, just as if you'd call $node->save();

The translation objects have to share all the $originalRevisionId property. In __clone we have to break the reference and make a copy of the value. In initializeTranslation we have to create the reference back again. That are different use cases and both need rest coverage.

Can we also verify that if you call $french->save() , the getOriginalId() is then the correct new ID for both $loaded and $french? That might not actually work yet and will need an explicit reference assignment like we do there for other properties.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1037,6 +1065,8 @@ public function __clone() {
+
+      $this->originalRevisionId = NULL;

Hmm now the clone will not have the $originalRevisionId set?

I think that #79 is what we need.

$entity->originalRevisionId == (clone $entity)->originalRevisionId will now evaluate to FALSE, what you have in your test as well. But this is not correct.

Thank you, this is what I was referring to.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1037,6 +1066,11 @@ public function __clone() {
+      $original_revision_id = $this->getOriginalRevisionId();

Here I would get the property directly by $this->originalRevisionId instead of using the getter function for it.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
@@ -0,0 +1,118 @@
+    $french->setNewRevision();
...
+    $this->assertNotEquals($french->getRevisionId(), $french->getOriginalRevisionId());
+    $this->assertGreaterThan($french->getRevisionId(), $french->getOriginalRevisionId());

After setting a new revision it would be nice to check not only that the revision id of the french translation has been reset but also of the default one. I would also backup first the revision id and after setNewRevision check that getOriginalRevisionId is returning the correct value.

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
@@ -0,0 +1,118 @@
+    $french->setNewRevision();
...
+    $french->save();
+    // Saving the new revision will reset the origin revision ID again.
+    $this->assertEquals($french->getRevisionId(), $french->getOriginalRevisionId());

After saving the entity with a new revision it would be nice to check also that the original revision id of the default translation has been updated.

I think this is ready now.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
   @@ -53,4 +53,20 @@ public function setRevisionTranslationAffected($affected);
   +  public function resetOriginalRevisionId();
   

   Why not setOriginalRevisionId() or because it's not a setter as such populateOriginalRevisionId()? Reset suggests it's been set already and is being reset.

Otherwise just nits:

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,124 @@
   +    // entity, not the same as the loaded entity (which do not have a revision
   

   which does

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,124 @@
   +    // id yet).
   

   ID

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,124 @@
   +    // In entity_test_entity_update() the original revision id was stored in
   

   ID again.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityOriginalRevisionTest.php
   @@ -0,0 +1,124 @@
   +    // and returned from the entity should be the same and the entity's revision
   

   as the?

Why not setOriginalRevisionId() or because it's not a setter as such populateOriginalRevisionId()? Reset suggests it's been set already and is being reset.
Otherwise just nits:

But the original revision id has been previously set, here we just update it to the new value. "Populate" however means it is not set and we are setting it for the first time.

We have the method EntityInterface::setOriginalId which is used exactly the same way in the EntityStorageBase::doPostSave as we have to update the original revision id in ContentEntityStorageBase::doPostSave so probably we could offer a similar method ContentEntityInterface::setOriginalRevisionId?

setOriginalRevisionId() seems fine yeah.

Hmm since it is a setter now it has to have a parameter for which to set the original revision id, right?

Then probably it would be better if the name of the method is updateOriginalRevisionId instead of setOriginalRevisionId, which implicates that there has to be a parameter as this is a setter?

update is OK with me, I misunderstood hchonov's comment in #92 to think they were /suggesting/ set*.

However this brings up another question. While we can't make this protected because the storage handler needs it, we really, really don't want people to call this method - should we add a note that it should be left alone? Otherwise looks RTBC for me.

Hmm what just catch's mentioned makes me thinking that the current implementation of updateOriginalRevisionId is dangerous -

$entity->setNewRevision();
$entity->updateOriginalRevisionId();
=> the original revision id is lost.

A comment is not something that might help not making such a mistake and having custom and contrib all together we do not know what could happen at some place. So I would say that if we go with updateOriginalRevisionId it should be retrieving the original revision id in a way not loosing it e.g. from $entity->values and ensure it is set or we go with the more stable solution setOriginalRevisionId which has a required parameter and if it is not set throw an exception. However I would like seeing the code example as a test not breaking the original revision id. This is something that we can solve.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -284,6 +296,21 @@ public function setNewRevision($value = TRUE) {
+  public function updateOriginalRevisionId() {
+    $this->originalRevisionId = $this->getRevisionId();
+    return $this;

If we go with this function then I would the assignment if ::getRevisionId returns something otherwise not

so this probably should be:

$this->originalRevisionId = $this->getRevisionId() ?: $this->originalRevisionId;

Can we add a test ensuring that updateOriginalRevisionId is not breaking the original revision id in this use case:
$entity->setNewRevision();
$entity->updateOriginalRevisionId();

This looks ready for me. Thanks.

Let's update the issue summary so that we can remove the tag and committers can't knock it back to NW.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -305,6 +305,7 @@ protected function doPostSave(EntityInterface $entity, $update) {
+      $entity->updateOriginalRevisionId();

So I think this is unavoidable and it akin to the call to setOriginalId() in the parent. I've done quite a bit of thinking about whether we should put this functionality in setNewRevision when it is passed with a FALSE value but I don't think that that makes sense.

Given that this analogous to the setOriginalId() I'm concerned by file_copy(), file_save_data() and file_save_upload() since they call setOriginalId() and do odd things but #2241865: Do not create a new file entity in order to overwrite an existing entity should solve the issue is files become revisionable - ponder about contrib though - ie. file_entity.

The discussion over the correct-ness of $entity->original containing the current default revision is interesting. I think the HEAD behaviour is completely unexpected. But as discussed this is an API change. One of the original directions of this issue was to add a ->previous. However maintaining both "original" and "previous" would be expensive. But in some ways adding a proper previous entity feels more correct than the current patch. Unfortunately the issue comments don't explain why this approach was dropped. That said at least having the original revision ID allows us to load the previous revision if required so this is definitely an improvement.

For the moment just writing some thoughts as they might spur other and better ideas. Leaving at rtbc because I have no definite plan of action.

We discussed this a bit in irc in relation to #2833049: ContentEntityBase::hasTranslationChanges will compare a forward revision with the default one instead of the newest forward revision. The big issue is that the revision we should be comparing against depends on context.

1. If you load the default revision, and save it as a new default revision, you should compare the old default and the new default.

2. If you load a non-default revision and save it as a new default revision (reverting, or publishing a draft), you should compare the old default and the new default.

In both cases $entity->original as it is now works for this case.

When saving a non-default revision it gets more complex.

1. No existing forward revision exists, saving a new non-default revision based on the current default. In this case you should compare against the default (which is simultaneously the most recent and the one you loaded)

2. Load and edit an existing forward revision, then save it as a new forward revision with a new workflow state. In this case you should compare against the loaded revision (which is simultaneously the most recent, but not the default).

3. You're reverting a forward revision back to a previous one - i.e. loading revision 1, saving revision 3. In this case the comparison ought to be against revision 2 (which is neither the revision that was loaded nor the default, but it is the most recent)

What this points to is that any one time, there are up to three revisions with different relationships to the one we're about to save:

1. The current default revision
2. The 'source' revision - i.e. the version we loaded as the basis of the one we're saving
3. The revision we're 'replacing' - this could be the default revision but it could also be the revision under consideration in a workflow.

(1,2) (2,3) (1,3), (1,2,3) can always point to the same revision (which we assume in the case with $entity->original)), but they can also all three be different.

Moving back to CNR not because this invalidates the patch, but it'd be good to confirm we all agree on the above since this doesn't solve all of the problems with $entity->original as it is.

Also because berdir mentioned in irc he's run into problems saving a within hook_entity_insert()/update().

Moving back to CNR not because this invalidates the patch, but it'd be good to confirm we all agree on the above since this doesn't solve all of the problems with $entity->original as it is.

This issue here is not intending of solving all the problems with $entity->original, for this @catch already created an another issue but this one would be needed there - #2833084: $entity->original doesn't adequately address intentions when saving a revision.

Also this one is needed for #2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load().

Also because berdir mentioned in irc he's run into problems saving a within hook_entity_insert()/update().

@berdir what kind of problems have you run into with this patch? Would you please provide some details in order for us to cover the use case?

Right. I'm using this API in #2640496: Revision ID in {node} and {node_revision} can get out of sync. What I'm doing it is prevent that you can re-save an existing revision but change the revision ID. This can for example happen when you run migrate updates that keep the revision ID while also manually updating/working on the target site.

So what I'm doing is, if no new revision is to be created, compare the current revision ID with getOriginalRevisionId(). And there are a few fails. One interesting problem is \Drupal\node\Tests\NodeSaveTest::testNodeSaveOnInsert().

What we're doing there is $node->save() in hook_node_insert(). That is sometimes required and a valid use case. But here is what is happening now:

1. Create a new node and save.
2. "new" revision, no existing revision ID, we're fine
3. hook_node_insert() is called
4. we're saving, no new revision, we now have a revision ID but getOriginalRevisionId() returns nothing, as we only reset it *after* the hooks. That means I now have a mismatch and I'm throwing an exception.

I think I can work around in that issue but maybe that's something that we should handle here. What should the expected behavior in that case be, though? Both during the second save but also in other insert hooks that are called *after* the one that re-saves. Note how this means that hook_node_update() can be called *before* hook_node_insert() for a module.

#2640496: Revision ID in {node} and {node_revision} can get out of sync now has a workaround, see latest interdiff, but we still might want to do something about that here, for example update the original revision ID before calling the insert hook for new entities. Not sure.

I really dislike recursive saving, but yes people do it. I guess what we do in that case depends on whether we consider the recursive save to be conceptually part of the same save, or a separate one? And that again might depend on context.

I'm wondering if we should call the method getLoadedRevisionId() so it's explicit that it's the revision ID that was loaded. This might make it easier when we try to name/document all the scenarios in the other issue.

I think we chose getOriginalRevisionId() for consistency with getOriginalId() but getLoadedRevisionId() would also work for me.

I am fine with the method name getLoadedRevisionId as well.

but we still might want to do something about that here, for example update the original revision ID before calling the insert hook for new entities.

I would expect insert hooks to not look for $entity->original, so this seems potentially harmless. Let's try that here and see what it looks like. Marking CNW for that and the method rename.

What I suggested is something like this. Not 100% sure we need that complexity but I think it makes sense.,

Didn't test, so something might fail.

I do not like that we make difference between saving a new entity and updating an existing entity, so I think we should make it consistent and decide whether we want that the original revision id is updated to the new revision id before or after the post save for both new and existing entities. However I think the post save hooks might still need an access to the previous revision id, so regarding them we have to update the original revision id after, what we actually still do for updates. New entities do not have an original revision id, so I guess we do not really need this logic and returning NULL is fine and returning something might even lead to misinterpreting the result. Take a look at the original ID, it is updated at the end after the post save, so if we do update the original revision id before the post save we would introduce an inconsistency between at which point we do update the original id and the original revision id. So if we do not have the original id during the post save hooks it does not make sense for me to have the original revision id either.

That are just my thoughts... If you are still not convinced then I guess that it would be great to hear what @catch thinks about this as well.

The concept of original ID doesn't exist for content entities anyway, they don't support changing the ID and #2640496: Revision ID in {node} and {node_revision} can get out of sync will enforce that. We generalized that concept at some point, but it really only exists for config entities or at least not for content entities. And original revision ID on the other hand doesn't exist for config entities. So I think we don't have to make them consistent. Both ID's for content entities are usually auto-increment or something like that, while config entities have them set explicitly. They are very different anyway.

See #119 for why setting it for the insert hooks initially might make sense. If you call $entity->save() in hook_entity_insert() then you are now updating an entity without having a loaded revision ID. That could be unexpected for code that expects one in a hook_entity_update(). We could also set it when someone actually calls save() not sure. Keep in mind that such save calls can also lead to cases where other insert hooks are called later, and possibly even after corresponding update hooks for the same entity were called. (This is also just one of in total 4 possible situations.. you could also explicitly save as a new revision in hook_entity_insert() and you could save with or without a new revision in hook_entity_update(). All of those can cause a lot of confusion in other hooks.

But we do support this and have explicit test coverage at least for saving the same revision in hook_entity_insert() so we should try to make it as reliable/predictable as possible. I'm not 100% sure but I think my suggestion is the most predictable (update hooks always have a loaded revision id, insert hooks always have it updated already, does not matter if they are before or after some other hook that resaves)

By saying consistent I've meant consistent when the original revision id is updated for both new and existing entities. About #119 - if getOriginalRevisionId returns NULL then you know there was no previous revision - and this is the correct way and to be expected in hook_entity_insert. I think and here you could use the already set revision ID through ::getRevisionId.

Re-saving an entity in hook_entity_insert is bad practice, but sure possible, and if you want to do this then I think you have to manually update the original revision ID.

Just thinking about this there are two hooks called after an entity is saved - hook_entity_insert or hook_entity_update, so if we do have the new entity revision ID set as original revision ID in one of those hooks this makes it inconsistent for me and confusing as well.

We could also set it when someone actually calls save() not sure

I would prefer this over updating it inconsistently in the storage based on if it is a new or an existing entity.

We would also need tests for this new use case.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -384,6 +384,17 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
+  public function save() {
+    if ($this->getEntityType()->isRevisionable()) {
+      // Make sure the loaded revision id is set before saving.
+      $this->updateLoadedRevisionId();
+    }
+    return parent::save();
+  }

never override save(), this is just a shortcut for $storage->save($entity). It needs to be \Drupal\Core\Entity\ContentEntityStorageBase::doPreSave().

Also, we should only do this if this is a) an update (entity is not new) and there is currently no loaded revision ID. And we should explicitly document that this is for cases like a save within hook_entity_insert() when we immediately save before having the "loaded" revision id assigned.