Problem/Motivation

See #2755073: WI: Content Moderation module roadmap and #2721129: Workflow Initiative

Proposed resolution

Rename and drop-in Workbench Moderation an experimental module into core with the following 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

Remaining tasks

Review the code.

User interface changes

Adding new user interfaces for moderating states and forward revisions etc.

API changes

No API changes. Only additions isolated in the experimental module.

Data model changes

No data model changes. Only additional entities and fields.

Files: 

Comments

dixon_ created an issue. See original summary.

dixon_’s picture

Issue summary: View changes
Status: Active » Needs work

This plan still needs work.

dixon_’s picture

Issue summary: View changes
dixon_’s picture

Issue summary: View changes
fago’s picture

>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.

dixon_’s picture

Issue summary: View changes

@fago That makes sense. I updated the proposal :)

dixon_’s picture

Title: WI: Entity Moderation module » WI: Content Moderation module
Crell’s picture

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?

timmillwood’s picture

Status: Needs work » Needs review
FileSize
290.24 KB
dawehner’s picture

Nice!

timmillwood’s picture

Status: Needs work » Needs review
FileSize
286.68 KB
601 bytes
timmillwood’s picture

Status: Needs work » Needs review
FileSize
350.66 KB
2.83 KB

Trying to address the failing tests.

amateescu’s picture

Status: Needs work » Needs review
FileSize
350.95 KB
1.94 KB

#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.

timmillwood’s picture

@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.

timmillwood’s picture

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
amateescu’s picture

Status: Needs work » Needs review
FileSize
351.1 KB
754 bytes

This should do it.

amateescu’s picture

Status: Needs work » Needs review
FileSize
288.32 KB

Removed a lot of unrelated changes from the patch.

dawehner’s picture

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.

amateescu’s picture

@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'.

Bojhan’s picture

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.

timmillwood’s picture

Category: Plan » Task

@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.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #24 seems good to me.

dawehner’s picture

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

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

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

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

jibran’s picture

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.

timmillwood’s picture

Assigned: Unassigned » timmillwood
Status: Needs review » Needs work

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.

dawehner’s picture

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

dixon_’s picture

Title: WI: Content Moderation module » Rename Workbench Moderation module and drop into core as experimental
Category: Task » Feature request
Issue tags: -Needs product manager review, -Needs framework manager review, -Needs subsystem maintainer review
Parent issue: #2721129: Workflow Initiative » #2755073: WI: Content Moderation module roadmap

The idea of this issue was to have this strictly as a planning issue, for needed sign-offs. But considering that the patch/review workflow already kicked-off on this issue, I'm moving the plan issue here: #2755073: WI: Content Moderation module roadmap

dixon_’s picture

Issue summary: View changes

Updated the issue summary to follow a traditional implementation issue.

dixon_’s picture

@dawehner In my opinion, the definition around what's ok/not ok for an experimental module is still not fully clear. But below are a few cut-outs from the meeting notes we took after the core conversation in NOLA:

Gates [for experimental modules]:

Testing - full functional coverage for the minimal use case, extensive unit tests not needed
Documentation - Minimal code documentation needed. Identify where handbook updates are needed, but not need to update
Performance - can be completely bypassed
UX/Accessibility - Can be bypassed as long as the idea/prototypes are planned and somewhat validated
Security/data integrity - no known issues

Bojhan: We don’t want experimental modules to be bad user experiences, but we also don’t want to fiddle around with form labels. It can be fine if an experimental module is not fully baked.
Once it emerges from an experimental state, we are picky about the details.
Once we have largely the right ideas, then we move on to a more robust evaluation.

The original notes are here: https://docs.google.com/document/d/17LIgHEXBbRm_IJeJRznlLzO5zQO21ImhvyPZ...

Based on the above, and various other conversations I've been part of, it seems like experimental modules are allowed a lot lower gates than we're used to in core. Basically to embrace the iterative part of this new proposed process.

So in my opinion, this seems like a good summary:

  • We don't compromise on security
  • We don't want the user experience to be bad, but not too much effort needs to be put into the very first iteration?
  • Performance can be completely bypassed
  • It's ok to compromise on testing and documentation, as long as the very basics are covered
  • Minor nit-picks should be done in the issue if/when we decide to convert this module from experimental to a standard module
  • It's ok to commit experimental modules that surface know and major bugs in core (see meeting notes here: #2721129-42: Workflow Initiative).

I think we need this formalized somewhere in the core governance documentation somewhere though...

catch’s picture

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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
289.59 KB
20.24 KB

#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.

larowlan’s picture

Status: Needs review » Needs work

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.

dixon_’s picture

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

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. So for now adding Tim into MAINTAINERS.txt makes sense to me.

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.

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. So WBM should remain in contrib and adding this UI ontop of the core module. So therefore it probably would make sense to keep any migration logic and tests for that in WBM itself.

larowlan’s picture

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.

Crell’s picture

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.

dawehner’s picture

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.

Crell’s picture

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.

timmillwood’s picture

@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.

agentrickard’s picture

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.

webchick’s picture

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.

larowlan’s picture

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

larowlan’s picture

webchick’s picture

Sweet!! :)

Bojhan’s picture

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 :)

alexpott’s picture

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

timmillwood’s picture

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

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

alexpott’s picture

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.

catch’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
alexpott’s picture

webchick’s picture

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. :)

I can see it!

Berdir’s picture

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?

larowlan’s picture

+++ 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

Wim Leers’s picture

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: File usage not incremented when adding new translation + #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.

timmillwood’s picture

#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.

dixon_’s picture

I agree we should look to fix the uninstall issue and perhaps also add an annotation key for opting out of moderation, as Berdir suggested.

After these fixes I would really encourage getting this patch committed and deal with the last reviews and nit picks in a follow-up. It's hard to both re-roll and review a big patch like this.

I have created both the follow-up issue and an initial draft change record for this: https://www.drupal.org/node/2757719

Crell’s picture

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.

alexpott’s picture

@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.

timmillwood’s picture

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

Crell’s picture

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.

catch’s picture

Status: Needs review » Needs work

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).

cosmicdreams’s picture

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

larowlan’s picture

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).

jhedstrom’s picture

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?

larowlan’s picture

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

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

jhedstrom’s picture

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.

Wim Leers’s picture

Could you provide a summary of the changes?

amateescu’s picture

@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.

amateescu’s picture

FileSize
28.41 KB

Oops, forgot to attach the file :/

alexpott’s picture

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

amateescu’s picture

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.

tim.plunkett’s picture

+++ 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');

timmillwood’s picture

Status: Needs work » Needs review
FileSize
40.15 KB
356.46 KB

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

timmillwood’s picture

Status: Needs work » Needs review
FileSize
356.63 KB
1.79 KB

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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
356.66 KB
3.47 KB

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

hass’s picture

Status: Needs work » Needs review

Green, but failed???

timmillwood’s picture

FileSize
358.51 KB
2.43 KB

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

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Needs work » Needs review

Passed second time around.

timmillwood’s picture

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.

timmillwood’s picture

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!

dawehner’s picture

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.

timmillwood’s picture

Issue summary: View changes

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

xjm’s picture

Issue tags: +beta target

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.

timmillwood’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 100: rename_workbench-2725533-100.patch, failed testing.

The last submitted patch, 100: rename_workbench-2725533-100.patch, failed testing.

The last submitted patch, 100: rename_workbench-2725533-100.patch, failed testing.

timmillwood’s picture

I'm struggling to recreate these failures locally.

The last submitted patch, 100: rename_workbench-2725533-100.patch, failed testing.

larowlan’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
358.41 KB
689 bytes

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.

alexpott’s picture

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

Status: Needs review » Needs work

The last submitted patch, 108: 2725533-2-108.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
964 bytes
359.06 KB

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

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

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.

alexpott’s picture

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.

catch’s picture

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.

catch’s picture

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.

alexpott’s picture

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.

alexpott’s picture

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.

alexpott’s picture

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

alexpott’s picture

Crell’s picture

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.)

alexpott’s picture

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

alexpott’s picture

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...

Bojhan’s picture

This is not "experiment module" talk anymore :P

Lets get it in! :)

jibran’s picture

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.

alexpott’s picture

@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.

alexpott’s picture

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

jibran’s picture

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!

swentel’s picture

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!

webchick’s picture

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

catch’s picture

Title: Rename Workbench Moderation module and drop into core as experimental » Add experimental content_moderation module

  • catch committed bc00f08 on 8.3.x
    Issue #2725533 by timmillwood, alexpott, amateescu, webchick, dixon_,...

  • catch committed 5cd146b on 8.2.x
    Issue #2725533 by timmillwood, alexpott, amateescu, webchick, dixon_,...

catch credited becw.

catch credited becw.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

xjm’s picture

Issue tags: +8.2.0 release notes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

benjy’s picture

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?

timmillwood’s picture

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