Architectural review of the Content Moderation module

With the beta deadline tomorrow and all "WI Critical" issues RTBC or Fixed we're in a good place, but the review of Content Moderation hasn't been done yet.

Looking at #2755073: WI: Content Moderation module roadmap there is one final "must have":
#2862041: Provide useful Views filters for Content Moderation State fields

Two "should have issues:
#2873287: Dispatch events for changing content moderation states - This isn't a major priority because hook_entity_update and hook_entity_create can be used, although not ideal.
#2890380: Remove descriptions of permissions in Content Moderation - This doesn't provide any added features, and isn't a BC break, it just brings Content Moderation in line with all other core modules.

Aiming to have a look at this tomorrow

I've started taking a look found a couple of very minor nit picks.

Here's what I have so far:

content_moderation.module:

1.  * Many default node templates rely on $page to determine whether to output the
    * node title as part of the node content.
   

   Not sure we need this comment

2. Most hooks are abstracted out into classes, but 9 are not. Abstracting hooks into classes is something we inherited from Workbench Moderation.
3. content_moderation_workflow_update() calls content_moderation_workflow_insert(), this seems a bit weird.

ContentModerationStateInterface.php

1. It's a bit weird that Drupal\content_moderation\ContentModerationStateInterface isn't for Drupal\content_moderation\ContentModerationState, it's for Drupal\content_moderation\Entity\ContentModerationState. Maybe we should move the interface into the Entity directory.

EntityOperations

1. We should document (in the docblock) that entityPresave is a Hook bridge.
2. In entityPresave $entity->moderation_state->value should never return NULL, so do we really need the check?

EntityTypeInfo

1. The documentation of hook bridges here is inconsistent with EntityOperations.
2. Many of the hook bridges are missing documentation that they are hook bridges.

Handler classes

1. Could we mark the handlers internal until #2842042: Review, organise, and clean up content moderation handlers is decided?

1. core/modules/content_moderation/src/EntityTypeInfo.php contains leftover from times it was an 'entity_reference':
->setSetting('target_type', 'moderation_state')
2. core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php injects StateTransitionValidation but doesn't use it

Fixing everything mentioned apart from #6content_moderation.module.2 because I'm not sure we really need to abstract all hooks into classes.

Good spot!

Patch fixes both items in #8.

I guess $entity->moderation_state->value can be null.

Thanks @tim.plunkett, we should be able to address most of these here.

#14.1 - Added ContentModerationInterface.
#14.2 - Agreed
#14.3 - StateInterface and Transition interface are @internal, so I guess we could remove them in 8.5.x if needed.
#14.4 - Switched to AccessException.
#14.5 - Not sure what to do for the best here, I personally don't have an issue with the optional param, but one suggestion from @sam152 was to add a method such as getModerationInitialState($entity); which is just used by Content Moderation.
#14.6 - Moved EntityModerationRouteProvider to \Drupal\content_moderation\Entity.

Great clean up, thoughts below.

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -276,18 +273,25 @@ function content_moderation_entity_bundle_delete($entity_type_id, $bundle_id) {
    function content_moderation_workflow_insert(WorkflowInterface $entity) {
   -  // Clear bundle cache so workflow gets added or removed from the bundle
   -  // information.
   -  \Drupal::service('entity_type.bundle.info')->clearCachedBundles();
   -  // Clear field cache so extra field is added or removed.
   -  \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
   +  _content_moderation_clear_entity_cache();
    }
   ...
    function content_moderation_workflow_update(WorkflowInterface $entity) {
   -  content_moderation_workflow_insert($entity);
   +  _content_moderation_clear_entity_cache();
   +}
   ...
   +function _content_moderation_clear_entity_cache() {
   +  // Clear bundle cache so workflow gets added or removed from the bundle
   +  // information.
   +  \Drupal::service('entity_type.bundle.info')->clearCachedBundles();
   +  // Clear field cache so extra field is added or removed.
   +  \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
    }
   

   Maybe we should just copy these cache clears into both functions?

2. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -113,13 +117,13 @@ public function entityPresave(EntityInterface $entity) {
      /**
   -   * Hook bridge.
   -   *
       * @param \Drupal\Core\Entity\EntityInterface $entity
       *   The entity that was just saved.
       *
   @@ -133,8 +137,6 @@ public function entityInsert(EntityInterface $entity) {
   

   Maybe these doc comments can be Implements hook_something(). like the hooks themselves? Do we have any precedent for this?

3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -31,7 +31,7 @@
   -class ContentModeration extends WorkflowTypeBase implements ContainerFactoryPluginInterface {
   +class ContentModeration extends WorkflowTypeBase implements ContentModerationInterface {
   
   +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModerationInterface.php
   @@ -0,0 +1,74 @@
   +interface ContentModerationInterface extends WorkflowTypeInterface, ContainerFactoryPluginInterface {
   

   I think we need to move ContainerFactoryPluginInterface back to being directly used by our implementation. We can't say for sure all implementations will need to use a factory and ::create is an implementation detail that shouldn't be part of the public api.

4. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModerationInterface.php
   @@ -0,0 +1,74 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getInitialState($entity = NULL);
   

   Maybe this is the perfect spot to explain why this exists if we don't decide to make it a new method.

#17.1 - yeh, that works too.
#17.2 - I made it universal to use @see, we don't have anywhere else in core doing this, but as @see is a recognised format it seemed to make sense.
##1.2a - Fixed
#17.3 - Added @param $entity.

+1 RTBC. Hopefully @tim.plunkett can review.

This looks like good clean-up.

I already did a major architectural review of content_moderation when it was first slated for commit (which ended up with the refactoring of workflows and some other patches), so haven't done a massive re-review - things are in a good spot generally.

Committed/pushed to 8.5.x and 8.4.x, thanks!

Push didn't make it.

One of two things that bugs me a lot with Content Moderation is the revision_tracker table, so I posted a patch in #2865579: Rewrite the 'Latest revision' views filter and remove the revision_tracker table hoping that we can get rid of it.

The other one is that \Drupal\content_moderation\ModerationInformationInterface::getWorkflowForEntity() hard-codes the assumption that an entity bundle can have only one Content Moderation workflow, but I don't think we can do anything about that anymore..

I've RTBCd the revision_tracker table removal.

Also actually pushed the commits here now.

Lots of test fails so rolled this back.

@catch: Patch is ok, but instead of it only CS fix removing unneeded use statement was uploaded (e.g. check c7c9a329)

Good spot @tacituseu, I was just about to say, I can't replicate this issue locally.

Ouch! Sorry about that and good spot. Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!