Problem/Motivation

After calling $entity->setNewRevision(TRUE) we have no way of knowing what revision id the entity used to be.

Considered a blocker for #2640496: Revision ID in {node} and {node_revision} can get out of sync which is critical.

Proposed resolution

Set the revision id to an originalRevisionId property in __construct
Update the originalRevisionId on post save
Return the property with getOriginalRevisionId() method

Remaining tasks

Review

User interface changes

none

API changes

Two methods added to ContentEntityBase and ContentEntityInterface

Data model changes

None

(Blocking #2809123: Reverting a revision doesn't keep moderation state)

Files: 

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Status: Active » Needs review
FileSize
1.44 KB

I think this resolves the issue, need to write a test to prove it.

Status: Needs review » Needs work

The last submitted patch, 2: 2809227-2.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
1.3 KB

Here's a test for this.

Status: Needs review » Needs work

The last submitted patch, 4: 2809227-3.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.36 KB
0 bytes

Fixed the broken test in #4.

catch’s picture

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.

timmillwood’s picture

I'm not sure I agree.

If you load an arbitrary revision, then save it as the default revision (for example a revert), the revision you loaded should be in $entity->original. This is a must when things are referencing specific revisions such as ContentModerationState in Content Moderation module.

In Content Moderation module if we have revisions 1,2,3, where 3 is currently the default revision, when reverting to 2 we need to know in the hook_entity_update to use the same moderation state for the newly created revision as we did for revision 2.

lomasr’s picture

timmillwood’s picture

After speaking to @catch I think this boils down to the backwards compatibility of this bug.

Yes, it may be a bug, but some people might depend on the way it currently works. So do we fix it, or keep it how it is then add $entity->previous or something to load the actual original revision.

timmillwood’s picture

FileSize
4.51 KB
4.55 KB

Here's a patch to use $entity->previous instead of $entity->original.

Status: Needs review » Needs work

The last submitted patch, 11: 2809227-11.patch, failed testing.

The last submitted patch, 11: 2809227-11.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.53 KB
638 bytes

Lets give this another shot.

Status: Needs review » Needs work

The last submitted patch, 14: 2809227-14.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
731 bytes

Only loading $entity->previous if is set.

Status: Needs review » Needs work

The last submitted patch, 16: 2809227-16.patch, failed testing.

timmillwood’s picture

FileSize
4.37 KB
2.3 KB

Broke my own test, that's not very productive.

Fixed!

timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

So the patches in #6 and #18 do pretty much the same thing. The first uses $entity->original and the second uses a new property $entity->previous

Please review the code, and the options between the two options.

Berdir’s picture

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.

timmillwood’s picture

Status: Needs review » Needs work

ok, I will get to work introducing getOriginalRevisionId() and loadUnchangedRevision() methods to ContentEntityBase.

amateescu’s picture

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

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
4.95 KB

Implemented getOriginalRevisionId() and loadUnchangedRevision(), added them to RevisionableInterface.

timmillwood’s picture

Title: Creating a new revision from an old revision loads the default revision as the original. » After setNewRevision() we don't know what the revision id was
Issue summary: View changes

Updating the summary and title to make more sense based on the direction of the latest patch.

timmillwood’s picture

FileSize
4.82 KB
2 KB
  • Changed the property from original_revision to originalRevisionId
  • Made the originalRevisionId property protected
  • Changed getOriginalRevisionId() to return the current revision id if no original one is set
  • Prevented loadUnchangedRevision() from returning anything if no revision id is set.
hchonov’s picture

Status: Needs review » Needs work
  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.

hchonov’s picture

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

hchonov’s picture

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

hchonov’s picture

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.

hchonov’s picture

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 :).

timmillwood’s picture

timmillwood’s picture

Status: Postponed » Needs work

Reviving this to implement getOriginalRevisionId().

timmillwood’s picture

Status: Needs review » Needs work

The last submitted patch, 34: 2809227-34.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
6.62 KB
775 bytes

oops

Status: Needs review » Needs work

The last submitted patch, 36: 2809227-36.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
5.97 KB
666 bytes

Fixing failing tests.

Status: Needs review » Needs work

The last submitted patch, 38: 2809227-38.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
1.08 KB

Re-rolling for 8.2.x

Berdir’s picture

Status: Needs review » Needs work
  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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
5.68 KB
1.94 KB

Made changes based on comments in #41.

timmillwood’s picture

timmillwood’s picture

FileSize
7.27 KB
2.73 KB

Updated the test to be a help explain how the original revision id should work.

Berdir’s picture

Status: Needs review » Needs work
  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.

Berdir’s picture

Priority: Major » Critical

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

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Needs work » Needs review
FileSize
7.47 KB
2.74 KB

Addressed items in #45.

Berdir’s picture

Status: Needs review » Needs work
  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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.46 KB
2.09 KB

Addressing items in #48

amateescu’s picture

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

timmillwood’s picture

FileSize
6.54 KB
3.47 KB

Addressed items in #50 which included a big change based on #50.4.

Moving to ContentEntityBase::postSave() didn't really work because it was fired too early, but moving to setNewRevision() works well because it is called at the end of doPostSave().

This means we can remove resetOriginalRevisionId(). Then the new interface for one method seems a bit odd, so moved getOriginalRevisionId() back into ContentEntityInterface.

Berdir’s picture

+++ 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

hchonov’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 51: 2809227-51.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.61 KB
4.22 KB

Fixing #51 and #52.

#53: I think it'll be ok.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

timmillwood’s picture

@Berdir - Yesterday @amateescu and I discussed in length about where to reset the original revision ID. Specifically to negate the use of resetOriginalRevisionId(). First I tried postSave() as @amateescu suggested in #50, however this fired too early, so when hook_update was called the original revision ID had already been reset. Putting it in setNewRevision works well because the original revision id is set right before it's nulled. Also setNewRevision() is called (with FALSE) in \Drupal\Core\Entity\ContentEntityStorageBase::doPostSave which means the original revision ID is reset as one of the last things in the process, this means the test is unchanged between #49 and #50.

The only concern here is if setNewRevision(FALSE) is ever removed from \Drupal\Core\Entity\ContentEntityStorageBase::doPostSave, or if other storage doesn't call it. However generally this will still produce the desired results because the original revision ID would still get set when setting a new revision.

hchonov’s picture

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.

timmillwood’s picture

I was wondering do we need to set the original revision ID in postSave() at all, because thst would set it to the new revision id, and therefore it would not be the original anymore.

hchonov’s picture

Status: Reviewed & tested by the community » Needs work

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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
8.05 KB
1.33 KB

Adding createDuplicate() update and test.

hchonov’s picture

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.

timmillwood’s picture

FileSize
6.76 KB
4.01 KB

After talking to @amateescu I've moved the "reset" of the original revision id property to the ContentEntityBase::postSave method. This is now reset too early for hook_update which was being used as part of the testing, so this section of the test was removed, but all other elements in the test are returning their original result.

Status: Needs review » Needs work

The last submitted patch, 63: 2809227-63.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
6.97 KB
1.24 KB

Need to remember to switch to 8.2.x before rolling this patch.

Berdir’s picture

+++ 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?

Status: Needs review » Needs work

The last submitted patch, 65: 2809227-65.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.36 KB
4.52 KB

Moved the test to use entity_test_entity_presave().

timmillwood’s picture

FileSize
7.64 KB
5.91 KB

Added resetOriginalRevisionId() back, and making use of it in all the places including doPostSave().

hchonov’s picture

Status: Needs review » Needs work

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 leaves $values, $entityKeys and $translatableEntityKeys pointing to the old object as an example. A test is needed for this as well.

hchonov’s picture

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

xjm’s picture

Issue summary: View changes
xjm’s picture

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

timmillwood’s picture

FileSize
8.64 KB
2.48 KB

Changed from #70 and #71.

timmillwood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 74: 2809227-74.patch, failed testing.

The last submitted patch, 74: 2809227-74.patch, failed testing.

Berdir’s picture

+++ 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();

hchonov’s picture

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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
1.4 KB

I think I agree with @Berdir.

Updated the test to enforce this. When creating a new translation, the original revision ID should be the same as it was before.

Berdir’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 80: 2809227-80.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
3.02 KB
9.48 KB
  • Added better language testing.
  • Removed revisonable check from __clone
hchonov’s picture

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

timmillwood’s picture

Not sure what you mean @hchonov.

hchonov’s picture

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

timmillwood’s picture

FileSize
10.07 KB
2.28 KB

Updating what __clone does. Hope this is what @hchonov was thinking.

hchonov’s picture

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.

timmillwood’s picture

FileSize
10.53 KB
2.23 KB

Updating based on #88.

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready now.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  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.

  2. Otherwise just nits:

  3. +++ 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

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

    ID

  5. +++ 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.

  6. +++ 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?

hchonov’s picture

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?

catch’s picture

setOriginalRevisionId() seems fine yeah.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.53 KB
2.81 KB

addressing #91.

Status: Needs review » Needs work

The last submitted patch, 94: 2809227-94.patch, failed testing.

timmillwood’s picture

This is why I shouldn't write patches while in a meeting.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.52 KB
1.22 KB
hchonov’s picture

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

timmillwood’s picture

I don't think we should allow the original revision id to be set to anything, it should only ever be allowed to be set to the current revision id.

hchonov’s picture

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?

timmillwood’s picture

yes, update could would. @catch, what do you think?

Status: Needs review » Needs work

The last submitted patch, 97: 2809227-97.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

FileSize
10.54 KB
2.11 KB

After talking to @catch on IRC switching setOriginalRevisionId() to updateOriginalRevisionId().

catch’s picture

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.

hchonov’s picture

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.

timmillwood’s picture

Feels like we're going around in circles but it'd be better if we needed need the updateOriginalRevisionId() method at all. The only way to do this would be to update the original revision id only in ContentEntityBase and not in the storage handler, but as we discovered there's no nice solution for this either.

timmillwood’s picture

FileSize
10.69 KB
640 bytes

Added a short comment to show what might happen if updateOriginalRevisionId() is used, and also made it internal.

Feel free to change the wording of the comment on commit.

hchonov’s picture

+++ 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;
timmillwood’s picture

FileSize
10.72 KB
563 bytes

I guess that kinda makes sense.

hchonov’s picture

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

timmillwood’s picture

FileSize
11.06 KB
1014 bytes

Adding a test to make sure calling updateOriginalRevisionId() doesn't screw things up.

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

This looks ready for me. Thanks.

jibran’s picture

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

timmillwood’s picture

Title: After setNewRevision() we don't know what the revision id was » Store revision id in originalRevisionId property to use after revision id is updated
Issue summary: View changes
Issue tags: -Needs issue summary update
alexpott’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs review

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().

hchonov’s picture

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?

Berdir’s picture

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.

Berdir’s picture

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

catch’s picture

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.

Berdir’s picture

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

hchonov’s picture

I am fine with the method name getLoadedRevisionId as well.

catch’s picture

Status: Needs review » Needs work

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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.92 KB
14.4 KB

Renaming from OriginalRevisionId to LoadedRevisionId.

Status: Needs review » Needs work

The last submitted patch, 125: 2809227-125.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.92 KB

Forgot to rename file when renaming class.

Berdir’s picture

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.

timmillwood’s picture

@Berdir - That works for me, and it looks as though it still passes the test included in the patch.

I would RTBC if it was moral to do so, maybe @hchonov or @catch could?

hchonov’s picture

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.

Berdir’s picture

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)

hchonov’s picture

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.

timmillwood’s picture

Here's an 8.2.x and 8.3.x version of the patch which updates the loaded revision id in save() as discusses with @hchonov and @Berdir on IRC.

The last submitted patch, 133: 2809227-133-for-8.3.x.patch, failed testing.

Berdir’s picture

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

timmillwood’s picture

Updating with suggestions from #135.

hchonov’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,137 @@
    +    $loaded->save();
    +
    +    // After saving, the loaded Revision id set in entity_test_entity_update()
    +    // and returned from the entity should be the same as the entity's revision
    +    // id because a new revision wasn't created, the existing revision was
    +    // updated.
    +    $loadedRevisionId2 = \Drupal::state()->get('entity_test.loadedRevisionId');
    

    Here we have to reset the variable set into the test - just to ensure the test is working correctly.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,137 @@
    +    $entity2 = EntityTestMulRev::create(['name' => 'EntityLoadedRevisionTest']);
    +    $entity2->save();
    +    $loadedRevisionId3 = \Drupal::state()->get('entity_test.loadedRevisionId');
    

    $entity2, but $loadedRevisionId3... probably you could isolate each test case so that you can just use $entity and $loadedRevisionId.

Those are my last remarks. After this I would RTBC it.

timmillwood’s picture

FileSize
4.14 KB
13.16 KB

In this version of the patch I have split up the test into 4 tests:
- General test
- Save without creating a new revision, plus clone and duplicate
- Multilingual entity
- Hook_entity_insert

hchonov’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,167 @@
    +
    

    Not needed empty line.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,167 @@
    +  public function testLoadedRevisionIdWithNoNewRevision() {
    

    Missing doc of the test method.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,167 @@
    +   * Tests the loaded revision ID works on a multilingual site.
    

    I would say "Tests the loaded revision ID for translatable entities".

  4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,167 @@
    +   * Test calling save() in entity_test_entity_insert() sets the correct ID.
    

    I would say "Tests re-saving the entity in hook_entity_insert."

  5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityLoadedRevisionTest.php
    @@ -0,0 +1,167 @@
    +  }
    +}
    

    We need an empty line between the end of the last method and the closing bracket of the class.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.72 KB
13.24 KB

Fixing the nits.

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Thank you.

The last submitted patch, 138: 2809227-38.patch, failed testing.

  • catch committed 9d8d1ed on 8.3.x
    Issue #2809227 by timmillwood, Berdir, hchonov, catch, amateescu,...
catch’s picture

Committed/pushed to 8.3.x, thanks!

Not really sure what to do here about 8.2.x given both the criticality of the issue and the interface change, so leaving RTBC against there a bit longer. Possibly we should continue with the other bugfixes then cherry-pick at once?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 140: 2809227-140.patch, failed testing.

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

Putting back to RTBC

e0ipso’s picture

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

Adding this two methods to the interface may be breaking BC.

This patch may be assuming that the ContentEntityInterface can only be implemented via the ContentEntityBase.

I could fix the Simple OAuth module because I got an early report, but others may not be as lucky.

#2839585: Bearer Token Authentication broken on 8.3.x HEAD

Berdir’s picture

See https://www.drupal.org/core/d8-bc-policy, this part:

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal
Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

So that's OK for 8.3.x. but I guess it means that this can't be added to 8.2.x. The argument against would be that it is necessary for a critical issue to be fixed.

catch’s picture

Status: Reviewed & tested by the community » Postponed

Yes exactly, we don't want people getting fatal errors on 8.2.x sites so this usually wouldn't go in. However data integrity issues are as bad/worse than fatal errors, so not ruling out an exception here. Either way we should postpone this on #2640496: Revision ID in {node} and {node_revision} can get out of sync being fixed in 8.3.x and revisit it then.

Berdir’s picture

Status: Postponed » Needs review

That went in, lets see if this still apples against 8.2.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Still passing, so setting to RTBC so that someone can make that decision :)

Personally, I'm not sure. Based on #147, we'll break at least one module by adding those new methods, but would be broken anyway in 8.3.x. But people might not be aware that updating core might break their existing site, e.g. in case of a security update that isn't properly tested before deploying.

And while I agree with alex pott in #2640496: Revision ID in {node} and {node_revision} can get out of sync that an exception is better than corrupt data that also leads to exceptions later, this situation is different. We might break a bunch of sites that have no data integrity problems at all and never will.

I guess that means I'm leaning towards -1 and not +1 on committing this or not. Anyone thinks it is very important to get this into 8.2? Then speak up now :)

alexpott’s picture

If this is going introduce breaks then I don't think we should commit this to 8.2.x

catch’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Yes I agree with Berdir's leaning towards -1 here. Also we haven't yet tracked down the code path that leads to this exception being thrown in the first place (although there are suspicions it might be #2748609: Preserving auto-increment IDs on migration is fragile. Given that I think we should leave this issue at 8.3.x and fixed.

  • catch committed 9d8d1ed on 8.4.x
    Issue #2809227 by timmillwood, Berdir, hchonov, catch, amateescu,...

  • catch committed 9d8d1ed on 8.4.x
    Issue #2809227 by timmillwood, Berdir, hchonov, catch, amateescu,...
timmillwood’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.