Content moderation state entity field data not removed when the host field data is

Nice find!

+++ b/core/modules/content_moderation/content_moderation.module
@@ -92,6 +92,15 @@ function content_moderation_entity_update(EntityInterface $entity) {
+ * Implements hook_entity_delete().
+ */
+function content_moderation_entity_delete(EntityInterface $entity) {

I'm pretty sure we need to do the same for hook_entity_translation_delete() and hook_entity_revision_delete() :)

This looks great, the refactor into loadContentModerationStateEntity is a nice clean up.

+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
@@ -176,6 +178,23 @@ public function testBasicModeration($entity_type_id) {
+    // Test content moderation state deletion.
+    $entity2 = $entity_storage->create([
+      'title' => 'Test title',
+      $this->entityTypeManager->getDefinition($entity_type_id)->getKey('bundle') => $bundle_id,
+    ]);
+    $entity2->save();
+    $entity2 = $this->reloadEntity($entity2);
+    $entity2->delete();
+    $result = $this->entityTypeManager
+      ->getStorage('content_moderation_state')
+      ->loadByProperties([
+        'content_entity_type_id' => $entity_type_id,
+        'content_entity_id' => $entity2->id(),
+        'workflow' => $workflow->id(),
+      ]);
+    $this->assertFalse($result);

Do we need this in the middle of this test, any chance we can use the same data provider and spit it out? It's already pretty big.

This looks like an awesome cleanup in addition to the bug fix. I'll wait for the test results before diving deeper into the problems mentioned in #6, but here's a review in the meantime:

1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -138,6 +139,53 @@ public static function updateOrCreateFromEntity(ContentModerationState $content_
   +  public static function loadFromEntity(EntityInterface $entity) {
   

   How about loadFromModeratedEntity()?

2. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -220,6 +212,61 @@ protected function setLatestRevision(EntityInterface $entity) {
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity being deleted.
   ...
   +  public function entityDelete(EntityInterface $entity) {
   ...
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity revision being deleted.
   ...
   +  public function entityRevisionDelete(EntityInterface $entity) {
   +    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
   ...
   +   * @param \Drupal\Core\Entity\EntityInterface $translation
   +   *   The entity translation being deleted.
   ...
   +  public function entityTranslationDelete(EntityInterface $translation) {
   +    /** @var \Drupal\Core\Entity\ContentEntityInterface $translation */
   

   Since we know that all these hook bridges are only working with content entities, why don't we type hint that directly?

3. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -220,6 +212,61 @@ protected function setLatestRevision(EntityInterface $entity) {
   +    $content_moderation_state = ContentModerationStateEntity::loadFromEntity($entity);
   +    if ($content_moderation_state) {
   ...
   +      $content_moderation_state = ContentModerationStateEntity::loadFromEntity($entity);
   +      if ($content_moderation_state) {
   

   I would inline the assignments in the condition, but that's just personal preference, feel free to ignore it :)

I've looked into that @todo before (#2859455: Remove superfluous @todos from content moderation). The ContentModerationState entity makes no attempt to match the default-ness of the entity being moderated. The latest ContentModerationState entity will always be the default. I believe this is by design or hasn't been considered before.

Possibly just need to remove the $default_revision check in \Drupal\content_moderation\Entity\ContentModerationState::loadFromModeratedEntity? Something like the following:

diff --git a/core/modules/content_moderation/src/Entity/ContentModerationState.php b/core/modules/content_moderation/src/Entity/ContentModerationState.php
index 563f6c1..9fb43c2 100644
--- a/core/modules/content_moderation/src/Entity/ContentModerationState.php
+++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
@@ -163,21 +163,19 @@ public static function loadFromModeratedEntity(EntityInterface $entity) {
       $query = $storage->getQuery()
         ->condition('content_entity_type_id', $entity->getEntityTypeId())
         ->condition('content_entity_id', $entity->id())
-        ->condition('workflow', $moderation_info->getWorkflowForEntity($entity)->id());
-
-      if (!$default_revision) {
-        $query
-          ->allRevisions()
-          ->condition('content_entity_revision_id', $entity->getRevisionId());
-      }
+        ->condition('workflow', $moderation_info->getWorkflowForEntity($entity)->id())
+        ->condition('content_entity_revision_id', $entity->getRevisionId())
+        ->allRevisions();
 
       $ids = $query->execute();
       if ($ids) {
         /** @var \Drupal\content_moderation\ContentModerationStateInterface $content_moderation_state */
-        $content_moderation_state = $default_revision ?
-          $storage->load(current($ids)) : $storage->loadRevision(key($ids));
+        $content_moderation_state = $storage->loadRevision(key($ids));
         if ($content_moderation_state instanceof ContentModerationStateInterface) {
           $result = $content_moderation_state;
+          if (!$entity->isDefaultTranslation()) {
+            $result = $content_moderation_state->getTranslation($entity->language()->getId());
+          }
         }
       }
     }

Note loading the matching translation as well.

Removing the flag and fixing with getLoadedRevisionId(). Seems to do the trick.

Sorry, a bit nitpicky. Otherwise, looks fine.

+++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
@@ -138,6 +139,46 @@ public static function updateOrCreateFromEntity(ContentModerationState $content_
+          $result = $content_moderation_state;

We could do an early return here. Then we don't need to set $result to NULL, and don't need to return $result. Also, $result isn't a very clear variable name. Without reading the docblock I'd expect a DB query result.

How about the following tidy-up? loadRevision already returns NULL, so no need to check that exists.

That looks like a great tidy up! 8 lines removed!

I'm not going to be too precious either way, and good catch on the PHP doc.

+1 LGTM

Perfect!

I looked over this patch as well and I could only find two small cosmetic issues. Fixed them quickly with the attached interdiff :)

1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -138,6 +139,44 @@ public static function updateOrCreateFromEntity(ContentModerationState $content_
   +
   +      $ids = $storage->getQuery()
   +        ->condition('content_entity_type_id', $entity->getEntityTypeId())
   +        ->condition('content_entity_id', $entity->id())
   +        ->condition('workflow', $moderation_info->getWorkflowForEntity($entity)->id())
   +        ->condition('content_entity_revision_id', $entity->getLoadedRevisionId())
   +        ->allRevisions()
   +        ->execute();
   +
   

   This should have an accessCheck(FALSE). In practice I can't ever see it being necessary, but bad if it does and it's a common mistake with nodes that are affected by node access modules.

2. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -198,6 +183,12 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
   +    // @todo what if $entity->moderation_state is null at this point?
   

   Is this still a @todo and if so does it need an issue?

That @todo is probably not relevant anymore now that #2844594: Default workflow states and transitions is in.

Fixing feedback from #31, I think these changes are small enough to go back to RTBC.

LGTM!

+++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -222,6 +212,61 @@ protected function setLatestRevision(EntityInterface $entity) {
+      $content_moderation_state = ContentModerationStateEntity::loadFromModeratedEntity($entity);
+      if ($content_moderation_state) {
+        $this->entityTypeManager
+          ->getStorage('content_moderation_state')
+          ->deleteRevision($content_moderation_state->getRevisionId());
+      }
+    }
+  }

Sorry another one I didn't notice first look through and these are all nits... isn't this identical to $content_moderation_state->deleteRevision($content_moderation_state->getRevisionId());?

@plach it looks like you're correct.

As this is already RTBC it should be fine for 8.4.x

Yes you're right, not sure where I came up with that non-existent method from, but looks good from here then.

Rebase of patch from #40 with latest 8.4.x branch.

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