The "from state" used for calculating available transitions is changed when using the content moderation state widget and preview mode

I believe that the problem is the UI. You're supposed to use the "Back to content editing" link when in preview mode. This link is not obvious, especially with the moderation state form still in the middle of the page. IMO we should remove that form on the preview page. There's probably more to do to make it pretty, but I think that's a start.

https://www.evernote.com/l/ARYvYkwAeZBNT6lRj6PD6DfRzUyhPvCbZfUB/image.png

I agree, it doesn't make sense to have the moderation control from appear during a preview, but I don't think that is what the issue summary is describing. The problem can be reproduced even when you disable the "Moderation control" component on the entity display. Exact steps that worked for me were:

1. Enable "Editorial" on article or page.
2. Visit the "node/add/article".
3. Enter some content and select "Published" for the "Save as" field.
4. Click "Preview" and then "Back to content editing".
5. Open the "Save as" field, see "Archived" is now an option.
6. Select "Archived" and hit "Preview" / "Back to content editing" again.
7. Screen displays "An illegal choice has been detected. Please contact the site administrator"

I think this issue could focus on what happens when a state changes on an unsaved entity and a new one could be spun out to hide the moderation form during a preview.

Attached is a pretty hacky solution. If the entity is new, always use the default state. If the entity already exists, reload the revision that has already been saved and use that. Comment in the patch kind of describes the issue too:

// Ensure the entity used in the transition validation uses the moderation
// state derived from either the "saved" entity or the default calculated
// above. If the moderation state is set during the lifecycle of an entity
// form and the widget is rebuilt, new state options should not be available
// until the entity has been saved.

Beyond doing some juggling like this, I can't think of an easy way to address this.

Here is my attempt at resolving the issue. I also included a new test case in ModerationStateNodeTest.php

1. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,25 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      $saved_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->load($entity_id);
   

   This will load the default revision, if there are pending revisions won't this cause incorrect transitions from being loaded?

2. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php
   @@ -186,4 +186,43 @@ public function testNoContentModerationPermissions() {
   +  public function testModerationStateAfterPreview() {
   

   We should add a test here for pending revisions as they're full of edge cases.

Re #10.1, I think \Drupal\content_moderation\Entity\ContentModerationState::loadFromModeratedEntity might help with this. Haven't had a chance to review the latest changes of the test.

Ok so I ended taking a slightly different approach. I was having issues with some of the translation test passing but I finally think I figured it out. I also added test cases for pending revisions. Lastly, this patch includes the changes in https://www.drupal.org/project/drupal/issues/2940701 since the changes for both issues live in the same file in the same area. Fixing one kinda fixed the other. So if the test pass here, I will close out the other ticket as a duplicate.

@nick.james the patch is empty.

Lol woops. Thanks @timmillwood.

+++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
@@ -121,10 +121,26 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    // Retrieve the correct current state value.
+    if (!$entity->isNew()) {
+      $revision_id = $entity->getLoadedRevisionId();
+      $saved_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($revision_id);
+      if ($saved_entity->getEntityType()->isTranslatable() && $saved_entity->hasTranslation($entity->langcode->value)) {
+        $saved_entity = $saved_entity->getTranslation($entity->langcode->value);
+        $workflow = $this->moderationInformation->getWorkflowForEntity($saved_entity);
+      }
+      $moderation_state = $saved_entity->moderation_state->value;
+      $default = $moderation_state ? $workflow->getTypePlugin()->getState($moderation_state) : $workflow->getTypePlugin()->getInitialState($saved_entity);
+
+      /** @var \Drupal\workflows\Transition[] $transitions */
+      $transitions = $this->validator->getValidTransitions($saved_entity, $this->currentUser);
+    }
+    else {
+      /** @var \Drupal\workflows\Transition[] $transitions */
+      $transitions = $this->validator->getValidTransitions($entity, $this->currentUser);
+    }

Does workflow need to be set outside of this conditional?

Also, this entire new section is doing quite a bit. Can some code comments be added to clarify what's going on in here?

@jhedstrom,

1. Great point. Code comments have been added.

2.

Does workflow need to be set outside of this conditional?

If you are referring to $workflow = $this->moderationInformation->getWorkflowForEntity($entity);, yes that part is still needed because it is used to grab the initial state for the entity if it is just being created.

@jhedstrom, Sorry I think I misunderstood you. The $workflow being defined in the if statement may not even be needed. The workflow should be the same for the saved entity vs the provided entity. I removed line that redefines the $workflow variable and tried it manually and saw no issues. I am running the test suite locally to see if it does indeed break something.

Redefining of $workflow has been removed.

Looking good @nick.james, I think my only suggestion would be to test the preview and back to edit form when editing the pending revision. Just to test that the widget it loading the current state for the latest revision and not the default revision.

#4 @douggreen - #2942497: Using content moderation block in preview mode causes EntityStorageException

As discussed on #2942497: Using content moderation block in preview mode causes EntityStorageException, my suggestion would be to remove the moderation form from the preview page altogether.

To me, it doesn't belong in the preview - I think editors generally want to see what the page will look like to site visitors, rather than what it looks like for other editors.

I rerolled tha patch and created a pending revision in the tests.

+++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
@@ -121,10 +121,30 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    // If the entity is not new, grab the most recent revision and
+    // load it. The moderation state of the saved revision will be used
+    // to display the current state as well determine the the appropriate
+    // transitions.
+    if (!$entity->isNew()) {
+      $revision_id = $entity->getLoadedRevisionId();
+      $saved_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($revision_id);
+
+      // If the entity allows translations, grab the translated version.
+      if ($saved_entity->getEntityType()->isTranslatable() && $saved_entity->hasTranslation($entity->langcode->value)) {
+        $saved_entity = $saved_entity->getTranslation($entity->langcode->value);
+      }
+      $moderation_state = $saved_entity->moderation_state->value;
+      $default = $moderation_state ? $workflow->getTypePlugin()->getState($moderation_state) : $workflow->getTypePlugin()->getInitialState($saved_entity);

We actually use all of this same logic in \Drupal\content_moderation\Plugin\Validation\Constraint\ModerationStateConstraintValidator::getOriginalOrInitialState. I wonder if we should centralise that somewhere and make getting this info a lot easier.

Nice catch,
what about moving ::getOriginalOrInitialState() to the ModerationInformationInterface?

Yeah, seems like the most appropriate place. Lets do that!

Ok, so I moved ::getOriginalOrInitialState() to the ModerationInformationInterface

1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   @@ -240,4 +240,45 @@ public function getUnsupportedFeatures(EntityTypeInterface $entity_type) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getOriginalOrInitialState(ContentEntityInterface $entity) {
   

   Needs a change record to detail this new functionality.

2. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
   @@ -198,4 +198,26 @@ public function getWorkflowForEntityTypeAndBundle($entity_type_id, $bundle_id);
   +  public function getOriginalOrInitialState(ContentEntityInterface $entity);
   

   The name of this is interesting. It feels like too much implementation detail in the name. Why not getOriginalState()? That fact that for new entities this is the initial state from the Workflow seems okay to me. I know this name is from the old validator method but now it is going on a public interface we need to consider the name a bit more. I've looked at the current ModerationInformationInterface and either name does not conflict with existing methods which is nice.

3. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   @@ -240,4 +240,45 @@ public function getUnsupportedFeatures(EntityTypeInterface $entity_type) {
   +      $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId());
   
   +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -118,13 +118,24 @@ public function form(FieldItemListInterface $items, array &$form, FormStateInter
   +      $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId());
   

   Multiple uncached reads of a revision. I wonder if we can try using $entity->original if it is there.

+++ b/core/modules/content_moderation/src/ModerationInformation.php
@@ -240,4 +240,45 @@ public function getUnsupportedFeatures(EntityTypeInterface $entity_type) {
+  /**
+   * {@inheritdoc}
+   */
+  public function getOriginalOrInitialState(ContentEntityInterface $entity) {

One concern I have with this using the initial state is the intersection between this issue and #2873287: Dispatch events for changing content moderation states which fires an event for state change. That defines the original state as NULL for new entities. Not the initial state for the workflow.

Addressing #30.2 and .3

I think #2873287: Dispatch events for changing content moderation states should fall in line and use this new method if it ever lands. I'm not sure NULL is ever a valid original state, since the initial state options the user has access to are based on a starting state selected by the system, even if that starting state was never saved to a specific entity revision.

With all that in mind, I think the new method makes it significantly easier for other integrations to query what is happening during the entity lifecycle with regards to moderation states, so hopefully that addresses #31?

With that in mind, I think all that's left is a review of the interdiff from #32 and a change record.

+++ b/core/modules/content_moderation/src/ModerationInformation.php
@@ -243,17 +243,20 @@ public function getUnsupportedFeatures(EntityTypeInterface $entity_type) {
+      if (empty($entity->original)) {
+        /** @var \Drupal\Core\Entity\ContentEntityInterface $original_entity */
+        $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId());
+        if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) {
+          $original_entity = $original_entity->getTranslation($entity->language()->getId());
+        }
+        $entity->original = $original_entity;
       }
-      if ($workflow_type->hasState($original_entity->moderation_state->value)) {
-        $state = $workflow_type->getState($original_entity->moderation_state->value);
+      if ($workflow_type->hasState($entity->original->moderation_state->value)) {
+        $state = $workflow_type->getState($entity->original->moderation_state->value);

To be honest, I thought $entity->original->moderation_state was broken, see this issue. Not sure why it serves our purposes in this specific instance.

I wonder if we should avoid using ->original at all i this patch. It feels a lot like a circular dependency, especially when accessing the ModerationStateFieldItemList class from it and I think is a complexity that is worth avoiding, given the complex nature of what we're already dealing with. What do you think @alexpott?

Assigning ->original also seems like a new and risky side-effect, with a complex debugging profile.

Overall my preference would be to revert #30.3 before RTBC.

@Sam152 @chr.fritsch -- I'm okay with reverting #30.3 for now. I do think we should provide a safe way of accessing the original entity at all times in a safe manner but I concede that $entity->original is not it. That is only assigned during preSave - \Drupal\Core\Entity\EntityStorageBase::doPreSave()

Let's revert those changes and open / look for a follow-up to add a better way of getting the original entity in all situations.

Ok, so I am starting again from #29 and only applied #30.2.

I am also linking #2839195: Add a method to access the original property as a potential follow-up.

Personally the ->original property always felt a bit of a mystery with regards to: when will it exist and what exactly does it represent. As far as I've encountered it, I can only define it as: the version of the entity we compare against for the purposes of evaluating what storage changes need to be made. Explicit methods seem like a reliable path forward and it looks like that is already being discussed in the related issue.

Thanks for reviewing again @alexpott and @chr.fritsch for sticking with it. This is a super annoying and quite visible bug, so appreciate the persistence. I've done another full detailed review of the whole patch.

These are all +1s or comments, the only code changes I think we should make are points 4 and 7.

1. +    // If the entity is not new, grab the most recent revision and
   +    // load it. The moderation state of the saved revision will be used
   +    // to display the current state as well determine the the appropriate
   +    // transitions.
   +    if (!$entity->isNew()) {
   +      /** @var \Drupal\Core\Entity\ContentEntityInterface $original_entity */
   +      $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId());
   +      if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) {
   +        $original_entity = $original_entity->getTranslation($entity->language()->getId());
   +      }
   +    }
   

   Okay, so here we reload the entity in storage for the purposes of calling getValidTransitions, since that method is the valid transitions for that entity, at that point in time, not necessarily what is already in storage which when dealing with a widget is the only thing we're interested in.

   Makes sense to me.

2. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   @@ -240,4 +240,45 @@ public function getUnsupportedFeatures(EntityTypeInterface $entity_type) {
   +  public function getOriginalState(ContentEntityInterface $entity) {
   ...
   +  protected function isFirstTimeModeration(ContentEntityInterface $entity) {
   

   The control flow for these methods match exactly what was already present in ModerationStateConstraintValidator, so +1 to keeping the scope narrow in this issue.

3. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
   @@ -198,4 +198,26 @@ public function getWorkflowForEntityTypeAndBundle($entity_type_id, $bundle_id);
   +  /**
   +   * Gets the original or initial state of the given entity.
   +   *
   

   This whole doc-block reads okay to me for the purposes of the interface. I think the specific info about how this method is used for validation makes sense to live here.

4. +++ b/core/modules/content_moderation/src/StateTransitionValidation.php
   @@ -42,7 +42,7 @@ public function __construct(ModerationInformationInterface $moderation_info) {
   -    $current_state = $entity->moderation_state->value ? $workflow->getTypePlugin()->getState($entity->moderation_state->value) : $workflow->getTypePlugin()->getInitialState($entity);
   +    $current_state = !$entity->isNew() && $entity->moderation_state->value ? $workflow->getTypePlugin()->getState($entity->moderation_state->value) : $workflow->getTypePlugin()->getInitialState($entity);
   

   It looks like this was introduced in #8 but I can't figure out if/why we need it. I think this should be reverted as well, and if not followed-up with a very specific kernel test that proves why it exists.

5. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -520,4 +520,135 @@ public function testWorkflowInUse() {
   +    ], t('Save'));
   ...
   +    $this->drupalPostForm($edit_path, $edit, t('Save'));
   ...
   +    ], t('Save'));
   ...
   +    $this->drupalPostForm($edit_path, $edit, t('Preview'));
   

   No need to translate these strings. I can see you're following the pattern in the rest of the test though, so maybe lets leave this and follow-up to fix the whole test at once.

6. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -520,4 +520,135 @@ public function testWorkflowInUse() {
   +
   +    // Create pending draft revision.
   +    $this->drupalPostForm($edit_path, [
   +      'title[0][value]' => 'Third revision ',
   +      'moderation_state[0][state]' => 'draft',
   +    ], t('Save'));
   +
   +    // Confirm the current state is Draft and not Published.
   +    $this->drupalGet($edit_path);
   +    $this->assertSession()->pageTextContains('Current state Draft');
   

   This could probably be excluded from the test, I think we've proved the test case by this point.

7. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -520,4 +520,135 @@ public function testWorkflowInUse() {
   +    // Adding languages requires a container rebuild in the test running
   +    // environment so that multilingual services are used.
   +    $this->rebuildContainer();
   

   This seemed questionable, but I realised this is very similar to the existing test case in the file, that makes me wonder: should we actually just insert "Preview" -> "Back to edit" button presses into the existing test cases? The rest of the test case should be unaffected if this bug has been resolved and the duplication of the steps in these two new test cases seems like it'll be a pain to maintain.

Implementing the feedback from #39.

So we're adding a method to an interface. That means we can only commit this to 8.8.x as per https://www.drupal.org/core/d8-bc-policy#interfaces - this falls under the "Interfaces within non-experimental, non-test modules not tagged with either @api or @internal" section.

http://grep.xnddx.ru/search?text=implements+ModerationInformationInterfa... shows that nothing in contrib is implementing it - workbench moderation has its own interface with the same name.

Perhaps we could backport this simply by copying the method on to the widget class?

It has been broken a long time, so also probably not the end of the world to ship it in 8.8.

Reroll and removing stray newline.

@alexpott did you have any other feedback on the implementation? If this is fine for 8.8 I'm willing to backport it without any interface changes.

I think #43 is fine for 8.8.x as long as we have a change record. As I pointed out in #41 method additions are permitted in minor releases.

And yeah if we what to backport to 8.7.x we can put the method on the widget but fixing it right first in 8.8.x feels correct.

Added a change record: Additional "getOriginalState" method added to the moderation information service and ModerationInformationInterface

Most of the legwork was done by @chr.fritsch, so RTBC, would be good to get this long standing bug resolved.

Credited @malcomio, @alexpott, @timmillwood for code reviews.

Committed 4a1e820 and pushed to 8.8.x. Thanks!

Backported to 8.7.

Let's get this into to 8.7

I have tested preview with content moderation on 8.8.x and I have encountered following problem.

When you have state that can not be transitioned to it self. For example: draft->unpublished and unpublished->draft, but without unpublished->unpublished. When you select that state and go to preview and back, state selection combobox is not available anymore.

Note: it happens only for new content create. fe. /node/add/article

I have investigated further and found a solution. Here are patches for 8.8.x and 8.7.x (+ interdiff for 8.7).

I wanted to keep changes small, so that's easier to review solution. If solution is good, then I would just reverse if statement and check for isNew() first.

Lets open up a new issue, it seems a bit weird to continue committing incremental patches to 8.8 and then combined patches to 8.7. Lets commit the bugfix to 8.8 in a new issue and then backport both at once in either a new issue or one of the 8.8 issues?

Lets start fresh:

• #3065802: Preview and content moderation widget broken for unsaved entities
• #3065804: Backport content moderation preview fixes to 8.7