Make Workspaces and Content Moderation work together

1. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -103,6 +104,10 @@ public static function create(ContainerInterface $container) {
   +    if ($entity instanceof SynchronizableInterface && $entity->isSyncing()) {
   +      return;
   +    }
   

   🔧 I wonder if this guard should go into ModerationHandler instead, so you could potentially undo this behaviour if you had an entity type that was special and made special use of SynchronizableInterface somehow.

2. +++ b/core/modules/workspaces/src/EntityTypeInfo.php
   @@ -70,6 +70,29 @@ public function entityTypeBuild(array &$entity_types) {
   +  public function entityTypeAlter(array &$entity_types) {
   +    foreach ($entity_types as $entity_type_id => $entity_type) {
   +      // Non-default workspaces display the active revision on the canonical
   +      // route of an entity, so the latest version route is no longer needed.
   +      $link_templates = $entity_type->get('links');
   +      unset($link_templates['latest-version']);
   +      $entity_type->set('links', $link_templates);
   +    }
   

   ❓ Does this neglect using content moderation in the default workspace? Is the expectation that you could use all the normal CM features if you were in the default workspace?

3. Advanced workflows like moderating an entire workspace are possible as well.

   ❓ This sounds to me like you would be able to actually transition the workspace entity itself through a moderation workflow, but I don't think that's actually what this patch unblocks right?

   FWIW, I have thought about this a bit and I imagine if this was what you wanted it you could create a new workflow type and instead of options like: default revision, published status etc, each state would track information like "does reaching this state deploy the workspace". I'd be interested in contributing to this if it came up in the roadmap.

Re: #6.3, if the incompatibility check is being removed, how does the current approach work by default though? It seems like after stable, it would be something that's tricky to change in core and possibly something that should have some functional testing built around.

So the default workflow for WS + CM that will provided by core is very much up for discussion

I was primarily responding to this part. Should that question be answered in detail before removing the hook_requirements?

As a random thought, I wonder if we could test this by extending a CM test and switching workspaces in the setup method?

In general CM is useful within an unpublished workspace if you manage large volumes of content in a workspace (we are looking to use it like that where I work). However, in order to make this work properly we would want to stop workspaces from being published/merged until all content is in a published state. The CM workflow within the workspace would ensure that each individual content has gone through review accordingly.

As mentioned in another comment above, I can also see a use case where you attach a workflow to the workspace entity itself. In our initial wireframes and mockups for workspaces we actually had multiple states on entire workspaces such as "draft" and "needs review".

Follow-up review, sorry if I repeat things that have already been decided/discussed.

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,62 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +/**
   + * Implements hook_ENTITY_TYPE_access() for the 'workspace' entity type.
   + */
   +function content_moderation_workspace_access(EntityInterface $entity, $operation, AccountInterface $account) {
   

   Is this tested anywhere?

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,62 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +  if ($operation === 'publish') {
   

   Could be a nice place to return early and reduce the indent of this whole function by 1.

3. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,62 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +    // Find all workflows which are moderating entity types of the same type to
   +    // those that are tracked by the workspace.
   

   Is this comment in the right place? Finding the workflows is done later on and by the looks of it the chunk as a whole is finding states, not workflows.

4. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,62 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +        $workflow_unpublished_states[$workflow->id()] = array_filter(array_map(function (ContentModerationState $state) {
   +          return !$state->isPublishedState() ? $state->id() : NULL;
   +        }, $workflow_type->getStates()));
   

   Could this chunk be simplified to: array_filter($workflow_type->getStates(), function($state) { return !$state->isPublished();}); and then you can simply deal with a state object for the rest of the method can call $state->id() later on?

5. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,62 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +    if ((bool) $query->count()->execute()) {
   

   Does range(0, 1) speed this up further?

6. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,62 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +      return AccessResult::forbidden(t('The @label workspace can not be published because it has content in an unpublished moderation state.', [
   +        '@label' => $entity->label(),
   +      ]));
   

   Hm, what about content that is both unpublished but default, like the archived state? Does the check need to be for "non default" instead of "unpublished"?

   If I archived some content in stage, I probably would want that archival to propagate when deploying the workspace.

7. ...content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   ...content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -7,12 +7,11 @@
   

   All the refactoring in here is great, thanks!

8. +++ b/core/modules/workspaces/src/EntityTypeInfo.php
   @@ -77,6 +77,29 @@ public function entityTypeBuild(array &$entity_types) {
   +   * @todo Content Moderation should not add the 'latest-version' link template
   +   *   when the Workspaces module is installed, but this can only happen after
   +   *   Workspaces is no longer an experimental module.
   +   *   @see https://www.drupal.org/project/drupal/issues/2732071
   

   I think since workspaces alters the default behavior of the canonical route, it's probably also correct for workspaces to clean up CMs interface for viewing pending revisions.

9. +++ b/core/modules/workspaces/src/WorkspaceAccessControlHandler.php
   @@ -23,7 +23,9 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +    // @todo Should we introduce an explicit "publish any|own workspace"
   +    //   permissions?
   

   Needs issue link?

10. +++ b/core/modules/workspaces/tests/modules/workspace_publish_access_test/workspace_publish_access_test.module
    @@ -0,0 +1,19 @@
    +function workspace_publish_access_test_workspace_access(EntityInterface $entity, $operation, AccountInterface $account) {
    +  if ($operation === 'publish') {
    +    return \Drupal::state()->get('workspace_publish_access_test.result', AccessResult::neutral());
    +  }
    +}
    

    Could add a bit of general utility here and return "workspace_access_test.result.$operation".

11. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspacesContentModerationStateTest.php
    @@ -0,0 +1,164 @@
    +  protected function createEntity($entity_type_id, $moderation_state = 'published', $create_workflow = TRUE) {
    +    $entity = $this->workspaceManager->executeOutsideWorkspace(function () use ($entity_type_id, $moderation_state, $create_workflow) {
    +      return parent::createEntity($entity_type_id, $moderation_state, $create_workflow);
    +    });
    +
    +    return $entity;
    +  }
    

    This is kind of mind-bending, but I love it! :D

12. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspacesContentModerationStateTest.php
    @@ -0,0 +1,164 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function testModerationWithFieldConfigOverride() {
    +    // This test does not assert anything that can be workspace-specific.
    +    $this->markTestSkipped();
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function testWorkflowDependencies() {
    +    // This test does not assert anything that can be workspace-specific.
    +    $this->markTestSkipped();
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function testWorkflowNonConfigBundleDependencies() {
    +    // This test does not assert anything that can be workspace-specific.
    +    $this->markTestSkipped();
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function testGetCurrentUserId() {
    +    // This test does not assert anything that can be workspace-specific.
    +    $this->markTestSkipped();
    +  }
    

    Does this still incur some install cost? I would be fine with moving them into another test class in the parent.

13. Lastly, I think some functional testing for the integration would also be quite valuable. The issue introduces quite an opinionated integration between the two, but nothing really codifies it.

@Sam152

Hm, what about content that is both unpublished but default, like the archived state? Does the check need to be for "non default" instead of "unpublished"?

If I archived some content in stage, I probably would want that archival to propagate when deploying the workspace.

@amateescu

The problem is that a moderation state that is unpublished and creates a default revision (like Archive) doesn't really make sense in the context of a workspace. We need to somehow alter the default Editorial workflow provided by the Standard install profile and remove the archive bits when Workspaces and CM are installed at the same.

That sounds a bit like "Archiving doesn't fit into our current implementation so let's hide it". The first sentence is also rather strong and isn't followed by any reasoning. Would you mind elaborating on that?

Nice! Moving from the publishing status to the default revision state makes all the sense. A few comments:

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -294,7 +294,7 @@ function content_moderation_workspace_access(EntityInterface $entity, $operation
      $workflow_unpublished_states = [];
   

   This variable now stores non-default-revision states.

2. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,65 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +    $message = \Drupal::translation()->formatPlural($count, 'The @label workspace can not be published because it contains 1 item in an unpublished moderation state.', 'The @label workspace can not be published because it contains @count items in an unpublished moderation state.', [
   

   Similar to the one above, the workspace cannot be published because it contains X items in a (non-default? temporary? intermediate?) state.

3. The name "Ready for publishing" makes sense in workspaces but is misleading in the live workspace. I don't have any good hints on how to solve that. Overriding ContentModerationState::label() doesn't look very clean.

A general question at the end to all participants. This is a real-world use case we're facing time after time. Multinational organizations often want to have their content structured into markets with inheritance. For instance, Quebec gets the same content as France but with the ability to customize it, i.e. hide existing entities, create new ones (such that won't be available in France) or just change pieces of the source items. This seems to be the flow that some products on the market, ones with very expensive licenses, are providing. If, as Dries said, we want Drupal to be a good alternative to that kind of products, we need to make it possible to have more than one active workspace. Is this something that we plan to support?

That's the functionality we want to provide in #3062486: Add the ability to create sub-workspaces in order to enable the dev -> stage -> live workflow for content, and, with that patch, the concrete use-case you mentioned can be accomplished in (at least) two ways:
- Quebec can have France as the parent workspace, so it inherits its content by default, but it's also able to modify or add new content
- Quebec and France can both have a common parent workspace, allowing them to inherit its content but also to diverge in any way that might be required

There's one missing piece there. If we want to be able to do market-level content moderation, then we need some kind of an indicator saying which revision is the default one in the context of a workspace. Example:

The Quebec workspace is used to serve content for that region. There is a published node which is going to be changed, so the editor creates a new draft which should go through the moderation flow. That last part is something we can't do now with the API. Entity::loadMultiple, the entity query and even ModerationInformation::getDefaultRevisionId all return the most recent revision, an unaccepted draft in this case.

It is counter intuitive, especially for the last method which name clearly states that it should return the default version. Even if we want the first two to return pending revisions, we could at least make this information available in the ModerationInformation service. The data is there in the database but the only way to retrieve it now is with an ugly db select. If altering getDefaultRevisionId is not a good idea then maybe we could add a new method just for that?

Reviewing the inter.

1. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceContentModerationIntegrationTest.php
   @@ -0,0 +1,36 @@
   +class WorkspaceContentModerationIntegrationTest extends BrowserTestBase {
   

   The new coverage is great, but it seems like it would be valuable to expand this to at least go through at least one test scenario from the UI.

2. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspacesContentModerationStateTest.php
   @@ -47,45 +50,92 @@ protected function setUp() {
   +    try {
   +      $this->workspaces['stage']->publish();
   +    }
   +    catch (WorkspaceAccessException $e) {
   +      $this->assertEquals('The Stage workspace can not be published because it contains 3 items in an unpublished moderation state.', $e->getMessage());
   +    }
   

   This style of assertion doesn't guarantee an exception is thrown. The test still passes with the following diff applied:

   --- a/core/modules/content_moderation/content_moderation.module
   +++ b/core/modules/content_moderation/content_moderation.module
   @@ -326,6 +326,7 @@ function content_moderation_workspace_access(EntityInterface $entity, $operation
      $query->condition($workflow_condition_group);
   
      if ($count = $query->count()->execute()) {
   +    return AccessResult::neutral();
        $message = \Drupal::translation()->formatPlural($count, 'The @label workspace can not be published because it contains 1 item in an unpublished moderation state.', 'The @label workspace can not be published because it contains @count items in an unpublished moderation state.', [
          '@label' => $entity->label(),
        ]);
   

3. +++ b/core/modules/workspaces/workspaces.module
   @@ -168,6 +169,25 @@ function workspaces_cron() {
   +/**
   + * Implements hook_modules_installed().
   + */
   +function workspaces_modules_installed($modules) {
   +  if (!in_array('content_moderation', $modules, TRUE) || \Drupal::isConfigSyncing() || !in_array(\Drupal::installProfile(), ['standard', 'demo_umami'], TRUE)) {
   +    return;
   +  }
   +
   +  // In the context of a workspaces, the 'Published' moderation state label is
   +  // ambiguous because when an entity reaches that state it is not automatically
   +  // published to the Live site, so we are changing its label to
   +  // 'Ready for publishing'.
   +  $workflows = Workflow::loadMultipleByType('content_moderation');
   +  if (isset($workflows['editorial'])) {
   +    $workflows['editorial']->getTypePlugin()->setStateLabel('published', 'Ready for publishing');
   +    $workflows['editorial']->save();
   +  }
   +}
   

   Hm, this seems a bit invasive and unnecessary. If the use case is split 80/20 I think some valid alternatives are: allow people to simply update and customise labels as they see fit or, ship a "Workspace editorial workflow" with updated labels created specifically for folks with that use case in mind.

   As a fallback, I think it should at least check that it isn't changing a state label that a user has already decided to customise.

1. +++ b/core/modules/workspaces/src/WorkspacePublisher.php
   @@ -74,6 +75,12 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Con
      public function publish() {
   +    $publish_access = $this->sourceWorkspace->access('publish', NULL, TRUE);
   +    if (!$publish_access->isAllowed()) {
   +      $message = $publish_access instanceof AccessResultReasonInterface ? $publish_access->getReason() : '';
   +      throw new WorkspaceAccessException($message);
   +    }
   +
   

   It's really hard to meaningfully review this issue for me because I don't have practical experience with workspaces and don't really know the code. Will have to try it out.

   This does feel a bit strange though.

   Usually access is more or less directly tied to the current user, but the way it is used here is more about validation I think and doesn't depend on the user. So normally, entity operations (like save and delete) don't enforce access), we leave that to the code calling it. e.g. this could be annoying for drush commands that want to publish a workspace combined with custom access checks.

   I didn't fully understand the publish access code but it feels more like validation or a different kind of conflict check like the one below? Maybe there could be a separate hook/event for that?

   Just thinking out loud.

2. +++ b/core/modules/workspaces/workspaces.module
   @@ -159,6 +169,25 @@ function workspaces_cron() {
   +  // In the context of a workspace, the 'Published' moderation state label is
   +  // ambiguous because when an entity reaches that state it is not automatically
   +  // published to the Live site, so we are changing its label to
   +  // 'Ready for publishing'.
   +  $workflows = Workflow::loadMultipleByType('content_moderation');
   +  if (isset($workflows['editorial'])) {
   +    $workflows['editorial']->getTypePlugin()->setStateLabel('published', 'Ready for publishing');
   +    $workflows['editorial']->save();
   +  }
   

   Yeah, also not sure about this, even if the 80% use case is to do this within a workspace, it's still wrong then for the other case. Might need to be more dynamic? But then it's hard to configure. Not sure :)

   Also wondering how this works exactly with non-EN sites (single and multilingual), because if nothing else, this label won't be identified as translatable. And if the site uses non-EN as the default language, it might be translated already and you'd set it back to an EN label.

FWIW beyond #31 I didn't have any additional feedback, so +1 RTBC if @Berdir's feedback is addressed.

This looks good to me, #32.1 seems asked and answered if it was a decision discussed by @amateescu and other core team members.

This looks good to me, thanks!

Aside from a few minor nits, the only thing that gave me pause is the fact that both CM and WS have a soft dependency on each other: given that CM has to know about WS but not viceversa, maybe we could move all the new tests and the code unsetting the latest-version template to CM to fix this.

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -273,6 +274,65 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
   +function content_moderation_workspace_access(EntityInterface $entity, $operation, AccountInterface $account) {
   

   Nit: can we use WorkspaceInterface $workspace here?

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
   @@ -274,11 +274,8 @@ public function testContentModerationStateTranslationDataRemoval($entity_type_id
   -      $langcode = 'it';
   ...
   +      $langcode = 'fr';
          $translation = $entity->addTranslation($langcode, ['title' => 'Titolo test']);
   

   This is confusing :D

3. +++ b/core/modules/workspaces/src/EntityTypeInfo.php
   @@ -77,6 +77,24 @@ public function entityTypeBuild(array &$entity_types) {
   +  public function entityTypeAlter(array &$entity_types) {
   

   As mentioned above: could we move this to CM? I'm wondering whether we could even change the logic in LatestRevisionCheck::access() code to further/only check that the latest revision is not also the default revision, regardless of it being pending. This would address WS implicitly without having to do a module exist or check that the entity type is handled by WS.

4. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceContentModerationIntegrationTest.php
   @@ -0,0 +1,118 @@
   +    $node_storage = \Drupal::entityTypeManager()->getStorage('node');
   

   Nit: this is not used.

5. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceCRUDTest.php
   @@ -227,8 +227,9 @@ public function testDeletingPublishedWorkspace() {
   -      'view any workspace',
   -      'delete any workspace',
   +      'view own workspace',
   +      'edit own workspace',
   +      'delete own workspace',
   

   Why switch to "own"? Is this just making the test stricter?

6. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspacesContentModerationStateTest.php
   @@ -0,0 +1,259 @@
   +namespace Drupal\Tests\workspaces\Kernel;
   

   As mentioned above, could move this and WorkspaceContentModerationIntegrationTest.php to CM?

Discussed #39.3 quickly with @catch, who didn't review the code: we agreed that mutual soft-dependencies are not ideal, however, since this an important step towards making WS stable, we're fine with further untangling happening in a follow-up, especially because EntityTypeInfo::entityTypeAlter() is not assuming the presence of Content Moderation explicitly. From a theoretical POV any module could provide the latest-version link template.

Aside from that, the only tricky aspect for me is what @Berdir mentioned #32.1, OTOH @amateescu's reply is compelling and we're talking about an implementation detail on an @internal service, thus, should we be able to find a cleaner solution, it should be possible to implement that in a non-breaking way.

Committed 4362606 and pushed to 8.8.x. Thanks!

It would be good to provide a CR for site builders.

Setting to NW for the missing stuff above.

Fixing tag

We've created the Workspaces moderation contrib module that makes the editors feel like they're moderating a live workspace. It introduces the concept of a shadow workspace that is loaded instead of a regular one for users with a specific permission. Shadow workspaces are excluded from all the listings so they cannot be switched to directly. Once an entity reaches a default revision state in the shadow workspace it is copied over to the real workspace.

The module is still a prototype. See this test for an example.

This is causing an issue with the contrib module Multiversion which defines an entity type "workspace", therefore the content_moderation_workspace_access() hook gets called.