I created a "Revisions" View which shows content_revisions. I created a column for "Operations" and for each revision that *is not* the default revision, the user can click "Delete Revision." They are able to do this for all past revisions (older than the default revision) and for newer *draft* revisions (higher VIDs than the default revision).

When a user clicks "Delete Revision" on a moderated revision in Draft state, they are taken to the delete revision confirmation page like normal. I have confirmed that the URL shows the correct NID and VID values to ensure we are indeed requesting deletion of a non-default revision.

However, upon confirming the operation, we instantly get a white page with an error. Looking at the log:

Drupal\Core\Entity\EntityStorageException: Default revision can not be deleted in Drupal\Core\Entity\ContentEntityStorageBase->deleteRevision() (line 356 of /srv/bindings/9c0c13e8d42e461382b31296b06f75c3/code/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

The funny thing is though, the revision is in fact deleted from the database, and when we return to our custom Revisions listing, the revision we wanted to delete has in fact been deleted (double checked in the database).

To reproduce:

  1. Create a new node in a content type that does NOT have moderation enabled, but DOES have revisioning
  2. Edit this node 1 or 2 times, saving a new revision each time.
  3. Enable moderation/workflow on this content type, with the default Draft/Published states.
  4. Edit the node again, but save your revision as "Published"
  5. Edit the node once more, and this time save it as a "Draft."
  6. You should now have a half dozen revisions - several with no moderation state (prior to moderation), one marked as "Published" which is the default revision, and the newest revision will be a "Draft" which should not be published nor the default revision.
  7. Type /node/X/revisions/Y/delete in the address bar, replacing "X" with the node ID, and "Y" with the revision id of the newest Draft revision.
  8. Click "Delete" on the confirmation page.
CommentFileSizeAuthor
#14 2933099-14.patch5.33 KBsam152
#8 2933099-8.patch6.27 KBsam152
#8 2933099-8-test-only.patch5.33 KBsam152
#5 patch_2933099.patch868 bytessathish.redcrackle

Comments

amaisano created an issue. See original summary.

amaisano’s picture

amaisano commented
Issue summary: View changes
amaisano’s picture

amaisano commented
Version: 8.4.0 » 8.4.3

Tested and occurs in the latest version as well.

amaisano’s picture

amaisano commented
Title: Default Revision cannot be deleted. But it's not the default revision... » "Default Revision cannot be deleted." But it's not the default revision...
sathish.redcrackle’s picture

sathish.redcrackle commented
StatusFileSize
new patch_2933099.patch868 bytes

Attached the patch file.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Interesting fix, but I think the real problem is that the default revision of content_moderation and the entity is not in sync. The result is that we then end up not deleting that revision and end up with bogus content_moderation revision?

Instead, we need to make sure that every time the entity is saved, we need to set the default revision flag on content_moderation accordingly.

sam152’s picture

sam152 commented
Version: 8.4.3 » 8.5.x-dev

I think it was originally a design decision that the defaultness of the composite entity was not kept in sync with the entity being moderated. It would just be another bit of metadata to juggle when trying to keep the entities in sync, with seemingly no real pay off. This seems like the first hindrance of this semantic to come up, nice catch!

To my knowledge there isn't anything specific which requires the current behavior to work properly so #6 makes sense as the fix to me.

sam152’s picture

sam152 commented
Status: Active » Needs review
StatusFileSize
new 2933099-8-test-only.patch5.33 KB
new 2933099-8.patch6.27 KB

Interested to see if anything depends on the default revision of the ContentModerationState entity always being the latest revision.

I believe to fix this properly, we will also need to provide an update hook to shuffle around the default status of these entities.

The last submitted patch, 8: 2933099-8-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 8: 2933099-8.patch, failed testing. View results

sam152’s picture

sam152 commented

So it looks like the views integration is impacted by this. We need to find a way to join to the latest revision of the content_moderation_state entity in the join from the base table of the moderated entity in \Drupal\content_moderation\ViewsData::getViewsData.

I've been having a look in \Drupal\views\Plugin\views\filter\LatestRevision for inspiration but can't get this to work so far.

Funnily enough, the broken test has a commented related to this exact issue:

// @todo I would have expected that the content_moderation_state default
//   revision is the same one as in the node, but it isn't.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

effulgentsia’s picture

effulgentsia commented

Is this solved now that #2941736: Moderation state revisions should have their isDefaultRevision() match the host entity's landed, or is there more to do here?

sam152’s picture

sam152 commented
Status: Needs work » Needs review
StatusFileSize
new 2933099-14.patch5.33 KB

This is indeed fixed now. Reuploading the test only patch to prove it. I suppose we could commit the test as well.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Workflow Initiative

More test coverage is always good, right?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Bug report » Task

Adding review credits - @amaisano for reporting the issue and documenting steps to reproduce and @Berdir for pointing towards the correct fix - that got fixed elsewhere.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

#2941736: Moderation state revisions should have their isDefaultRevision() match the host entity's doesn't appear to have added extra deletion tests so I think added the extra coverage is a good idea.

Committed and pushed 445b7d0515 to 8.6.x and ed7ab51d9e to 8.5.x. Thanks!

Backported to 8.5.x since this is a test-only change and the fix went into 8.5.x

  • alexpott committed 445b7d0 on 8.6.x 
    Issue #2933099 by Sam152, sathish.redcrackle, amaisano, Berdir: "Default...

  • alexpott committed ed7ab51 on 8.5.x 
    Issue #2933099 by Sam152, sathish.redcrackle, amaisano, Berdir: "Default...

Status: Fixed » Closed (fixed)

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