When enabling moderation apply a relative state

We'd possibly be guessing which state is appropriate if we didn't ask the user. Would it make sense to have a "default published" and a "default unpublished" state and make all the initialisation decisions based on two statuses instead of one?

Hm, it's tricky because even with an unpublished/published default, we'd still need to collect which state brand new items of content start their life out as. 3 states does perhaps feel overkill. Maybe these are configuration options that only appear when:

• Enabling moderation on a bundle for the first time.
• Enabling moderation on a bundle that actually has entities.

...and aren't stored and don't show up beyond that.

#4 doesn't account for entity created programatically in a published or unpublished state that don't set a moderation state (see #2839371: Programatically creating a published entity doesn't result in a published entity.). For other modules that interact with entities using EntityPublishedInterface, we will want content moderation to work and not squash those decisions, hence doing this as a once off when moderation is enabled is a no-go.

I don't think there much getting around needing to store three default states:

• We need a default state because entities start their life out neither published or unpublished.
• We need a default published state for entities that start their CM life published.
• We need a default unpublished state for entities that start their CM life unpublished.

I think this hasn't really been addressed before because the mental model is that content moderation takes over all things publish-related, but that of course isn't true of entities manipulated programatically without knowledge of CM or when CM is enabled for the first time.

If we can agree on that, I think we can move forward with a code solution.

I don't think we need to cope with

We need a default state because entities start their life out neither published or unpublished.

... this is not possible. Or if it is should be considered a bug.

I agree that we need a default published and default unpublished state to handle what happens to existing entities. I think these should be stored in the content moderation workflow because it allows for a new workflow to be deployed from a site with no entities to one with.

What about new entities that are created after CM is enabled with a publish status and no moderation status? Does the relative status get applied to those?

edit: for clarity

@Sam152 you can't actually do that :)...

    // Create a node as published.
    $node = Node::create([
      'type' => 'example',
      'title' => 'Test title',
    ]);
    $this->assertTrue($node->isPublished());
    $node->setPublished()->save();
    $this->assertFalse($node->isPublished());

    $node = $this->reloadNode($node);
    $this->assertEquals('draft', $node->moderation_state->value);
    $this->assertFalse($node->isPublished());

This passes when added to the end of \Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest::testBasicModeration

Basically whilst saving the node with no moderation status it'll just do what it should and ignore the prior nonsense of setting it to published because once you've enable moderation it wins.

The case in #11 makes sense, but I was considering the case of programatically setting the status of the node after moderation was already enabled.

Before the workflow component split, this used to work and published content would stay published, so my expectations were that EntityPublishedInterface would still be meaningful outside the context of moderation. When this behaviour changed it prompted me to open #2839371: Programatically creating a published entity doesn't result in a published entity..

Having read the opinions of others, I do now agree with the conclusion that CM should totally restrict and control the publishing status for safety, but it does "break" any other code that wants to publish or unpublish something.

The reason this all relates to this issue is that the relatives states could be applied any time something manages to publish or unpublish, so both CM and users of EntityPublishedInterface could continue to work, but as others have already stated, sounds dangerous :)

Good point re: not requiring a third state, two states sound good.

This relates to #2843083: Select entity type / bundle in workflow settings in the sense we'll need a whole workflow settings form for these settings.

Looking pretty good. Comments/questions as follows.

1. +++ b/core/modules/content_moderation/config/install/workflows.workflow.editorial.yml
   @@ -60,4 +60,6 @@ type_settings:
   +  default_unpublished: draft
   +  default_published: published
   

   I think we're still missing the UI to actually select these right? Sounds like it would be good to ensure the discussions in #2843083: Select entity type / bundle in workflow settings are aware the plugin-based workflow settings form is likely to grow to at least two more inputs.

2. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -230,7 +231,15 @@ public function deleteState($state_id) {
   +  public function getInitialState($entity = NULL) {
   

   It feels a bit wrong for the workflow entity to know about content publishing and entities in this manner. Perhaps this should delegate the initial state to the workflow plugins instead, that way it can all be contained in the content_moderation plugin?

Re: #16.2, why is that? As far as I know workflows makes no assumptions that a workflow entity is controlling an item of content or even has anything to do with other entities at all. AFAIK content moderation is the coupling between content entities/publishing and the workflow config entity.

Here is an idea for a different approach. Not totally convinced but it solves a few problems. Namely:

1. $entity doesn't have to become part of the 'getInitialState' method, keeping entity-centric things out of workflow interfaces.
2. All content moderation features which assume the existence of the corresponding content_moderation_state entity when dealing with states will work (the views integration for example).

One downside is the extra complexity, the other approach didn't have to save any entities.

If we decouple Workflow entity from Workflow Type plugin - then the approach in #20 becomes way more via as an overload of the initialState method that is already on the workflow type plugin. Just need to do the not small task of #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface

Yeah, I the simpler approach is probably preferable, provided it doesn't behave different to entities with a valid content_moderation_state counterpart in too many scenarios. Views is one of those, but perhaps not a deal breaker.

@Sam152 you're right - we need to actually do all the ContentModerationState content entity creating... nice start. I.e. views is a deal breaker.

Okay, I can have a look at resolving the @todos and polishing this up of the approach is agreed upon.

Here is a patch that resolves the @todos and adds test coverage for the issues #24 had. I think this is ready, but a possible follow up could be connecting the queue items to a batch in the UI after saving a workflow, however this just gets normally processed on cron if the workflow entity is updated with a config import or programatically.

+++ b/core/modules/content_moderation/src/DefaultStateManager.php
@@ -40,13 +40,16 @@ public function createQueueItems(WorkflowInterface $workflow) {
-        // @todo, don't query for entities that already have associated
-        // content_moderation_state entities, otherwise states will rest on
-        // cron.

+++ b/core/modules/content_moderation/src/Plugin/QueueWorker/ModerationStateChange.php
@@ -51,7 +71,17 @@ public static function create(ContainerInterface $container, array $configuratio
+    $existing_state = $this->contentModerationStateStorage->loadByProperties([
+      'content_entity_id' => $entity->id(),
+      'content_entity_type_id' => $entity->getEntityTypeId()
+    ]);
+    if ($existing_state) {
       return;
     }

This was a bit of a different approach to this problem. Doing this in the query would require a tag and some altering to connect it to the content_moderation_state entity. Given it's not the default case that there will be lots of entities already under moderation when enabling a bundle, it seems okay to queue all entities in that bundle and filter out the ones with an existing state during the processing.

1. +++ b/core/modules/content_moderation/content_moderation.install
   @@ -0,0 +1,14 @@
   +function content_moderation_install() {
   +  $queue = \Drupal::queue('content_moderation_state_change');
   +  $queue->createQueue();
   

   Do we need to remove the queue on uninstall? Also why do we need to create the queue on install? Aggregator doesn't appear to do this. In fact I think this is unnecessary - i think queues are created on demand. What's a little amusing is that i'm not sure queue tables are removed.

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -238,11 +238,17 @@ function content_moderation_entity_bundle_info_alter(&$bundles) {
    function content_moderation_workflow_insert(WorkflowInterface $entity) {
   
   +++ b/core/modules/content_moderation/tests/src/Kernel/DefaultStatesTest.php
   @@ -0,0 +1,245 @@
   +        $entity->setUnpublished();
   

   I think we need to also do this on workflow update too.

3. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -238,11 +238,17 @@ function content_moderation_entity_bundle_info_alter(&$bundles) {
   +  if ($entity->getTypePlugin()->getPluginId() !== 'content_moderation') {
   +    return;
   +  }
   

   $entity->getEntityTypeId() can make this a bit shorter.

4. +++ b/core/modules/content_moderation/src/DefaultStateManager.php
   @@ -0,0 +1,62 @@
   +        $query = $entity_storage->getQuery();
   +        $query->condition($entity_definition->getKey('bundle'), $bundle_id);
   +
   +        foreach ($query->execute() as $entity_id) {
   +          $this->queue->createItem([
   +            'entity_id' => $entity_id,
   +            'entity_type_id' => $entity_type_id,
   +          ]);
   +        }
   

   This looks like it'll blow when there are lots and lots of entities. This is the advantage of @timmillwood's approach. Maybe there is a way to make it work with views. The scale of the problem when you enable a workflow on a bundle with 1000s of entities already scares me. But for content_moderation to work somehow we have to solve this.

5. I would have expected some UI changes as well. I would have expected the site to enter a batch process after enabling moderation on a node type to process existing nodes. Maybe we should postpone this issue on the one to move the entity_type/bundle selection to the workflow interface?

More on point 4. I think we need 2 queues to do this properly at scale. One queue to save this entity type/bundle ID and another to store the entities to process. The entity type/bundle ID will be responsible for adding entities to the other queue. It'll need to store where it is at - and the highest entity ID when it was created.

Re: #33, the approach was to ensure views support works.

Two questions before addressing the feedback and continuing with this:

• Are we sure views support can't work with the original solution.
• Are we sure that's the only thing that breaks when there is no actual content_moderation_state entity backing the status?

Are we sure views support can't work with the original solution.

I think Views support could be fixed with some COALESCE() trickery. Not sure that it will actually work for this complex case where two different values (moderation states) have to be returned based on the value of an entity field, but it's worth a shot.

Are we sure that's the only thing that breaks when there is no actual content_moderation_state entity backing the status?

What would definitely break are entity queries (e.g. an entity query for: "give me a list of article nodes that are in a 'draft' state"). However, 'moderation_state' is a computed field and I don't think EQ support querying on those, so we should be fine in this regard.

I think #37 is perhaps more complicated based on the fact it's not based on a field, but an interface. We could assume something about what's going on behind the scenes based on entity keys, but it's not perfect.

In any case, I'd be interested in seeing what it looks like.

Few nits and things. Otherwise, as per IRC, lets get this in as a first pass and look at creating some kind of batch to make views work as a follow up.

1. +++ b/core/modules/content_moderation/tests/src/Kernel/InstallTest.php
   @@ -0,0 +1,79 @@
   + * Tests links between a content entity and a content_moderation_state entity.
   

   Old doc comment?

2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -289,4 +290,19 @@ public function getConfiguration() {
   +    if ($entity instanceof EntityPublishedInterface) {
   +      if ($entity->isPublished()) {
   +        return $workflow->getState('published');
   +      }
   +      else {
   +        return $workflow->getState('draft');
   +      }
   +    }
   +    return parent::getInitialState($workflow, $entity);
   

   Can probably reduce a level of nesting here with:

   return $workflow->getState($entity->isPublished() ? 'published' : 'draft');

3. +++ b/core/modules/content_moderation/tests/src/Kernel/InstallTest.php
   @@ -0,0 +1,79 @@
   + * Tests links between a content entity and a content_moderation_state entity.
   

   Old comment?

4. +++ b/core/modules/content_moderation/tests/src/Kernel/InstallTest.php
   @@ -0,0 +1,79 @@
   +class InstallTest extends KernelTestBase {
   

   Maybe a better name for this? InitialStateTest?

5. +++ b/core/modules/content_moderation/tests/src/Kernel/InstallTest.php
   @@ -0,0 +1,79 @@
   +
   

   Stray newline

6. +++ b/core/modules/workflows/src/WorkflowTypeInterface.php
   @@ -94,6 +94,17 @@ public function decorateTransition(TransitionInterface $transition);
   +   * @param \Drupal\Core\Entity\EntityInterface|NULL
   +   *   The entity to get the initial state for, if there is one.
   ...
   +  public function getInitialState(WorkflowInterface $workflow, $entity = NULL);
   

   Lets just overload $entity in the ContentModeration type plugin. Passing workflow is a bit annoying but necasary for the current structure I think.

Created #2867312: Create a batch process to ensure entities that exist for newly moderated entity types get a corresponding ContentModerationState entity, so that views works..

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -289,4 +290,14 @@ public function getConfiguration() {
+    return parent::getInitialState($workflow, $entity);

Lets not pass $entity to parent, otherwise RTBC.

Fixing tiny nit, then can RTBC if green.

Committed and pushed 469003d to 8.4.x and 013edec to 8.3.x. Thanks!

Backported to 8.3.x because content_moderation and workflow are both experimental.

diff --git a/core/modules/content_moderation/tests/src/Kernel/InitialStateTest.php b/core/modules/content_moderation/tests/src/Kernel/InitialStateTest.php
index 35c3174..0150d3c 100644
--- a/core/modules/content_moderation/tests/src/Kernel/InitialStateTest.php
+++ b/core/modules/content_moderation/tests/src/Kernel/InitialStateTest.php
@@ -78,4 +78,5 @@ public function testInitialState() {
     $this->assertEquals('published', $loaded_published_node->moderation_state->value);
     $this->assertEquals('draft', $loaded_entity_test->moderation_state->value);
   }
+
 }
diff --git a/core/modules/workflows/src/WorkflowTypeInterface.php b/core/modules/workflows/src/WorkflowTypeInterface.php
index 128f148..2412bf9 100644
--- a/core/modules/workflows/src/WorkflowTypeInterface.php
+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -96,7 +96,7 @@ public function deleteTransition($transition_id);
   /**
    * Gets the initial state for the workflow.
    *
-   * @param \Drupal\workflows\WorkflowInterface
+   * @param \Drupal\workflows\WorkflowInterface $workflow
    *   The workflow entity.
    *
    * @return \Drupal\workflows\StateInterface

Coding standards fixes on commit.