Problem/Motivation

When trying to save a new revision after restoring the original revision ID, an integrity constraint violation exception is thrown, as the SQL storage tries to create a new revision record with an existing revision ID.

Proposed resolution

Revert the "new revision" state of an entity object, if the original revision ID is restored.

Remaining tasks

  • Validate the proposed solution
  • Write a patch
  • Reviews

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#2 entity-new_revision_revert-2928778-2.test.patch2.52 KBplach
#2 entity-new_revision_revert-2928778-2.patch3.34 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach created an issue. See original summary.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Status: Active » Needs review
FileSize
entity-new_revision_revert-2928778-2.patch3.34 KB
entity-new_revision_revert-2928778-2.test.patch2.52 KB

This was found while working on #2892132: Entity validation does not always prevent concurrent editing of pending revisions.

Status: Needs review » Needs work

The last submitted patch, 2: entity-new_revision_revert-2928778-2.test.patch, failed testing. View results

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Status: Needs work » Needs review
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented 
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -745,6 +745,12 @@ public function onChange($name) {
+          $this->newRevision = FALSE;

There is a lot of other stuff going on in setNewRevision(), should we be calling that rather than just updating the property?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented

tldr;

I think the current code is fine, as doing that will trigger an infinite loop, since also ::setNewRevision() will set the revision ID field value.

Long story:

This new bit of logic is complementary to ::setNewRevision(), and being able to execute it without triggering an infinite loop would buy us nothing. In fact we are just setting the flag to FALSE when the revision ID matches the loaded revision ID, which is the assignment ::setNewRevision() would perform, if called in that situation (thus triggering an invocation of ::onChange()).

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

ok, makes sense, just wanted to check!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented
Issue summary: View changes

  • catch committed 0ee2531 on 8.5.x 
    Issue #2928778 by plach: Exception when trying to save a new revision...

  • catch committed ce6aa26 on 8.4.x 
    Issue #2928778 by plach: Exception when trying to save a new revision...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

This seems to cause a regression in our install profile with paragraphs default content, which has a fixed revision_id and ERR references to it, which then fails.

What's happening is that the revision ID is set/changed on a new entity which does not yet have a revision_id and then the revision id is the same as the loaded (= empty) and newRevision is set to FALSE. And then this has some impact later, possibly due to other calls to setNewRevision(TRUE) that then again unset the revision_id because it thinks that previously, it did not save as a new revision, despite actually being a new entity.

This seems to fix it for me: https://gist.githubusercontent.com/Berdir/9468aff9027edbcdc7bfb66975f127...

Will open a new issue and include a test.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach for Acquia commented

Ouch, thanks for catching that, @Berdir!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

No worries, I didn't spot this either when I looked at the issue.

Here's the follow-up, with a test: #2934517: Setting a revision ID on a new entity sets the newRevision flag to false