When there is an existing moderated, published node with a forward revision, and you disable workbench moderation for that node type, then the latest revision is still shown in the node edit form (good) but when you save changes, the default revision of the content is not updated (bad). This prevents you from updated the published revision of that content, unless you use the "revert" functionality on the "revisions" tab.

To duplicate on a fresh install:

  1. Enable moderation for the "article" content type
  2. Create and publish an article
  3. Edit the article and save the changes as a draft, creating a forward revision
  4. Disable moderation for the "article" content type
  5. Visit the article page
    • The default revision has not changed (good)
    • The edit tab is labeled "Edit draft"
    • The "Latest revision" tab is visible
  6. Visit the "Latest revision" tab - you get a 403 access denied
  7. Visit the edit tab
  8. The latest revision is displayed in the edit form
  9. Edit, then click "Save and publish"
  10. You are taken to the view page
    • The content is not updated (default revision is the same) <-- this is the only part that should change
    • A new revision has been created (seen in the revisions tab)
  11. Go back to the edit form... and your latest changes are still there
CommentFileSizeAuthor
#21 can_edit_but_not-2646244-21.patch2.53 KBbecw
#16 can_edit_but_not-2646244-16.patch7.02 KBbecw
#3 forward_revisions_after_disable-test-2646244-3.patch4.49 KBbecw
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

becw created an issue. See original summary.

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented

This is a followup to #2645608: Can't edit existing content after enabling Workbench Moderation; we're fixing these things kind of piecemeal, so maybe when writing the tests for this we can make them broader and catch more of this? For example, what happens to content if the state it is in gets deleted or is no longer available on the content type?

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented
FileSize
forward_revisions_after_disable-test-2646244-3.patch4.49 KB

Here is a messy test to that shows this issue.

  • becw committed c6de3b9 on disabling-moderation-2646244 
    refs #2646244: messy test, fails intentionally because of the ref issue.
Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Issue tags: +Release blocker
Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: forward_revisions_after_disable-test-2646244-3.patch, failed testing.

The last submitted patch, 3: forward_revisions_after_disable-test-2646244-3.patch, failed testing.

The last submitted patch, 3: forward_revisions_after_disable-test-2646244-3.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Assigned: Unassigned » becw
becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented

This happens because of the way core handles default revisions. When an entity is loaded by SqlContentEntityStorage, isDefaultRevision gets set depending on whether the revision ID matches the revision ID in the base table. This value is used when saving the next revision--nowhere in the normal entity save process does core touch this value.

In core, this means that when you create a new revision of a content entity, whether or not the new revision becomes the default revision is always determined by whether the previous revision was the default revision.

Core DOES explicitly set the default revision when you create a new revision using the "revert" button in the revision list.

So, I'm not sure if this is something we should even change. I suspect that when Workbench Moderation is disabled, the default revision will be displayed in the edit form--meaning that if you have forward revisions, they will be available in the revisions list but otherwise inaccessible. If anything, maybe disabling moderation on a content type should make the content type behave the same as if moderation were uninstalled.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented

Well, per #2627012: Delete moderation_state fields when uninstalling workbench_moderation on uninstall we should delete the field content. I do not think we should delete field content just for disabling the moderation checkbox, though. That seems like a great way to lose data. :-)

If you're suggesting that we just don't touch that bundle at all, that makes sense. However, in that case we should probably leave some kind of language on the configuration form that disabling moderation may leave any existing forward revisions orphaned, but they're still accessible from the revisions tab. (Or something.)

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented

If you're suggesting that we just don't touch that bundle at all, that makes sense. However, in that case we should probably leave some kind of language on the configuration form that disabling moderation may leave any existing forward revisions orphaned, but they're still accessible from the revisions tab. (Or something.)

Yes, that.

  • becw committed 03a4048 on after-disabling-moderation-2646244 
    refs #2646244: don't override core revision loading behavior when...

  • becw committed 022fa33 on disabling-moderation-2646244 
    refs #2646244: don't override core revision loading behavior when...
  • becw committed 09b85ac on disabling-moderation-2646244
    refs #2646244: add a special message when moderation is disabled.
becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented
Status: Needs work » Needs review
FileSize
can_edit_but_not-2646244-16.patch7.02 KB

Here's a patch that disables our revision loading on the edit form when moderation isn't enabled for a content entity bundle. It includes the messy test from #3.

Status: Needs review » Needs work

The last submitted patch, 16: can_edit_but_not-2646244-16.patch, failed testing.

The last submitted patch, 16: can_edit_but_not-2646244-16.patch, failed testing.

The last submitted patch, 16: can_edit_but_not-2646244-16.patch, failed testing.

  • becw committed 15502d0 on disabling-moderation-2646244 
    refs #2646244: remove test that no longer describes this issue.
becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented
FileSize
can_edit_but_not-2646244-21.patch2.53 KB

Turns out, that test doesn't demonstrate the issue anyway: it edits and re-saves the latest revision, but the solution we're working toward with this issue is to work with the way core manages the forward revision/default revision with respect to the node edit form.

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/src/Form/EntityModerationForm.php
@@ -58,6 +58,20 @@ class EntityModerationForm extends EntityForm {
+    if ($bundle->getThirdPartySetting('workbench_moderation', 'enabled', FALSE)) {

I'm confused about that code. This if would appear when workbench moderation is enabled, don't we want to detect that its disabled?

becw’s picture

becw CreditAttribution: becw at Palantir.net for Acquia commented

@dawhehner: that code sets it up so that the extra message on the config form only appears if moderation is already enabled, and the user unchecks the "enable" box--since this behavior appears when moderation has been enabled and then gets disabled.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

@dawhehner: that code sets it up so that the extra message on the config form only appears if moderation is already enabled, and the user unchecks the "enable" box--since this behavior appears when moderation has been enabled and then gets disabled.

AAAh gotcha! Nevermind then.

Crell’s picture

Crell CreditAttribution: Crell at Palantir.net for Acquia commented
Status: Reviewed & tested by the community » Fixed

Merged. Thanks, Bec!

  • becw committed 022fa33 on 8.x-1.x 
    refs #2646244: don't override core revision loading behavior when...
  • becw committed 09b85ac on 8.x-1.x
    refs #2646244: add a special message when moderation is disabled.
  • becw committed 15502d0 on 8.x-1.x
    refs #2646244: remove test that no longer describes this issue.
  • becw committed c6de3b9 on 8.x-1.x
    refs #2646244: messy test, fails intentionally because of the ref issue.

Status: Fixed » Closed (fixed)

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