Use EntityPublishedInterface during moderation of entities to add support beyond nodes

This makes sense, is status guaranteed to be a boolean field even on custom entities? Likely but maybe not always.

Also, probably need a test for this behaviour, maybe it's already tested elsewhere and this passes because the behaviour for nodes hasn't changed?

I think we should maybe think about this depending on #2810381: Add generic status field to ContentEntityBase then we can be sure that anything with the 'status' entity key has a boolean status field.

We don't have any unit tests for the moderation handlers, maybe we should.
Another good way to test this would be to add a status entity key to one of the entity_test entity types, then add to one of the kernel tests.

Now that #2810381: Add generic status field to ContentEntityBase has been committed I think we need to re-visit this issue.

Entity keys are not going to be a requirement for a publishing status field, therefore we can't depend on that. It'll be nice when EntityPublishedInterface is in then we can check $entity_type instance of EntityPublishedInterface.

Until then we could use entities keys, because some entity types will have them, but we'll also need another check. This check can't be for a status field because some entity types have a status field which is not used for publishing status. Maybe we can check for setPublished or isPublished methods?

Should we not just get EntityPublishedInterface in and use that?

@catch yes, #2789315: Create EntityPublishedInterface and use for Node and Comment is ready for review. ;)

Postponing on #2789315: Create EntityPublishedInterface and use for Node and Comment.

Oops.

Now #2789315: Create EntityPublishedInterface and use for Node and Comment has been committed we can re-open this.

Looks good IMHO.

+++ b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
@@ -16,9 +16,6 @@ class NodeModerationHandler extends ModerationHandler {
   public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {
     if ($this->shouldModerate($entity, $published_state)) {
       parent::onPresave($entity, $default_revision, $published_state);
-      // Only nodes have a concept of published.
-      /** @var \Drupal\node\NodeInterface $entity */
-      $entity->setPublished($published_state);
     }
   }

\Drupal\content_moderation\Entity\Handler\NodeModerationHandler::shouldModerate() contains some logic whether we need to change the published flag or not, and this logic now applies to any entity type that implements EntityPublishedInterface, so we need to move that method up one level, use it in \Drupal\content_moderation\Entity\Handler\ModerationHandler::onPresave() and remove \Drupal\content_moderation\Entity\Handler\NodeModerationHandler::onPresave() completely.

This patch actually moves all of the node handler logic to the base.

That's exactly what I thought it would be needed :)

+++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
@@ -30,9 +31,33 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+        $entity->{$published_state ? 'setPublished' : 'setUnpublished'}();

I would prefer an explicit if / else block here, but I won't hold up a commit on it.

+++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
@@ -33,6 +34,10 @@ public function onPresave(ContentEntityInterface $entity, $default_revision, $pu
+      $entity->setPublished($published_state);

Can we update this for non-deprecated method calls - setPublished()/setUnPublished()?

Otherwise looks good.

I think @catch posted his comment without reloading the page, so he was looking at the patch in #25 which did have $entity->setPublished($published_state);, but it's nice that the tests are also updated now :)

@Sam152, you should upload test-only patches first so the testbot doesn't kick the issue back to NW because of them ;)

Needs a reroll... amazingly not that big..

git ac https://www.drupal.org/files/issues/2812811-set-published-states-37.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20671  100 20671    0     0   6007      0  0:00:03  0:00:03 --:--:-- 77419
error: patch failed: core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php:6
error: core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php: patch does not apply

Reading this patch makes me wonder why the NodeModerationHandler exists and why the handlers are also doing form alters. Looking at \Drupal\content_moderation\Entity\Handler\ModerationHandlerInterface it is a pretty confused thing. It's methods are:

• onPresave
• onBundleModerationConfigurationFormSubmit - I can't even see where this is used :(
• enforceRevisionsEntityFormAlter
• enforceRevisionsBundleFormAlter

I think before committing this - which makes sense we should:

• File a followup to address the above issue - or maybe 2 followups - one to remove the dead code and the other to split the transition logic and form alter logic into tow different things.
• Add a test for an entity type that does not implement \Drupal\Core\Entity\EntityPublishedInterface

Filed the two follow up issues.
#2842040: Remove unused code from Content Moderation handlers
#2842042: Review, organise, and clean up content moderation handlers

Going to look at the test for unpublishable entities now.

I had thought about adding a further test entity type and test bundle to test entities that implement EntityPublishedInterface and those that don't, but thought Node is testing EntityPublishedInterface, and EntityTestWithBundle can test without EntityPublishedInterface.

Adding entity_test_publish_with_bundle which implements EntityPublishedInterface so we now test three entity types.

Adding extra test coverage with block_content too.

Some high-level overview for now, I'm going to go into nitpick mode after the big questions are answered :)

1. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -81,26 +105,51 @@ public function testBasicModeration() {
   +    if ($entity instanceof EntityPublishedInterface) {
   +      $this->assertFalse($entity->isPublished());
   +    }
   

   This looks suspicious, how can a entity type be moderated without being a publishable entity type?

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -301,24 +339,43 @@ public function testNonLangcodeEntityTypeModeration() {
   +  protected function setEntityTestWithBundleKeys($keys, $remove_keys = []) {
   +    $entity_type = clone \Drupal::entityTypeManager()->getDefinition('entity_test_with_bundle');
   +    $original_keys = $entity_type->getKeys();
   +    foreach ($remove_keys as $remove_key) {
   +      unset($original_keys[$remove_key]);
   +    }
   +    $entity_type->set('entity_keys', $keys + $original_keys);
   +    \Drupal::state()->set('entity_test_with_bundle.entity_type', $entity_type);
   +    \Drupal::entityDefinitionUpdateManager()->applyUpdates();
   +  }
   

   Since we now define a new test entity type class, do we still need to test this one as well?

3. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestPublishWithBundle.php
   @@ -0,0 +1,81 @@
   +class EntityTestPublishWithBundle extends ContentEntityBase implements EntityPublishedInterface {
   

   Does this entity type really need to have bundles?

Discussed with @timmillwood and my main concern from #58 is actually: why do have to test with entity types that store bundles in config (i.e. a config entity type). He's going to look into that now.

After looking into this today it's not going to be possible to use content moderation without a config bundle until we resolve #2799785: Entity types with non-config bundles can not be moderated. Also I think we do need to test entity types that can be published and entity types that can't be published until we decide on #2842692: Warn users before enabling moderation on an entity type that can't be (un)published.

+++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
@@ -30,9 +31,33 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {

After some thought about this handlers, do we really need onPresave in them? Why don't we just put this code in \Drupal\content_moderation\EntityOperations::entityPresave?

Simplifying the patch now we don't need bundles for testing.

I don't think we really need the presave in the handler, lets move it out.
Also removing an unused schema from a previous version of the patch.

We can discuss removing the presave (and maybe the whole handler) in #2842042: Review, organise, and clean up content moderation handlers.

Interdiff based on #63.

I reviewed this patch a few times already, and the latest one looks great!

1. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
   @@ -30,9 +31,33 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   -    // This is probably not necessary if configuration is setup correctly.
   -    $entity->setNewRevision(TRUE);
   -    $entity->isDefaultRevision($default_revision);
   +    if ($this->shouldModerate($entity, $published_state)) {
   ...
   +  /**
   +   * Check if an entity's default revision and/or state needs adjusting.
   +   *
   +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   +   *   The entity to check.
   +   * @param bool $published_state
   +   *   Whether the state being transitioned to is a published state or not.
   +   *
   +   * @return bool
   +   *   TRUE when either the default revision or the state needs to be updated.
   +   */
   +  protected function shouldModerate(ContentEntityInterface $entity, $published_state) {
   +    // @todo clarify the first condition, https://www.drupal.org/node/2834953.
   +    // First condition is needed so you can add a translation.
   +    // Second condition checks to see if the published status has changed.
   +    return $entity->isDefaultTranslation() || (!($entity instanceof EntityPublishedInterface) || $entity->isPublished() !== $published_state);
      }
   
   +++ b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
   @@ -74,22 +62,4 @@ public function enforceRevisionsBundleFormAlter(array &$form, FormStateInterface
   -  /**
   -   * Check if an entity's default revision and/or state needs adjusting.
   -   *
   -   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   -   *   The entity to check.
   -   * @param bool $published_state
   -   *   Whether the state being transitioned to is a published state or not.
   -   *
   -   * @return bool
   -   *   TRUE when either the default revision or the state needs to be updated.
   -   */
   -  protected function shouldModerate(ContentEntityInterface $entity, $published_state) {
   -    // @todo clarify the first condition.
   -    // First condition is needed so you can add a translation.
   -    // Second condition checks to see if the published status has changed.
   -    return $entity->isDefaultTranslation() || $entity->isPublished() !== $published_state;
   -  }
   

   I'm not sure about this logic change. Before we were always calling $entity->setNewRevision(TRUE) for non nodes and now we are not.

2. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
   @@ -30,9 +31,33 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   +        $entity->{$published_state ? 'setPublished' : 'setUnpublished'}();
   

   I think this is a pretty complex structure to grok and it makes finding code usages unnecessarily hard.

   How about (if you want to keep the ternary)

   $published_state ? $entity->setPublished() : $entity->setUnpublished();
   

   Or just use a regular if.

• Moved setNewRevision and isDefaultRevision back out of shouldModerate.
• Cleaned up ternary statement.

+++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
@@ -32,6 +33,30 @@ public function onPresave(ContentEntityInterface $entity, $default_revision, $pu
+    if ($this->shouldModerate($entity, $published_state)) {
+      if ($entity instanceof EntityPublishedInterface) {

We should probably combine these 'if's :)

Good point, merged publishing interface check into shouldModerate().

The check has moved to shouldModerate()

Removed shouldModerate(), this seems to work now.

This will also resolve #2834953: Clarify use of isDefaultTranslation() to determine if an entity values should be updated..

Funnily enough I think the idea that ContentModeration should only work on entity types that implement EntityPublishedInterface makes way more sense than the current requirement for revisions.

Anyhow this is a step in the right direction that makes it easier to use content moderation on more entity types without customisation.

Committed d55b420 and pushed to 8.3.x. Thanks!

diff --git a/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
index 28accc0..76f00e0 100644
--- a/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
+++ b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
@@ -3,7 +3,6 @@
 namespace Drupal\content_moderation\Entity\Handler;
 
 use Drupal\content_moderation\ModerationInformationInterface;
-use Drupal\Core\Entity\ContentEntityInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
diff --git a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
index 6338746..69bb580 100644
--- a/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
@@ -5,7 +5,6 @@
 use Drupal\content_moderation\Entity\ContentModerationState;
 use Drupal\Core\Entity\EntityPublishedInterface;
 use Drupal\entity_test\Entity\EntityTestBundle;
-use Drupal\entity_test\Entity\EntityTestRev;
 use Drupal\entity_test\Entity\EntityTestWithBundle;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\KernelTests\KernelTestBase;
@@ -385,7 +384,7 @@ protected function setEntityTestWithBundleKeys($keys, $remove_keys = []) {
   /**
    * Reloads the entity after clearing the static cache.
    *
-   * @param \Drupal\Core\Entity\EntityInterface
+   * @param \Drupal\Core\Entity\EntityInterface $entity
    *   The entity to reload.
    * @param int|bool $revision_id
    *   The specific revision ID to load. Defaults FALSE and just loads the
diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevPub.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevPub.php
index 4f8434e..862958e 100644
--- a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevPub.php
+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulRevPub.php
@@ -50,7 +50,7 @@
  *   }
  * )
  */
-class EntityTestMulRevPub extends EntityTestMulRev  implements EntityPublishedInterface {
+class EntityTestMulRevPub extends EntityTestMulRev implements EntityPublishedInterface {
 
   use EntityPublishedTrait;
 

Fixed some code style stuff on commit.

#2846830: Add changelog for Drupal 8.3.0