Add experimental content_moderation module

>Proposed module name: Entity Moderation (entity_moderation.module)

As this is going to cover content entities and we have content translation module, it would make more sense as content_moderation module imo.

The current working plan (from discussion elsewhere) is to take Workbench Moderation 1.0 as is, rename it, and put it into core directly as an experimental module. We can then iterate as necessary to smooth out any rough edges and transfer some of its utility code directly into Entity API.

Tim Millwood has already started the rename work here: https://github.com/timmillwood/content_moderation

Is anyone opposed to this approach?

Nice!

Trying to address the failing tests.

#2753733: AccountProxy can do unnecessary user loads to get an ID was opened for the ConfigImportAllTest fail, and I implemented a workaround in the meantime. Let's see if upper-casing the module name in its info.yml file fixes the other test fail.

I also fixed a few small things that jumped out at a quick glance through the .module file.

Just a heads up that we're going to commit these to WBM this week

• #2750365: Permissions: moderation states with the same name is unclear for user to assign to a role
• #2702041: Add views relationship from content to latest revision
• #2708941: Allow roles to view but not administer moderation state

@larowlan - Thanks for the heads up, I guess for now (until WBM 8.x-2.x) we need to re-roll all contrib patches against core, and all core patches against contrib.

Looking deeper into the failing test

1) Drupal\KernelTests\Core\Config\DefaultConfigTest::testDefaultConfig
Schema key views.view.latest:display.default.display_options.filters.latest_revision.value failed with: missing schema

This should do it.

Removed a lot of unrelated changes from the patch.

That seems totally fixable!

+++ b/core/modules/content_moderation/config/schema/content_moderation.schema.yml
@@ -48,3 +48,14 @@ block_content.type.*.third_party.content_moderation:
+views.filter.latest_revision:
+  type: views_filter
+  label: 'Latest revision'
+  mapping:
+    value:
+      type: sequence
+      label: 'Values'
+      sequence:
+        type: string
+        label: 'Value'

Does that really make sense? The least revision should not have any kind of configuration for the value.

@dawehner, I agree, but then I wonder how did the 'value' key got into core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.latest.yml:

...
+      filters:
+        latest_revision:
+          id: latest_revision
+          table: node_revision
+          field: latest_revision
+          relationship: none
+          group_type: group
+          admin_label: ''
+          operator: '='
+          value: ''
+          group: 1
+          exposed: false
+          expose:
+            operator_id: ''
+            label: ''
+            description: ''
+            use_operator: false
+            operator: ''
+            identifier: ''
+            required: false
+            remember: false
+            multiple: false
+            remember_roles:
+              authenticated: authenticated
+          is_grouped: false
+          group_info:
+            label: ''
+            description: ''
+            identifier: ''
+            optional: true
+            widget: select
+            multiple: false
+            remember: false
+            default_group: All
+            default_group_multiple: {  }
+            group_items: {  }
+          entity_type: node
+          plugin_id: latest_revision
...

I assume that's why the test is complaining about missing schema for 'value'.

It would be worthwhile to have a discussion about this. About the scope, approach and get initial signoffs before proceeding. The idea of a plan issue to get that.

The idea of putting a patch in this issue at this moment in time, baffles me.

@Bojhan - When discussing this with Alex and Catch we had verbal sign off to get Workbench Moderation module in as Content Moderation experimental module. This will help expose and test many core issues around this space.

That being said we should have more discussion around this.

Patch in #24 seems good to me.

You know I just looked quickly at the test coverage as well, and thought about fixing things correctly but andrei was simply faster/did it. Don't worry

That's great, still needs product manager and subsystem maintainer sign-off :)

This received no UI review, so this cannot be RTBC.

Some review points. Who is the subsystem maintainer here? We are missing MAINTAINERS.txt entry as well.

1. +++ b/core/modules/content_moderation/content_moderation.install
   @@ -0,0 +1,40 @@
   +  // Some modules, such as Entity Pilot, seem to have a weirdness with their
   +  // base field definition such that an entity may end up in this list that
   

   Can we generalize this comment?

2. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -0,0 +1,267 @@
   +   * @var \Drupal\content_moderation\ModerationInformationInterface
   ...
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   

   Desc block is missing.

3. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -0,0 +1,267 @@
   +    // @todo make this more functional, less iterative.
   

   Issue link please.

4. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -0,0 +1,358 @@
   +   *   - delete: (optional) String containing markup (normally a link) used as the
   ...
   +   *   - entity: The machine name of an entity, such as "node" or "block_content".
   

   More then 80 chars.

5. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -0,0 +1,358 @@
   +    // @todo write a test for this.
   ...
   +      // @todo write a test for this.
   ...
   +      // @todo write a custom widget/selection handler plugin instead of
   +      // manual filtering?
   

   Don't we need issue links for these?

6. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -0,0 +1,358 @@
   +      $form['actions']['submit']['#submit'][] = '\Drupal\content_moderation\EntityTypeInfo::bundleFormRedirect';
   

   Why can't we use ['class', 'method'] here?

7. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -0,0 +1,358 @@
   +   * @param array $form
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   

   Desc is missing.

8. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -0,0 +1,358 @@
   +    $moderation_info = \Drupal::getContainer()->get('content_moderation.moderation_information');
   

   This should be injected.

9. +++ b/core/modules/content_moderation/src/Event/ContentModerationTransitionEvent.php
   @@ -0,0 +1,69 @@
   +class ContentModerationTransitionEvent extends Event {
   

   Class doc missing.

10. +++ b/core/modules/content_moderation/src/Event/ContentModerationTransitionEvent.php
    @@ -0,0 +1,69 @@
    +  protected $stateBefore;
    ...
    +  protected $stateAfter;
    

    Desc block is missing for vars.

11. +++ b/core/modules/content_moderation/src/Event/ContentModerationTransitionEvent.php
    @@ -0,0 +1,69 @@
    +  public function getStateBefore() {
    ...
    +  public function getStateAfter() {
    

    Function doc block is missing.

12. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,191 @@
    +  protected $entityTypeManager;
    

    Var desc is missing.

13. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,191 @@
    +   * @inheritDoc
    ...
    +   * @inheritDoc
    

    This is not right.

14. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,191 @@
    +   * @todo I don't know why this needs to be separate from the form() method.
    

    Who is I? Crell?

15. +++ b/core/modules/content_moderation/src/Form/ModerationStateForm.php
    @@ -0,0 +1,85 @@
    + * @package Drupal\content_moderation\Form
    

    I think we don't use that.

16. +++ b/core/modules/content_moderation/src/Form/ModerationStateForm.php
    @@ -0,0 +1,85 @@
    +class ModerationStateForm extends EntityForm {
    +  /**
    

    Space missing.

17. +++ b/core/modules/content_moderation/src/Form/ModerationStateForm.php
    @@ -0,0 +1,85 @@
    +        'exists' => '\Drupal\content_moderation\Entity\ModerationState::load',
    
    +++ b/core/modules/content_moderation/src/Form/ModerationStateTransitionForm.php
    @@ -0,0 +1,141 @@
    +        'exists' => '\Drupal\content_moderation\Entity\ModerationStateTransition::load',
    

    Why can't we use ['class', 'method'] here?

18. +++ b/core/modules/content_moderation/src/Form/ModerationStateForm.php
    @@ -0,0 +1,85 @@
    +      // @todo When these are added, the checkbox default value does not apply properly.
    

    More then 80 chars.

19. +++ b/core/modules/content_moderation/src/Form/ModerationStateForm.php
    @@ -0,0 +1,85 @@
    +      // '#states' => [
    +      //   'checked' => [':input[name="published"]' => ['checked' => TRUE]],
    +      //   'disabled' => [':input[name="published"]' => ['checked' => TRUE]],
    +      // ],
    

    Commented code.

20. +++ b/core/modules/content_moderation/src/Form/ModerationStateTransitionDeleteForm.php
    @@ -0,0 +1,51 @@
    +        [
    +          '%label' => $this->entity->label()
    +        ]
    +        )
    

    Wrong indentation.

21. +++ b/core/modules/content_moderation/src/Form/ModerationStateTransitionForm.php
    @@ -0,0 +1,141 @@
    +  protected $entityTypeManager;
    ...
    +  protected $queryFactory;
    

    Doc block missing.

22. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -0,0 +1,216 @@
    + * @todo Much of this code may eventually migrate to the Entity module, and
    + * from there to Drupal core.
    

    I think we are moving to core now.

23. +++ b/core/modules/content_moderation/src/ModerationStateTransitionListBuilder.php
    @@ -0,0 +1,91 @@
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $transition_storage
    +   * @param \Drupal\Core\Entity\EntityStorageInterface $state_storage
    

    Desc line missing.

24. +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
    @@ -0,0 +1,103 @@
    +  protected $moderationInformation;
    

    Desc is missing.

25. +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
    @@ -0,0 +1,103 @@
    +   * @todo: If the parent class is ever cleaned up to use EntityTypeManager
    +   * instead of Entity manager, this method will also need to be adjusted.
    

    Can we do this now? Or at least have an issue.

26. +++ b/core/modules/content_moderation/src/Permissions.php
    @@ -0,0 +1,45 @@
    +    // @todo write a test for this.
    

    Link please.

27. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublishNode.php
    @@ -0,0 +1,60 @@
    + * @see PublishNode
    
    +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublishNode.php
    @@ -0,0 +1,60 @@
    + * @see UnpublishNode
    

    FQDN please.

28. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublishNode.php
    @@ -0,0 +1,60 @@
    +  protected $moderationInfo;
    
    +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublishNode.php
    @@ -0,0 +1,60 @@
    +  protected $moderationInfo;
    

    Desc is missing.

29. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublishNode.php
    @@ -0,0 +1,60 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ModerationInformationInterface $mod_info) {
    
    +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublishNode.php
    @@ -0,0 +1,60 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ModerationInformationInterface $mod_info) {
    

    Doc block is missing.

30. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -0,0 +1,267 @@
    +  protected $moderationInformation;
    ...
    +  protected $moderationStateTransitionStorage;
    ...
    +  protected $validator;
    

    Desc is missing.

31. +++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
    @@ -0,0 +1,104 @@
    +    // @todo write a test for this.
    ...
    +    // @todo write a test for this.
    

    Links please.

32. +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationState.php
    @@ -0,0 +1,21 @@
    + * Dynamic Entity Reference valid reference constraint.
    

    Sorry what now?

33. +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateValidator.php
    @@ -0,0 +1,113 @@
    +  public static function create(ContainerInterface $container) {
    

    Doc block is missing.

34. +++ b/core/modules/content_moderation/src/Plugin/views/filter/LatestRevision.php
    @@ -0,0 +1,121 @@
    +  protected $entityTypeManager;
    ...
    +  protected $joinHandler;
    ...
    +  protected $connection;
    ...
    +   * @param array $configuration
    +   * @param string $plugin_id
    +   * @param mixed $plugin_definition
    +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
    +   * @param \Drupal\views\Plugin\ViewsHandlerManager $join_handler
    +   * @param \Drupal\Core\Database\Connection $connection
    

    Desc is missing.

35. +++ b/core/modules/content_moderation/src/Routing/EntityModerationRouteProvider.php
    @@ -0,0 +1,117 @@
    +          $entity_type_id => ['type' => 'entity:' . $entity_type_id, 'load_forward_revision' => 1],
    

    This should be multi line.

36. +++ b/core/modules/content_moderation/src/Routing/EntityTypeModerationRouteProvider.php
    @@ -0,0 +1,58 @@
    +          //'_title_callback' => '\Drupal\Core\Entity\Controller\EntityController::editTitle'
    

    Commented code.

Going to tackle all 36 issues @jibran awesomely brought up. I naively thought that as an experimental module we could just drop in Workbench Moderation as Content Moderation.

I will work with the Workflow Initiative team and Workbench Moderation maintainers to make sure we have suitable subsystem maintainers.

Can someone please clarify what kind of feedback one is allowed to give? I'm asking about experimental modules in general.

I think what we're still missing (partly because we haven't done it yet) is the process of getting from experimental to stable release. i.e. we don't have a workflow for reviewing thousands of lines of code that works at all.

That could probably use a separate issue, or there might be a plan issue open covering some of it already.

#32.8. Is a static function so can't use the service property.

I think I have covered all of the items, apart from some of the creating new issues for todos.

Probably should contact wbm maintainers to ask if they want to be listed in maintainers.txt, given its their work.

What happens to sites with wbm enabled? Two modules defining the same base field - do you end up with a field mismatch.

What happens to existing data if you disable wbm to enable this module? Do you lose all your existing data.

We need tests for that.

So therefore it probably would make sense to keep any migration logic and tests for that in WBM itself.

We need to at least test (even manually) what happens when we have two modules defining the same base field definition. We might need one of the two to check for the presence of each other before changing field definitions.

Tim and I have reached out to both agentrickard and Crell, and they both said they won't be able to do any day-to-day maintenance of the module and could therefore not be counted on as maintainers

I'd be happy to pitch in too, given a large amount of the module was adapted from my code and I'm keeping an active eye on the D8 branch.

Sorry what now?

Ha ha, copy pasted from DER into moderation_state, into workbench_moderation and now into content_moderation.
Love it.

To get maximum use out of this module I really think we need #2702041: Add views relationship from content to latest revision in this patch too. I'm going to commit it to wbm tomorrow.

The latest conversations that we had here at Dev Days (I will be posting the notes very soon) is that we should remove the UI for adding and managing state transitions from the experimental module in core.

This fills me with dread. We've tried to put "junior versions" of contrib in core before. It's always been a disaster. What's the reasoning for this? Sure, those UIs could be improved (what can't), but there's no reason to split the module in half while doing so. A Content Moderation UI module sounds like an awful idea.

Also, for the record, Dixon accurately summarizes my (lack of) maintenance capacity right now.

This fills me with dread. We've tried to put "junior versions" of contrib in core before. It's always been a disaster. What's the reasoning for this? Sure, those UIs could be improved (what can't), but there's no reason to split the module in half while doing so. A Content Moderation UI module sounds like an awful idea.

Well, its the idea of an experimental module to not be complete. The idea is to get a simple version in, and then iterate on top of it. Its not meant to be used as stable module, or for example would be enabled by default.

At the same time we try to limit the amount of additional technical and product debt by keeping the UI as simple as possible, in this case, by dropping the less important part of the module, aka. the states and state transition UI. There is nothing which blocks you from creating custom states via YML files. The module though for itself is already helpful as the common default set is quite useful already.

How does it increase tech debt to include a mostly-Drupal-default UI that we can then iterate on in core? All I can see this doing is creating more work and possibly ending up with a less-useful module in core at the end. An experimental module means the UI is subject to change, too, so we aren't committing to anything. Experimental or not, I see no wins in this approach.

@larowlan - I'll happily add you to MAINTAINERS.txt in the patch.

@dixon_ @dawehner - I agree with @Crell. I think updating this patch to remove the UI, then creating WBM 8.x-2.x with only the UI will take a lot of time. This time would be better placed just updating the UI. At the moment we're not 100% sure what that UI might be so let's just get WBM in "as is" then iterate.

I agree with Tim and Larry, above. Removing the UI and putting that in contrib seems like a poor use of time and a burden on maintainers.

Left a novella review of this patch over at #2755073: WI: Content Moderation module roadmap (spoiler alert: I agree with the last few comments), since it seemed more high-level about the scope of the broader sub-initiative.

In there I also suggested a couple of commit-blockers: 1) reducing the list of default states/transitions and 2) adding some means of listing content that's in a workflow state. I don't want to split discussion, nor create more work for people. Is the best thing to do mark this postponed until #2755073: WI: Content Moderation module roadmap is resolved and discuss the "spec" over there, or should we discuss it here, or..?

The only relevant technical piece from the review is that for some reason the "Content Moderation" menu item is not showing up under admin/structure. Probably something silly that's still not quite renamed properly yet.

adding some means of listing content that's in a workflow state

that's #2702041: Add views relationship from content to latest revision which we need to adapt here too

#2702041: Add views relationship from content to latest revision and #2708941: Allow roles to view but not administer moderation state are now in workbench_moderation in contrib

Sweet!! :)

A small note, we should be moving this to /config after commit. We have a actual category for this in Config designated especially for this type of configuration - it just took 6 years to use it :)

Patch attached:

1. Makes the code compliant with our coding standards - we can't commit code that does not pass our phpcs.xml.dist rules
2. Fixes a few wrongly referenced classes - FQDNs helps developers and automatic code review
3. Removes ModerationStateTransitionStorage the whole dance to use setModerationStateConfigPrefix is not required - see changes to ModerationStateTransition::calculateDependencies() - reduces unnecessary public API
4. Renames the config entities:
   1. content_moderation.moderation_state.ID to content_moderation.state.ID
   2. content_moderation.moderation_state_transition.ID to content_moderation.state_transition.ID

   This is harder to change once in core. The second 'moderation' is just repetition... node types are node.type.ID not node.node_type.ID

• Added contrib patches from #50.
• Added @larowlan to MAINTAINERS.txt.
• Updated admin paths to /admin/config/workflow, rather than /admin/structure.

Added a new subissue to not let the module workaround the missing common status interface: #2757055: Add a EntityStatusInterface

Patched does the following:

1. Applies #2708941: Allow roles to view but not administer moderation state
2. Applies #2702041: Add views relationship from content to latest revision - interestingly the test added by this patch was not running and needed fixing - issue on the original project coming soon
3. Fixes incorrect link templates
4. Removes unnecessary services.After discussion with @catch, @dawhener and @timmillwood in IRC we decided to remove services for the service container that are not meant to be swapped out. They are more like event listeners listening for hooks - we can just replace these with new Class(). In the case of InlineEditingDisabler this functionality was just moved into the .module file since the class / service are just an unnecessary layer.
5. Adds @larowlan to MAINTAINERS.txt for the module

The first interdiff is relative to #53.

We should open a side issue for the moderation state tracking.

Since this is a field on entities, it would be easy enough via the API to make a forward/older revision in a non-published moderation state the default revision, without updating the moderation state, creating a mis-match. Not sure what/if the answer to that is but worth discussing on its own.

Moved paths from admin/structure/content-moderation to admin/config/workflow/moderation

Just a quick note that the bug I noticed in the last version I tested with the menu item not being visible is fixed now. :)

Have been looking through the code a bit, feedback below. The most important thing is the first one, that's my biggest concern. The second is that I don't see the capability to uninstall module again as far as I see. Seems like a one-way street right now, which seems especially problematic given this is an experimental module where we don't provide an upgrade path (I suppose?) and people might be required to uninstall it again? Unlike our existing experimental module, this isn't nicely separated and removeable like migrate or bigpipe but *deeply* integrated into the content. I'm expecting some interesting challlenges there. But I suppose that was already discussed.

The third point (which just like the second is not mentioned below anymore) is that I didn't see any logic for entity types that are added after this module was installed, but maybe I missed that, it is a lot of code ;)

The other points are mostly smaller things, but I was looking through the code anyway and noticed them, we can still fix them later if not important enough for now.

1. +++ b/core/modules/content_moderation/content_moderation.install
   @@ -0,0 +1,42 @@
   +
   +  /** @var \Drupal\Core\Entity\ContentEntityTypeInterface $type */
   +  foreach ($revisionable_entity_definitions as $type) {
   +    $content_moderation_definition = $field_manager->getFieldStorageDefinitions($type->id())['moderation_state'];
   +    $entity_definition_update_manager->installFieldStorageDefinition('moderation_state', $type->id(), 'moderation_state', $content_moderation_definition);
   +  }
   

   so all revisionable entity types with a bundle get a moderation state?

   I don't really like this. There are many examples for entity types where this really makes no sense. Take paragraphs, field collection and similar "composite entity types" which only exist in the context of e.g. a node. They have no reason for having their own revision handling/moderation, each node revision also has corresponding paragraph revisions and they're managed as a whole.

   Users might get revisionable soon and while they don't have a bundle and are therefore currently excluded from this (why? custom entity types might not have bundles but still want to use content moderation, we could also support the settings we need as entity type annotations for example) are another example for an entity type that has revisions but doesn't need moderation IMHO.

   I think as a minimum, we should support an annotation key so that certain entity types can opt out of this. or even make this configuration so that users can decide what content they want to have moderated. Like content_translation does.

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   +  if ($hook === 'entity_view_alter') {
   +    // Find the quickedit implementation and move content after it.
   +    unset($implementations['content_moderation']);
   +    $implementations['content_moderation'] = FALSE;
   +  }
   

   that's not what the code is doing?

3. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   +  $moderation_information = \Drupal::service('content_moderation.moderation_information');
   +  if ($moderation_information->isModeratableEntity($entity) && !$moderation_information->isLatestRevision($entity)) {
   +    // Hide quickedit, because its super confusing for the user to not edit the
   +    // live revision.
   +    unset($build['#attributes']['data-quickedit-entity-id']);
   +  }
   

   is it worth checking if quickedit is enabled before those checks? unset() doesn't result in notices, no idea how expensive those checks are.

4. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   + */
   +function content_moderation_entity_bundle_field_info_alter(&$fields, EntityTypeInterface $entity_type, $bundle) {
   +  _content_moderation_create_entity_type_info()->entityBundleFieldInfoAlter($fields, $entity_type, $bundle);
   +}
   

   don't really understand the pattern with that create helper function (why not a service?)

5. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   + */
   +function content_moderation_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   +  _content_moderation_create_entity_type_info()->bundleFormAlter($form, $form_state, $form_id);
   +}
   

   worth checking if $form_state->getFormObject() is actually an entity form before calling this?

6. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   +
   +  /** @var \Drupal\Core\Entity\EntityTypeManagerInterface $etm */
   +  $etm = \Drupal::service('entity_type.manager');
   +
   

   we usually don't shorten variables like this.

7. +++ b/core/modules/content_moderation/content_moderation.permissions.yml
   @@ -0,0 +1,24 @@
   +view any unpublished content:
   +  title: 'View any unpublished content'
   +  description: 'This permission is necessary for any users that may moderate content.'
   +
   

   For this to actually work, we'd need to implement the node grants system, so that e.g. admin/content would shown unpublished content. How are we going to deal with that?

8. +++ b/core/modules/content_moderation/content_moderation.views.inc
   @@ -0,0 +1,21 @@
   + * Implements hook_views_data().
   + */
   +function content_moderation_views_data() {
   +  $views_data = new ViewsData(
   

   why is the alter hook not also in this file?

9. +++ b/core/modules/content_moderation/src/Access/LatestRevisionCheck.php
   @@ -0,0 +1,82 @@
   +    // @todo Do we need any extra cache tags here?
   +    $entity = $this->loadEntity($route, $route_match);
   +    return $this->moderationInfo->hasForwardRevision($entity)
   +      ? AccessResult::allowed()->addCacheableDependency($entity)
   +      : AccessResult::forbidden()->addCacheableDependency($entity);
   

   we had some serious performance issues initially in 8.x because the query to check if a node had revisions was extremely slow due to missing indexes. has this been checked here? (I understand performance is not a concern for an experimental module apparently, but sooner or later, we'll have to figure this out)

10. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -0,0 +1,84 @@
    +
    +    return;
    +  }
    

    also not something we usually do IMHO

11. +++ b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
    @@ -0,0 +1,63 @@
    +      parent::onPresave($entity, $default_revision, $published_state);
    +      // Only nodes have a concept of published.
    +      /** @var \Drupal\node\NodeInterface $entity */
    +      $entity->setPublished($published_state);
    

    The method yes, but an entity key status is more or less a unified concept although not widely used in content entities. Could be done based on that.

12. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -0,0 +1,276 @@
    +    // @todo make this more functional, less iterative.
    +    // https://www.drupal.org/node/2755099
    +    foreach ($to_check as $entity) {
    

    $to_check is a strange variable name for an entity.

13. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -0,0 +1,276 @@
    +    /** ContentEntityInterface $entity */
    

    that's not how this works :)

    Keeping the initial check outside of this method would allow to type hint on what we actually require.

14. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +  protected function addModerationToEntity(ContentEntityTypeInterface $type) {
    ...
    +  protected function addModerationToEntityType(ConfigEntityTypeInterface $type) {
    

    those method names make no sense to me. both are entity types, one is content and one config.

15. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +    // @todo Core forgot to add a direct way to manipulate route_provider, so
    +    // we have to do it the sloppy way for now.
    

    All those "stupid core forgot to do this and that" are a bit weird now that this is in core (or will be) ;)

16. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +      ->setTargetEntityTypeId($entity_type->id())
    

    shouldn't be necessary.

17. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +      ->setDisplayOptions('view', [
    +        'label' => 'hidden',
    +        'type' => 'hidden',
    +        'weight' => -5,
    +      ])
    

    I don't think this does anything, not being exposed is the default behavior

18. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +      ->setDisplayConfigurable('form', FALSE)
    +      ->setDisplayConfigurable('view', FALSE);
    

    same here.

19. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +  /**
    +   * Force moderatable bundles to have a moderation_state field.
    +   *
    +   * @param \Drupal\Core\Field\FieldDefinitionInterface[] $fields
    +   *   The array of bundle field definitions.
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    +   *   The entity type definition.
    +   * @param string $bundle
    +   *   The bundle.
    +   *
    +   * @see hook_entity_bundle_field_info_alter();
    +   */
    +  public function entityBundleFieldInfoAlter(&$fields, EntityTypeInterface $entity_type, $bundle) {
    +    if ($this->moderationInfo->isModeratableBundle($entity_type, $bundle) && !empty($fields['moderation_state'])) {
    +      $fields['moderation_state']->addConstraint('ModerationState', []);
    +    }
    +  }
    

    code and description doesn't match? also isn't this constraint always added above?

20. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,190 @@
    +   * {@inheritdoc}
    +   *
    +   * We need to blank out the base form ID so that poorly written form alters
    +   * that use the base form ID to target both add and edit forms don't pick
    +   * up our form. This should be fixed in core.
    

    fixed how and why? That's the point of having that.. also not valid to add docs to @inheritdoc

21. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,190 @@
    +    $options = [
    +      $this->t('Unpublished')->render() => $options_unpublished,
    +      $this->t('Published')->render() => $options_published,
    +    ];
    +
    

    usually we just use string casting for this I think. We also do this in lots of places without extensive comments, so not sur it is necessary here.

22. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,190 @@
    +   * Form builder callback.
    +   *
    +   * @todo This should be folded into the form method.
    +   *
    

    don't understand this todo, this works as defined?

23. +++ b/core/modules/content_moderation/src/Form/ModerationStateTransitionDeleteForm.php
    @@ -0,0 +1,51 @@
    +/**
    + * Builds the form to delete Moderation state transition entities.
    + */
    +class ModerationStateTransitionDeleteForm extends EntityConfirmFormBase {
    

    doesn't seem to do anything special, could use the default delete from?

24. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -0,0 +1,213 @@
    +   * {@inheritdoc}
    +   */
    +  public function selectRevisionableEntityTypes(array $entity_types) {
    +    return array_filter($entity_types, function (EntityTypeInterface $type) use ($entity_types) {
    ...
    +   */
    +  public function selectRevisionableEntities(array $entity_types) {
    +    return array_filter($entity_types, function (EntityTypeInterface $type) use ($entity_types) {
    +      return ($type instanceof ContentEntityTypeInterface)
    

    the same entity type vs entity naming that still doens't make sense to me :)

25. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -0,0 +1,266 @@
    +      $container->get('entity_type.manager'),
    +      $container->get('entity_type.manager')->getStorage('moderation_state'),
    +      $container->get('entity_type.manager')->getStorage('moderation_state_transition'),
    +      $container->get('entity.query')->get('moderation_state_transition', 'AND'),
    

    an entity query object is not re-usable, it's not a good idea to inject that.

    You can also do getQuery() on the storage if you have that already, then you can avoid this additional argument completely

26. +++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
    @@ -0,0 +1,104 @@
    +/**
    + * Defines a class for making the edit tab use 'Edit draft' or 'New draft'
    + */
    +class EditTab extends LocalTaskDefault implements ContainerFactoryPluginInterface {
    +
    +  use StringTranslationTrait;
    

    strange name as we call this LocalTask in code, not tab?

+++ b/core/modules/content_moderation/src/Entity/ModerationStateTransition.php
@@ -72,23 +71,16 @@ class ModerationStateTransition extends ConfigEntityBase implements ModerationSt
-      $this->addDependency('config', $prefix . $this->stateFrom);
+      $this->addDependency('config', ModerationState::load($this->stateFrom)->getConfigDependencyName());

Thanks @alexpott, this was my code back from moderation_state module, nice to know there's a better way

I reviewed this code from the POV of Quick Edit, cacheability and access control in particular, and consistency/maintainability in general.

I refrained from posting nitpicks, of which I could easily post 100 or so. Lots of code formatting problems.

1. This is a lot of code.

2. +++ b/core/modules/content_moderation/config/install/content_moderation.state_transition.needs_review_draft.yml
   @@ -0,0 +1,11 @@
   +stateFrom: needs_review
   

   I think this is the first time we would use camelCase in config.

   Do we want that?

   (Grepping the codebase for the regexp [a-z]+[A-Z]+: on all *.yml files confirms what I said.)

3. +++ b/core/modules/content_moderation/content_moderation.install
   @@ -0,0 +1,42 @@
   +  // An intermittent issue remains where calling
   +  // ModerationInformation:selectRevisionableEntities in hook_install
   +  // returns a different result than calling it in hook_entity_type_alter.
   +  // The result is that the moderation_state field is null, and thus trying to
   +  // install a field with a null definition explodes (rightly so).
   +  // Until that oddity is sorted out, we can at least put an extra check in
   +  // here to filter out such broken entities.
   +  // @todo Remove when the underlying bug is fixed.
   +  // @see https://www.drupal.org/node/2674446
   +  $revisionable_entity_definitions = array_filter($revisionable_entity_definitions, function(ContentEntityTypeInterface $type) use ($field_manager) {
   +    return !empty($field_manager->getFieldStorageDefinitions($type->id())['moderation_state']);
   +  });
   

   This sounds scary.

4. +++ b/core/modules/content_moderation/content_moderation.libraries.yml
   @@ -0,0 +1,5 @@
   +  version: 1.x
   

   This asset library version seems wrong.

5. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   + * @todo How to remove the live version (i.e. published => draft without new
   + *   revision) - i.e. unpublish
   

   Should link to an issue.

   (Quite a few todos should.)

6. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   +      $output .= '<p>' . t('The Content Moderation module provides basic moderation for content. For more information, see the <a href=":content_moderation">online documentation for the Content Moderation module</a>.', array(':content_moderation' => 'https://www.drupal.org/documentation/modules/workbench_moderation/')) . '</p>';
   

   This still links to the workbench_moderation docs. Perhaps that page can be reused, but then its alias should be updated.

7. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   +    // Find the quickedit implementation and move content after it.
   +    unset($implementations['content_moderation']);
   +    $implementations['content_moderation'] = FALSE;
   

   The comment does not match the logic.

8. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   +  if ($moderation_information->isModeratableEntity($entity) && !$moderation_information->isLatestRevision($entity)) {
   +    // Hide quickedit, because its super confusing for the user to not edit the
   +    // live revision.
   +    unset($build['#attributes']['data-quickedit-entity-id']);
   +  }
   

   1. We need a major (non-critical) follow-up issue to evaluate Quick Edit + Content Moderation: what should work, how should it work, et cetera.
   2. Hereby confirming that this is otherwise okay for now. Particularly note that since moderation state is a field on the entity, that means that changing the moderation state means changing the entity, which means the entity's cache tag will be invalidated, which means that render cache items of this entity will be invalidated, which means this will be called again (i.e. the entity will be re-rendered) after each moderation state change.

9. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,275 @@
   + * Implements hook_entity_insert().
   ...
   + * Implements hook_entity_update().
   

   Do we have test coverage for this, also in relation to translations & revisions?

   Because editor module also implements these hooks and has several bugs despite test coverage (boo @ me!), so I just want to warn you about that because these hooks do work in unexpected ways.

   See #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations + #2729953: Clarify handling of multiple references to the same file from entities. (I know, not exactly the same, but IMO good to double-check those aspects.)

10. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -0,0 +1,275 @@
    +  if ($operation == 'view') {
    +    return (!$entity->isPublished())
    +      ? AccessResult::allowedIfHasPermission($account, 'view any unpublished content')
    +      : AccessResult::neutral();
    +  }
    +  elseif ($operation == 'update' && $moderation_info->isModeratableEntity($entity) && $entity->moderation_information && $entity->moderation_information->target_id) {
    +    /** @var \Drupal\content_moderation\StateTransitionValidation $transition_validation */
    +    $transition_validation = \Drupal::service('content_moderation.state_transition_validation');
    +
    +    return $transition_validation->getValidTransitionTargets($entity, $account)
    +      ? AccessResult::neutral()
    +      : AccessResult::forbidden();
    +  }
    

    Cacheability metadata here is wrong.

    1. In the view case, you're depending on the entity, but the entity's cacheability metadata is not added to the access result.
    2. In the other case, the result depends on getValidTransitionTargets(), and it apparently depends on the Entity and User objects (both of which have cache tags), and I suspect it further depends on configuration (which also has cache tags). Yet despite so many things that could change (Entity/User/Config) being involved in deciding what the access result is, there is no cacheability metadata whatsoever. That cannot be right.

    Finally, let's use strict comparisons.

11. +++ b/core/modules/content_moderation/content_moderation.routing.yml
    @@ -0,0 +1,89 @@
    +    _admin_route: TRUE
    ...
    +    _admin_route: TRUE
    ...
    +    _admin_route: TRUE
    ...
    +    _admin_route: TRUE
    ...
    +    _admin_route: TRUE
    ...
    +    _admin_route: TRUE
    

    You only need to specify this route option on routes that have a path that does not begin with /admin.

12. +++ b/core/modules/content_moderation/src/Access/LatestRevisionCheck.php
    @@ -0,0 +1,82 @@
    +    // This tab should not show up period unless there's a reason to show it.
    +    // @todo Do we need any extra cache tags here?
    +    $entity = $this->loadEntity($route, $route_match);
    +    return $this->moderationInfo->hasForwardRevision($entity)
    +      ? AccessResult::allowed()->addCacheableDependency($entity)
    +      : AccessResult::forbidden()->addCacheableDependency($entity);
    

    This looks much better.

    For the @todo: AFAICT ModerationInfo does not look at any configuration, it only looks at the data stored in the entity. Therefore no additional cache tags are necessary.

13. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -0,0 +1,275 @@
    +/**
    + * Implements hook_preprocess_HOOK().
    + *
    + * Many default node templates rely on $page to determine whether to output the
    + * node title as part of the node content.
    + */
    +function content_moderation_preprocess_node(&$variables) {
    +  $content_process = new ContentPreprocess(\Drupal::routeMatch());
    +  $content_process->preprocessNode($variables);
    +}
    

    +++ b/core/modules/content_moderation/src/ContentPreprocess.php
    @@ -0,0 +1,57 @@
    +  public function preprocessNode(array &$variables) {
    +    // Set the 'page' template variable when the node is being displayed on the
    +    // "Latest version" tab provided by content_moderation.
    +    $variables['page'] = $variables['page'] || $this->isLatestVersionPage($variables['node']);
    +  }
    

    Eh… this looks bizarre. But probably a necessary evil.

14. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -0,0 +1,84 @@
    +  public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {
    +    // This is *probably* not necessary if configuration is setup correctly,
    +    // but it can't hurt.
    +    $entity->setNewRevision(TRUE);
    +    $entity->isDefaultRevision($default_revision);
    +  }
    

    +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +    // @todo write a test for this.
    ...
    +      // @todo write a test for this.
    
    +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,190 @@
    +    // @todo write a test for this.
    
    +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -0,0 +1,266 @@
    +    // @todo write a test for this.
    

    This does not inspire confidence, and suggests a lack of test coverage.

15. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +        'title' => t('Manage moderation'),
    +        'weight' => 27,
    

    $this->t()

    And that weight seems… very specific. It should be documented why that particular weight was chosen.

16. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,360 @@
    +   * @return \Generator
    ...
    +      foreach ($result as $bundle_name) {
    +        yield ['entity' => $type->getBundleOf(), 'bundle' => $bundle_name];
    +      }
    

    Fancy! :)

    This will be the first use of yield in Drupal 8.

#62 feedback:

We should probably work on an uninstaller before 8.2.x as a priority.

1. Yes, all revisionable entity types with a bundle get a moderation state. However being moderatable is disabled by default and needs to be enabled within the bundle settings. It was discussed about making revisionable entity types without bundles moderatable, but where do you add the settings? It was a simpler option to just add it for bundlable entity types. Maybe adding an opt-out annotation would be good.

4. They used to be services, but being standard classes makes it simpler, doesn't add unnecessary services, and doesn't give them impression these services can or should be swappable.

All other items look to be things that should be addressed, but are they commit blocking? I feel many of them would be easier to address as sub-issues rather than keep re-rolling this already unwieldy patch.

#64 feedback:

1. yes it is, let's just get it committed, then fix all these items in sub-issues before 8.2.x release.

3. is it? We're just adding a field to all revisionable & bundlable entity types.

6. yes, this is good enough to make core help tests pass, but we should update the docs before 8.2.x release.

Re uninstalling: Yeah, that's a problem. The issue is that:

1) Core doesn't let you uninstall a module that provides a field if that field is in use. (Reasonable)
2) In use includes on a non-current revision. (Reasonable)
3) This module forces the use of revisions, which therefore means lots of old revisions. (Obviously)
4) Those old revisions have the moderation_state field on them, provided by this module. (Obviously)
5) You can't edit an old revision. (Obviously)
6) So... as long as those old revisions exist, and have a moderation_state field, which they will always have, the field is always in use, and so the module rightly cannot be uninstalled! (Crap)

For WBM, we punted and decided we didn't want to deal with that for 1.0. The only way to cleanly make the module uninstallable is a cleanup script of some kind that disables moderation for any entity/bundle that's using it, then saves a new revision without that field on it, then deletes all old revisions of any previously-moderated entities. At that point the field usage should be purged, so Drupal should allow the module to be uninstalled.

If someone knows a better way than that, I'd love to here it because that was a thorn in our side while developing WBM. :-(

Incidentally, I am reasonably sure (although not 100%) that the module does not add the moderation state field to all entities unconditionally; it just adds them to those where it's been enabled. I may be wrong on that, though, but I think that's the case. (That part of the code dates to larwolan's original and I didn't 100% grok it, to be honest.)

Wim: That was like my 3rd attempt to have a legitimate "yield" in the module. :-) Yay for more advanced language toys in core!

Berdir: A number of those hooks are services in WBM. Apparently alexpott wanted them taken out and done the Drupal 7 way (per #57). I do not know why, and disagree strongly with that position. "Why would you swap it?" is a poor reason to avoid a service.

@Crell no interfaces and only ever located through \Drupal::service() is why these shouldn't be services. If there really was a use case for it to be a service they'd have interfaces and be injected properly into something.

Note: moderation_state field is added to the database table before making a bundle moderatable.

alexpott: Half of those were bridging off of hooks to classes. We've been talking in another issue about making it possible to register services straight to hooks without having the function at all. That would allow "just drop the function and add a tag" upgrades. Adding an interface to them suits me fine.

Not being able to uninstall this needs to block the initial commit. We're OK with experimental modules themselves having serious bugs, or the upgrade path for them being uninstall then re-install, but the inability to uninstall prevents even that from happening. Even if people take our advice and don't use alpha-level experimental modules in production, it's reasonable to develop and un-launched site with them, and that un-launched site may want a canonical database. This is already covered in https://www.drupal.org/core/experimental#requirements

So marking needs work for that. We can work on that in #2757119: Data integrity and uninstall issues with moderation state field (which I'll re-title).

Should we use the PP-1 syntax for this issue then? To make it clear from the title how many blockers this issue has

I think it'd be wise to step away from maintaining this, realistically I'm spread too thin. Can my name be taken out in the next re-roll.
(Discussed this with @timmillwood on irc first).

There are quite a few folks putting effort into issues in the Workbench Moderation issue queue. However, there hasn't been a commit in over a month, so I'm wondering if that's due to the assumption that this will go in soon?

@jhedstrom no, I intend to keep committing stuff to wbm.

content moderation won't be for production sites (experimental) whereas wbm is.

Thanks for the clarification @larowlan.

I'd like to propose a potential alternative approach here.

Rather than trying to get the current workbench moderation module in as a single patch, in its current state, how about tagging crucial issues in the WBM module queue, and focus on those for say a month or so. This will a) get more eyes on those issues, and b) once they are resolved, this patch can go in sometime before 8.2.0 beta with minimal review of the giant patch, since issues will have been fleshed out individually in that queue. At some point after that, an upgrade/migration path in WBM can be made to the now identical core module, and existing sites can readily move to that.

Going even further, since the contrib namespace for content_moderation is currently listed as unsupported, that namespace could be taken over, and as issues are resolved in WBM, they could be immediately mirrored there. Then the final patch to core is even more readily reviewable. The added benefit there is that new sites could start to use that module instead, requiring no migration in the future from WBM.

Thought it was time for a quick progress patch.

Could you provide a summary of the changes?

@Wim Leers, most of the stuff that can be seen in the interdiff from #77 is the work from #2757119: Data integrity and uninstall issues with moderation state field, which changed the architecture of the module to store state transitions in a new content entity instead of in a field on the moderated entity.

Aside from that, the code has been cleaned up a lot all over the place and I also fixed most of your review points from #64. Here's an interdiff for that review, it also contains a few unrelated cleanups but it should be pretty easy to follow the fixed items.

Oops, forgot to attach the file :/

@Wim Leers also there's the commit history available at https://github.com/timmillwood/content_moderation/commits/master

Two major usability issues have been fixed, #2757349: WI: Deal with scalability issues in the UI and #2757275: Decide on default moderation states, so here's a new patch with interdiffs for both of them.

+++ b/src/ModerationStateTransitionListBuilder.php
@@ -68,12 +82,19 @@ class ModerationStateTransitionListBuilder extends DraggableListBuilder {
+    $allowed_roles = array_filter($this->roleStorage->loadMultiple(), function ($role) use ($transition_id) {
+      return $role->hasPermission('use ' . $transition_id . ' transition');
+    });
+
+    $allow_role_labels = [];
+    foreach ($allowed_roles as $role) {
+      $allow_role_labels[] = $role->label();
+    }

Just for the record, this is duplicating $allow_role_labels = user_role_names(FALSE, 'use ' . $transition_id . ' transition');

Updating patch with the latest commits from https://github.com/timmillwood/content_moderation/commits/master

Fixing the schema issue from #85.
Updating paramconverter to only load the latest revision if isRevisionTranslationAffected() as mentioned in #2766957: Forward revisions + translation UI can result in forked draft revisions.

Fixing tests, and updating the change in #87 to do isRevisionTranslationAffected() on the correct language.

Green, but failed???

Adding a test to test the translated forward revision issue in #2766957: Forward revisions + translation UI can result in forked draft revisions.

Passed second time around.

After working through #2766957: Forward revisions + translation UI can result in forked draft revisions in more detail, I am standing by #82 as my preferred solution.

The biggest blocker to this, #2688945: Allow removing a module's content entities prior to module uninstallation, has landed today.

IMHO we should be good to go with this!

It would be kinda great to have some sort of quick issue summary about the architectual design of the module or at least some differences between the contrib and core one.

Updated summary with high level changes:
- Replace moderation_state field with a computed field.
- Add ContentModerationState entity to link a specific entity revision to a moderation state.
- Updated views support for ContentModerationState.
- Add weights to moderation states
- Code clean up
- Coding standards

The core committers (@alexpott, @xjm, @Cottser, @catch, @effulgentsia) discussed this issue and agreed that this is a high-priority feature for the Drupal 8.2.0 release. We agreed to make it a beta target (that can be committed after the first beta but ideally before the second) in order to take the time to get this right, if it is ready within the beta phase.

Adding what I hope is the final patch to remove @larowlan from MAINTAINERS.txt as requested in #73.

As long as this passes tests I will then RTBC so we can commit before beta freeze.

We should give credit to more than those who provided patches here, an maybe even those who helped via #2766957: Forward revisions + translation UI can result in forked draft revisions too.

I'm struggling to recreate these failures locally.

Some observations

1. +++ b/core/modules/content_moderation/content_moderation.links.menu.yml
   @@ -0,0 +1,21 @@
   +# Moderation state menu items definition
   ...
   +# Moderation state transition menu items definition
   

   Nit: do we need these?

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,249 @@
   + * @todo include UI bits of https://www.drupal.org/node/2429153
   + * @todo How to remove the live version (i.e. published => draft without new
   + *   revision) - i.e. unpublish
   

   These are old comments from moderation_state.module

3. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,249 @@
   +      $output .= '<p>' . t('The Content Moderation module provides basic moderation for content. For more information, see the <a href=":content_moderation">online documentation for the Content Moderation module</a>.', array(':content_moderation' => 'https://www.drupal.org/documentation/modules/workbench_moderation/')) . '</p>';
   

   New url needed?

4. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,249 @@
   +  _content_moderation_create_entity_type_info()->entityOperation($entity);
   

   This runs once for each entity in a list. Which means we're creating several instances of the helper class. Could it be a service instead?

5. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,249 @@
   +  return new EntityOperations(
   

   If this were a service we'd only create one in the live cycle of a request. As it stands we create several just to save a single entity. One in presave, one in insert|update, so on.

6. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,249 @@
   +  _content_moderation_create_entity_operations()->entityView($build, $entity, $display, $view_mode);
   

   Creating one of these helper objects for every entity being viewed seems like overkill? Can it just be a service?

7. +++ b/core/modules/content_moderation/content_moderation.routing.yml
   @@ -0,0 +1,73 @@
   +# ModerationState routing definition
   ...
   +# ModerationStateTransition routing definition
   

   Do we normally comment these?

8. +++ b/core/modules/content_moderation/src/ContentModerationStateInterface.php
   @@ -0,0 +1,16 @@
   +interface ContentModerationStateInterface extends ContentEntityInterface, EntityOwnerInterface {
   

   Should we point to the code in the entity handler that uses this in its instanceof checks?

9. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -0,0 +1,179 @@
   +    // @todo Add constraint that enforces unique content_entity_type_id / content_entity_id
   +    // @todo Index on content_entity_type_id, content_entity_id and content_entity_revision_id
   

   Is this coming? I think the index would be a blocker. Also nit > 80 chars

10. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -0,0 +1,76 @@
    +    // The Revisions portion of Entity API is not uniformly applied or
    +    // consistent. Until that's fixed in core, we'll make a best-attempt to
    +    // apply it to the common entity patterns so as to avoid every entity type
    +    // needing to implement this method, although some will still need to do so
    +    // for now. This is the API that should be universal, but isn't yet.
    

    We're in core now, should we be referencing an issue to fix this rather than saying 'this is core's fault'?

11. +++ b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
    @@ -0,0 +1,66 @@
    +    // @todo clarify the first condition.
    

    Do we have an issue or is this no longer relevant?

12. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -0,0 +1,280 @@
    +      $moderation_state = ModerationState::load($entity->moderation_state->target_id);
    

    We have the entity type manager injected, should we be using the storage handler direct instead of the static method?

13. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -0,0 +1,280 @@
    +    // @todo what if $entity->moderation_state->target_id is null at this point?
    

    Do we have a test for this?

14. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,363 @@
    +    // @todo Core forgot to add a direct way to manipulate route_provider, so
    +    // we have to do it the sloppy way for now.
    ...
    +    // @todo Core forgot to add a direct way to manipulate route_provider, so
    +    // we have to do it the sloppy way for now.
    

    This needs to reference an actual issue or be removed, we can't blame core from core.

15. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,363 @@
    +        'weight' => 27,
    

    Should we document why 27 is needed? Should we make it a constant? Seems magic

16. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,363 @@
    +      // @todo write a custom widget/selection handler plugin instead of
    +      // manual filtering?
    

    This is an old comment from moderation_state.module, we have a custom widget now?

17. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,196 @@
    +   * We need to blank out the base form ID so that poorly written form alters
    +   * that use the base form ID to target both add and edit forms don't pick
    +   * up our form. This should be fixed in core.
    

    Same again, we should be linking to an issue instead of blaming

18. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,196 @@
    +    // This is screwy, but the key of the array needs to be a user-facing string
    +    // so we have to fully render the translatable string to a real string, or
    +    // else PHP chokes on an object used as an array key.
    

    The language here isn't consistent with core's commenting

19. +++ b/core/modules/content_moderation/src/Form/BundleModerationConfigurationForm.php
    @@ -0,0 +1,196 @@
    +   * @todo This should be folded into the form method.
    ...
    +    // @todo write a test for this.
    

    These are old moderation_state comments I think

20. +++ b/core/modules/content_moderation/src/Form/EntityModerationForm.php
    @@ -0,0 +1,161 @@
    +    // @todo should we just just be updating the content moderation state
    +    //   entity? That would prevent setting the revision log.
    

    Still open?

21. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -0,0 +1,213 @@
    +    // We really shouldn't be checking for a base class, but core lacks an
    +    // interface here. When core adds a better way to determine if we're on
    +    // a Bundle configuration form we should switch to that.
    

    We should add an issue for this and point to it here

22. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
    @@ -0,0 +1,249 @@
    +    // @todo write a test for this.
    

    An old comment from moderation_state.module

23. +++ b/core/modules/content_moderation/src/Plugin/Field/ModerationStateFieldItemList.php
    @@ -0,0 +1,82 @@
    +    if (!isset($this->list[$index]) || $this->list[$index]->isEmpty()) {
    +      $moderation_state = $this->getModerationState();
    +      // Do not store NULL values in the static cache.
    +      if ($moderation_state) {
    +        $this->list[$index] = $this->createItem($index, ['entity' => $moderation_state]);
    

    nice

24. +++ b/core/modules/content_moderation/src/Plugin/Menu/EditTab.php
    @@ -0,0 +1,104 @@
    +    // @todo write a test for this.
    ...
    +    // @todo write a test for this.
    

    More old moderation_state.module comments :)

25. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.info.yml
    @@ -0,0 +1,10 @@
    \ No newline at end of file
    

    nit

Fixing the fails. Will address #106 later.

EDIT: To explain my testbot fails and locally does not - #1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available - different ways of reading YAML. I wonder what the spec says about duplicate keys and which one is doing it right.

1. Seems harmless and breaks it into sections.
2. Agreed - removed.
3. I think we should have a followup to get this right once the module goes in. Removed the link to workbench_moderation since that's just wrong.
4. We moved away from services for responding to hooks since this pattern is not set in core yet. Let's consider our options once this patch lands.
5. See 4.
6. See 4
7. I think in this case they are useful - kinda like separators for the different sections
8. Not sure what you mean - we don't do that on any other entity interface
9. Removed the @todo's and added the necessary stuff to make the index
10. Let's fix this in a followup
11. I think this is all related to the critical #2766957: Forward revisions + translation UI can result in forked draft revisions - I think this should be dealt with post commit.
12. Sure why not.
13. I think this is a follow-up
14. I think this is a follow-up - to find/create issue and adjust comment
15. I think this is a follow-up
16. Removed
17. Adjusted the language to just explain.
18. Adjusted the language to just explain.
19. They still seem relevant - let's handle this in a followup
20. Yes still open and still very tricky.
21. Removed - what's in core is in core. That's what we have. If someone wants to file an issue so be it.
22. I think we should leave all the // @todo write a test for this. and file a plan to remove them and add (or determine if we already have) test coverage.
23. :)
24. See 22.
25. Fixed

So be it let's add a link in the the hook_help() implementation.

Think we're good to go.

Lots of follow up filed and still to file, but for now I think this is ready for commit to 8.2.x.

Trying to give this a full look over. I didn't get anywhere near finishing, posting this and I'll come back to it later. None of this is blocking and it's of varying levels of nitpicking.

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,245 @@
   +function content_moderation_help($route_name, RouteMatchInterface $route_match) {
   

   This should normally have a link to permissions and configuration, doesn't block initial commit but should block stability.

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,245 @@
   +  if ($moderation_information->isModeratableEntity($entity) && !$moderation_information->isLatestRevision($entity)) {
   +    // Hide quickedit, because its super confusing for the user to not edit the
   +    // live revision.
   +    unset($build['#attributes']['data-quickedit-entity-id']);
   +  }
   

   I'd rather see this handled in hook_requirements() or use #746752: New "conflicts" property for .info.yml extension files if we had it in core. Just hiding it could also lead to confusion if people expect it to be there.

3. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,245 @@
   +      ? AccessResult::allowedIfHasPermission($account, 'view any unpublished content')
   

   I can see it getting confusing between this new permissions, 'bypass node access' and 'administer nodes'. That's not entirely the fault of this patch, it's already confusing in there.

4. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -0,0 +1,245 @@
   +
   +  // The publish/unpublish actions are not valid on moderated entities. So swap
   +  // their implementations out for alternates that will become a no-op on a
   +  // moderated node. If another module has already swapped out those classes,
   +  // though, we'll be polite and do nothing.
   +  if (isset($definitions['node_publish_action']['class']) && $definitions['node_publish_action']['class'] == PublishNode::class) {
   +    $definitions['node_publish_action']['class'] = ModerationOptOutPublishNode::class;
   +  }
   +  if (isset($definitions['node_unpublish_action']['class']) && $definitions['node_unpublish_action']['class'] == UnpublishNode::class) {
   +    $definitions['node_unpublish_action']['class'] = ModerationOptOutUnpublishNode::class;
   +  }
   +}
   

   Could do with a followup to discuss the implications of this.

5. +++ b/core/modules/content_moderation/content_moderation.permissions.yml
   @@ -0,0 +1,24 @@
   +
   +'view moderation states':
   +  title: 'View moderation states'
   +  description: 'View moderation states.'
   +
   

   Why is this necessary as a separate permission?

6. +++ b/core/modules/content_moderation/content_moderation.permissions.yml
   @@ -0,0 +1,24 @@
   +
   +view latest version:
   +  title: 'View the latest version'
   +  description: 'View the latest version of an entity. (Also requires "View any unpublished content" permission)'
   +
   

   Why is this necessary vs view all revisions?

7. +++ b/core/modules/content_moderation/content_moderation.routing.yml
   @@ -0,0 +1,73 @@
   +entity.moderation_state.collection:
   

   Could we use the new auto-route for this?

8. +++ b/core/modules/content_moderation/content_moderation.views.inc
   @@ -0,0 +1,37 @@
   +function _content_moderation_views_data_object() {
   

   Not sure the 6 lines of duplicate code this saves is worth the 12 lines to define the function.

9. +++ b/core/modules/content_moderation/content_moderation.views.inc
   diff --git a/core/modules/content_moderation/css/entity-moderation-form.css b/core/modules/content_moderation/css/entity-moderation-form.css
   new file mode 100644
   
   new file mode 100644
   index 0000000..ec09407
   
   index 0000000..ec09407
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/content_moderation/css/entity-moderation-form.css
   
   +++ b/core/modules/content_moderation/css/entity-moderation-form.css
   +++ b/core/modules/content_moderation/css/entity-moderation-form.css
   @@ -0,0 +1,16 @@
   
   @@ -0,0 +1,16 @@
   +ul.entity-moderation-form {
   +  list-style: none;
   +  display: -webkit-flex; /* Safari */
   +  display: flex;
   +  -webkit-flex-wrap: wrap; /* Safari */
   +  flex-wrap:         wrap;
   +  -webkit-justify-content: space-around; /* Safari */
   +  justify-content:         space-around;
   +  -webkit-align-items: flex-end; /* Safari */
   +  align-items:         flex-end;
   +  border-bottom: 1px solid gray;
   +}
   +
   +ul.entity-moderation-form input[type=submit] {
   +  margin-bottom: 1.2em;
   +}
   

   Do we really need the CSS in the module?

10. +++ b/core/modules/content_moderation/src/Access/LatestRevisionCheck.php
    @@ -0,0 +1,85 @@
    +   *   The parametrized route.
    

    parameterized.

11. +++ b/core/modules/content_moderation/src/ContentModerationStateStorageSchema.php
    @@ -0,0 +1,29 @@
    +
    +    // Creates an index to ensure that the lookup in
    +    // \Drupal\content_moderation\Plugin\Field\ModerationStateFieldItemList::getModerationState()
    +    // is performant.
    +    $schema['content_moderation_state_field_data']['indexes'] += array(
    

    +1

12. +++ b/core/modules/content_moderation/src/ContentPreprocess.php
    @@ -0,0 +1,57 @@
    +  /**
    +   * Wrapper for hook_preprocess_HOOK().
    +   *
    +   * @param array $variables
    +   *   Theme variables to preprocess.
    +   */
    +  public function preprocessNode(array &$variables) {
    +    // Set the 'page' template variable when the node is being displayed on the
    +    // "Latest version" tab provided by content_moderation.
    +    $variables['page'] = $variables['page'] || $this->isLatestVersionPage($variables['node']);
    +  }
    

    This is probably going to run into #2528314: template_preprocess_node() does not add cacheability metadata. Not the fault of this patch.

13. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -0,0 +1,180 @@
    +
    +    // @todo Add constraint that enforces unique content_entity_type_id / content_entity_id
    +    // @todo Index on content_entity_type_id, content_entity_id and content_entity_revision_id
    +
    

    Could use a follow-up, - harder to do once we need an upgrade path.

14. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
    @@ -0,0 +1,180 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function save() {
    +    $related_entity = \Drupal::entityTypeManager()
    +      ->getStorage($this->content_entity_type_id->value)
    +      ->loadRevision($this->content_entity_revision_id->value);
    +    if ($related_entity instanceof TranslatableInterface) {
    +      $related_entity = $related_entity->getTranslation($this->activeLangcode);
    +    }
    +    $related_entity->moderation_state->target_id = $this->moderation_state->target_id;
    +    return $related_entity->save();
    +  }
    +
    +  /**
    +   * Saves an entity permanently.
    +   *
    +   * When saving existing entities, the entity is assumed to be complete,
    +   * partial updates of entities are not supported.
    +   *
    +   * @return int
    +   *   Either SAVED_NEW or SAVED_UPDATED, depending on the operation performed.
    +   *
    +   * @throws \Drupal\Core\Entity\EntityStorageException
    +   *   In case of failures an exception is thrown.
    +   */
    +  protected function realSave() {
    +    return parent::save();
    +  }
    +
    

    This is tricky and could use docs for why it's necessary.

15. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -0,0 +1,76 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {
    +    // This is *probably* not necessary if configuration is setup correctly,
    +    // but it can't hurt.
    +    $entity->setNewRevision(TRUE);
    +    $entity->isDefaultRevision($default_revision);
    +  }
    

    Won't it be necessary for progammatic saves?

16. +++ b/core/modules/content_moderation/src/Entity/Handler/NodeModerationHandler.php
    @@ -0,0 +1,66 @@
    +      $form['workflow']['options']['revision']['#disabled'] = TRUE;
    

    Minor, but I'd probably make this #access false to have less on the form.

17. +++ b/core/modules/content_moderation/src/Entity/ModerationState.php
    @@ -0,0 +1,97 @@
    + * Defines the Moderation state entity.
    

    These could use more high-level docs explaining what the entity does, or an @see to somewhere that does that.

18. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -0,0 +1,279 @@
    +   *
    +   * This method is only applicable when an entity is loaded that has
    +   * no moderation state on it, but should. In those cases, failing to set
    +   * one may result in NULL references elsewhere when other code tries to check
    +   * the moderation state of the entity.
    +   *
    

    How can you get into this situation? Shouldn't it throw an exception or at least trigger_error() if we get here?

19. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -0,0 +1,279 @@
    +    return $default_revision && $default_revision->moderation_state->entity->isPublishedState();
    

    This is where the lack of an entity status API bites us. I'd trust $node->published over the moderation state entity here.

20. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,361 @@
    +    foreach ($revisionable_types as $type_name => $type) {
    

    Could probably be array_walk() rather than foreach? (and below)

21. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -0,0 +1,361 @@
    +        yield ['entity' => $type->getBundleOf(), 'bundle' => $bundle_name];
    

    fancy.

I'm pretty sure that #2362435: When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be covers the case of #112.2 and therefore this code is unnecessary. I think removing it prior to commit might be a good thing.

And some more - again I think this is non-blocking.

The main-substantive question I have is about the transition storage. The split between state config entities + transition entities + third party settings for bundles seems a bit fragmented.

1. +++ b/core/modules/content_moderation/config/install/content_moderation.state_transition.draft_published.yml
   @@ -0,0 +1,11 @@
   diff --git a/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml b/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml
   
   diff --git a/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml b/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml
   new file mode 100644
   
   new file mode 100644
   index 0000000..f3a866a
   
   index 0000000..f3a866a
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml
   
   +++ b/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml
   +++ b/core/modules/content_moderation/config/install/content_moderation.state_transition.published_archived.yml
   @@ -0,0 +1,11 @@
   
   @@ -0,0 +1,11 @@
   +langcode: en
   +status: true
   +dependencies:
   +  config:
   +    - content_moderation.state.archived
   +    - content_moderation.state.published
   +id: published_archived
   +label: 'Archive'
   +stateFrom: published
   +stateTo: archived
   +weight: -6
   

   Also these transitions are global - what happens if a site wants different transitions (and states) for different entity types?

2. +++ b/core/modules/content_moderation/src/ContentPreprocess.php
   @@ -0,0 +1,57 @@
   +    $variables['page'] = $variables['page'] || $this->isLatestVersionPage($variables['node']);
   

   Wondering if we could just ditch this and have it displayed full view mode?

3. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
   @@ -0,0 +1,361 @@
   +    // @todo Core forgot to add a direct way to manipulate route_provider, so
   

   Is there an issue for this?

4. +++ b/core/modules/content_moderation/src/Form/ModerationStateForm.php
   @@ -0,0 +1,82 @@
   + * Class ModerationStateForm.
   

   Class moderationstateform is a moderation state form class.

5. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   @@ -0,0 +1,210 @@
   +    return $this->isModeratableBundle($entity->getEntityType(), $entity->bundle());
   

   Is moderatable a word? Should this be moderated instead?

6. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   @@ -0,0 +1,210 @@
   +  public function isModeratedEntityForm(FormInterface $form_object) {
   

   Also uses moderated here.

7. +++ b/core/modules/content_moderation/src/ModerationInformation.php
   --- /dev/null
   +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
   

   Should this be explicitly marked @internal or we don't care? Some of these aren't specific to content_moderation and could be moved into core.

8. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
   @@ -0,0 +1,212 @@
   +   * Filters entity lists to just the definitions for moderatable entities.
   

   moderatable or revisionable?

9. +++ b/core/modules/content_moderation/src/ModerationStateTransitionListBuilder.php
   @@ -0,0 +1,173 @@
   +      $delta = 10;
   +      // Change the delta of the weight field if have more than 20 entities.
   +      if (!empty($this->weightKey)) {
   +        $count = count($this->entities);
   +        if ($count > 20) {
   +          $delta = ceil($count / 2);
   +        }
   

   I'm sure this weight-on-config-entities issue has come up before, but can't remember where exactly.

10. +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
    @@ -0,0 +1,109 @@
    +   * This is a custom flag defined by WBM to load forward revisions rather than
    

    What is this WBM you speak of?

11. +++ b/core/modules/content_moderation/src/RevisionTracker.php
    @@ -0,0 +1,152 @@
    +class RevisionTracker implements RevisionTrackerInterface {
    

    Not sure why this is necessary - the entity table itself has the same information.

12. +++ b/core/modules/content_moderation/src/StateTransitionValidation.php
    @@ -0,0 +1,247 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getValidTransitionTargets(ContentEntityInterface $entity, AccountInterface $user) {
    +    $bundle = $this->loadBundleEntity($entity->getEntityType()->getBundleEntityType(), $entity->bundle());
    +
    +    $states_for_bundle = $bundle->getThirdPartySetting('content_moderation', 'allowed_moderation_states', []);
    +
    +    /** @var \Drupal\content_moderation\Entity\ModerationState $current_state */
    +    $current_state = $entity->moderation_state->entity;
    +
    +    $all_transitions = $this->getPossibleTransitions();
    +    $destination_ids = $all_transitions[$current_state->id()];
    +
    +    $destination_ids = array_intersect($states_for_bundle, $destination_ids);
    +    $destinations = $this->entityTypeManager->getStorage('moderation_state')->loadMultiple($destination_ids);
    +
    +    return array_filter($destinations, function(ModerationStateInterface $destination_state) use ($current_state, $user) {
    +      return $this->userMayTransition($current_state, $destination_state, $user);
    +    });
    +  }
    

    This comes back to my earlier question about transitions. So you can set moderation states per bundle, but not transitions. Part of me wonders whether it wouldn't be simpler to just store the transitions in bundle third party settings rather than the combination of multiple configuration entities plus this setting referencing the states.

    Do the third party settings get cleaned up when a state is removed?

13. +++ b/core/modules/content_moderation/src/StateTransitionValidation.php
    @@ -0,0 +1,247 @@
    +        && $user->hasPermission('use ' . $transition->id() . ' transition');
    

    This covers permission question above.

14. +++ b/core/modules/content_moderation/src/StateTransitionValidationInterface.php
    @@ -0,0 +1,71 @@
    +  public function isTransitionAllowed(ModerationStateInterface $from, ModerationStateInterface $to);
    

    Maybe'isTransitionValid' to differentiate it from userMayTransition.

Opened up a follow-up issue for the states/transitions stuff at #2779647: Add a workflow component, ui module, and implement it in content moderation. I don't think that needs to block commit, but would be good to have a proper look at it after since it'll need to be decided before we get out of alpha.

Thanks for the review @catch. We've got a metric tonne of minor followups to create from this issue. Most minor and some more substantial in terms of architectural changes (#2779647: Add a workflow component, ui module, and implement it in content moderation) or bugginess (#2766957: Forward revisions + translation UI can result in forked draft revisions).

I'm +1 on the rtbc I think it is time to start iterating on the functionality in core.

Here's a few of the nits from @catch's review and things that PHPStorm noticed.

The big change is a find and replace on 'moderatable' to 'moderated' since the first one is not a word. I think the code reads better for using a real word.
Also addressed in #112.2 which allows us to remove content_moderation_module_implements_alter().
And parametrized is a word apparently and preferred in core (8 v 1)

I've left all the other points of catch's review for followup because either they require discussion or are nicely self-contained.

Creditting some of the people on this issue - this process is by no means complete.

If I recall, at least when I was working on the module "moderatable" meant "we could apply moderation to it". "moderated" meant "we have done so". Eg, a node type that doesn't have moderation enabled is moderatable, but not moderated.

That may have changed in Tim's updates, but at least originally there WAS a distinction between the two. (And yes, moderatable is totally a word in English.)

I can't find moderatable in the first three online dictionaries I looked in.

That said it seems that adding -able is a productive process and could be considered allowed because moderate is a transitive verb - http://english.stackexchange.com/questions/132535/to-be-able-to-toggle-s...

This is not "experiment module" talk anymore :P

Lets get it in! :)

I have done a final review and found some minor points.

1. +++ b/core/modules/content_moderation/src/ContentPreprocess.php
   @@ -0,0 +1,57 @@
   +   * @param \Drupal\node\Entity\Node $node
   ...
   +  public function isLatestVersionPage(Node $node) {
   

   We should use an interface here.

2. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -0,0 +1,180 @@
   +    $fields['content_entity_type_id'] = BaseFieldDefinition::create('string')
   ...
   +    $fields['content_entity_id'] = BaseFieldDefinition::create('integer')
   ...
   +    $fields['content_entity_revision_id'] = BaseFieldDefinition::create('integer')
   

   I see a new module here dynamic_entity_reference_revisions. :D

3. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -0,0 +1,180 @@
   +    // @todo Add constraint that enforces unique content_entity_type_id / content_entity_id
   +    // @todo Index on content_entity_type_id, content_entity_id and content_entity_revision_id
   

   > 80.

4. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
   @@ -0,0 +1,76 @@
   +    // This is *probably* not necessary if configuration is setup correctly,
   

   I didn't know we can use * like that in comments.

5. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
   @@ -0,0 +1,76 @@
   +    // The Revisions portion of Entity API is not uniformly applied or
   +    // consistent. Until that's fixed in core, we'll make a best-attempt to
   +    // apply it to the common entity patterns so as to avoid every entity type
   +    // needing to implement this method, although some will still need to do so
   +    // for now. This is the API that should be universal, but isn't yet.
   +    // @see \Drupal\node\Entity\NodeType
   

   Is there an issue to fix this? We should add @todo here.

6. +++ b/core/modules/content_moderation/src/Entity/ModerationStateTransition.php
   @@ -0,0 +1,110 @@
   + *       "add" = "Drupal\content_moderation\Form\ModerationStateTransitionForm",
   + *       "edit" = "Drupal\content_moderation\Form\ModerationStateTransitionForm",
   + *       "delete" = "Drupal\content_moderation\Form\ModerationStateTransitionDeleteForm"
   

   I wish we could use class name constants here.

7. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -0,0 +1,277 @@
   +    // @todo what if $entity->moderation_state->target_id is null at this point?
   

   Is it still pending?

8. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -0,0 +1,255 @@
   +    // @todo write a test for this.
   

   Can we have a d.o link please?

9. +++ b/core/modules/content_moderation/src/Routing/EntityModerationRouteProvider.php
   @@ -0,0 +1,122 @@
   +            'load_forward_revision' => 1,
   

   Shouldn't this be TRUE?

10. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsDataIntegrationTest.php
    @@ -0,0 +1,159 @@
    +        // @todo I would have expected that the content_moderation_state default
    +        //   revision is the same one as in the node, but it isn't.
    

    We can improve this comment.

@jibran this is an experimental module - the code does not have to be perfect. We have time whilst the module is in alpha to make any change we like. Also in general if you want to post nitpicky reviews I encourage you to post a patch with the review. There is general confusion over the level of review an experimental module is supposed to have but I'm prettyu sure that this one has had plenty.

Anyhow,
1. This can be a followup
2. Maybe
3. Removed comment that's been done and created #2779931: Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access
4. Fixed
5. Adjusted comment to not be about "in core" but I don't think this actually needs an @todo and an issue it is just explanation of what the code is doing.
6. Okay
7. Yes
8. Created #2779933: Check for missing test coverage in the Content Moderation module and added to @todos
9. That's the stuff of followups if correct - certainly does not need changing now.
10. I think I want to leave this comment as it is for now. I think it represents a mis-understanding of how the revision of the content moderation state is related to the revision of content entity it is related to. They are not really related at all.

I've also chosen to revert the 'moderatable' to 'moderated' change. This can also be discussed after commit. It is annoying when IDEs pick up this as a spelling mistake and I think the change is worth it. But @Crell's point about some of the is..Moderatable functions needing to be converted to canBeModerated or canModerate is valid and therefore the change is not a simple find and replace.

Created #2779939: Cleanup the ModerationInformationInterface to address the moderatable issue

Thanks @alexpott for fixing the review.

I encourage you to post a patch with the review.

I didn't change the status of the issue. IMO that means the review can be ignored or some review points can be skipped. I should have mentioned that in my review. I know my review was bit too much and I didn't want to disturb the development flow of the issue that's why I didn't upload the patch.

We just need (new) follow ups for 1, 7, 9 with links to the patch and we are good to go. yay!

No code review, just a + 1, nicely done! One thing that came to mind was the revisions overview page. I'm not sure whether this is discussed somewhere or not already (here or a different issue, didn't really find something on a quick search), but it feels like that page could potentially be taken over by content moderation to show the state of the revision. Because you can set previous revisions of any state back to the current revision, but that doesn't necessarily sets that revision to 'live', because you might be manipulating a draft revision. While nothing breaks, it confused me a bit since I was able to set a early draft back to the latest revision which is also the live revision and then the 'Latest version' tab is the same. (hope that latest sentence makes sense). Anyway, should not hold up commit!

Unless I missed it somehow in testing, we seem to still be missing a way to view moderated content on e.g. admin/content.

Opened #2780613: Ensure that content in moderation can be found in the admin UI easily for webchick's comment in #129.

OK there's lots of minor and major follow-up work to do here, but already a lot of progress since this was proposed, and it'll be easier to review as individual patches with the code in core, so I've gone ahead and committed/pushed to 8.3.x and 8.2.x

Between #60 and #77 it seems that all the events were removed. Is there an issue to add those back or documentation as to why they were left out?

Moderation states are created and updated by using the Content Moderation state entity, therefore entity hooks can be used.