Add Workspaces experimental module

For CPS there is a similar concept (called 'site versions'). The way it gets around the condition on every query was the following:

1. There is a hard-coded, 'published' site version. When viewing the 'published' site (i.e. not previewing), any query alters are skipped.

2. When previewing the site in a site version, you have to join on the tracking table, can't really avoid that.

3. Additionally, only entities modified in a site version are tracked in that table - it uses a CASE IF WHEN clause in the query alter to either get the default revision for untracked entities or the draft revision for entities in the workspace.

I'm not sure what the storage should look like for core, but conceptually this means the best performance when not previewing (i.e. no change at all), and allows for workspace storage to only track changes, not every entity. (After a site version is published the full history is stored, which does/has caused problems with scaling, but there are issues discussing that in the CPS queue).

Publishing a site version is then taking the 'target' revision for each entity and making it the default revision (then changing status of the site version itself) - this means the full process of publishing is usually a handful of entity saves, which can happen in a single transaction.

+1 to #4.

The query alter only when not in the 'published' workspace, did work very well for us.

Also a table + COALESCE() on the join did also wonders for us.

I think COALESCE is supported also for other DB engines.

Edit:

Uhm and cloning entities per workspace is a no-go for core. Should just use revisions and track those, but always work on 'live' if not overridden in the workspace (e.g. similar to an always rebased branch).

That also worked very well and scalable for us in CPS.

When content is added to the "stage" workspace how do we prevent it from appearing on "Live"?

1. Save an initial, default revision. Regardless of the 'published' status of the entity, set it to unpublished. This way it can never show up on the live site.

2. Save a draft revision, with the published status however it was set on the entity, this one is attached to the workspace. Since this will be 'published' in the workspace (if it was set to published on the form), it'll show up.

CPS does this, except the revision saving is tricky because Drupal 7 doesn't allow you to save a revision without updating the base table, but the end result is the same.

This requires published/unpublished support for all entities that can be put into workspaces.

Not sure on the where question. First inclination is to put it in a module though, its not really needed except for whole-site-preview and publishing-sets-of-content-at-once, and sites can be built without either of those.

#8: A further part that CPS does here is to remove unpublished content (unpublished on the base entity) from listings while outside of a changeset / site version (e.g. in the live changeset).

And because you cannot just un-publish something, but need to go via the workflow, that works very well.

At least that is how I remember this works.

Replicating content from a non-live to a live workspace is just a case of removing the entry from the table (or entity), at which point the entity will "belong" to the live workspace.

For reference the way CPS does this is the following:

1. Publishing a workspace means 'make the revision associated with the workspace/site version the default one' - which either publishes a draft revision or publishes the entity in general if it's new). Ideally that should happen in a single transaction.

2. The workspace/site version gets 'archived' - so the historical information of what was in it is available. (CPS also records the state of all entities on the site at the time it was published, for historical review and rollback which I don't think we should do in core since it's an implementation nightmare).

Well, one question I would ask myself here. What is the concrete usecase we solve with this additional experimental module. Can endusers somehow profit from it? Having an experimental module without any enduser features is indeed a big weird. I'm wondering whether we could implement a mini version of replication, or some other feature which workspaces would allow, even if it would be thrown away later.

@timmillwood commented on #2755073: WI: Content Moderation module roadmap that the Workflow team wanted to target this for 8.4.

It's a ways down the roadmap in #2721129: Workflow Initiative and hadn't been discussed with release managers yet. We'd like to see Content Moderation and Workflow stabilize before we add additional workflow features to core. We'd also like to ensure bugs like #2856363: Path alias changes for draft revisions immediately leak into live site and issues with forward revisions are addressed before this is considered for core (they are probably also blockers for Content Moderation being stable anyway).

So, marking postponed for now based on that. Thanks @timmillwood!

FWIW, #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger was also a must-have for Phase A and not complete, and this is Phase F. (Also the revision upgrade path is still listed as Phase A, but I'm unclear on the status of that.)

This module is gigantic, and super hard to make a mind off.

1. +++ b/core/modules/workspace/src/Changes/ChangesInterface.php
   @@ -0,0 +1,42 @@
   +  public function includeDocs($include_docs);
   

   A classical example why having short names doesn't help at all: This could mean includeDocuments or includeDocumentation ... and well in the end its all about entities anyway, so could this be named incldueEntitiess?

2. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,177 @@
   +  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields = parent::baseFieldDefinitions($entity_type) + self::publishedBaseFieldDefinitions($entity_type);
   +
   +    $fields['uid'] = BaseFieldDefinition::create('entity_reference')
   +      ->setLabel(t('User'))
   +      ->setDescription(t('The username of the entity creator.'))
   +      ->setSetting('target_type', 'user')
   +      ->setDefaultValueCallback('Drupal\workspace\Entity\ContentWorkspace::getCurrentUserId')
   +      ->setTranslatable(TRUE)
   +      ->setRevisionable(TRUE);
   +
   +    $fields['workspace'] = BaseFieldDefinition::create('entity_reference')
   +      ->setLabel(t('workspace'))
   +      ->setDescription(t('The workspace of the referenced content.'))
   +      ->setSetting('target_type', 'workspace')
   +      ->setRequired(TRUE)
   +      ->setTranslatable(TRUE)
   +      ->setRevisionable(TRUE)
   +      ->addConstraint('workspace', []);
   +
   +    $fields['content_entity_type_id'] = BaseFieldDefinition::create('string')
   +      ->setLabel(t('Content entity type ID'))
   +      ->setDescription(t('The ID of the content entity type this workspace is for.'))
   +      ->setRequired(TRUE)
   +      ->setRevisionable(TRUE);
   +
   +    $fields['content_entity_id'] = BaseFieldDefinition::create('integer')
   +      ->setLabel(t('Content entity ID'))
   +      ->setDescription(t('The ID of the content entity this workspace is for.'))
   +      ->setRequired(TRUE)
   +      ->setRevisionable(TRUE);
   +
   +    $fields['content_entity_revision_id'] = BaseFieldDefinition::create('integer')
   +      ->setLabel(t('Content entity revision ID'))
   +      ->setDescription(t('The revision ID of the content entity this workspace is for.'))
   +      ->setRequired(TRUE)
   +      ->setRevisionable(TRUE);
   +
   +    return $fields;
   +  }
   

   Given that I assume you cannot put translations in their own workspaces?

3. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,177 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getOwner() {
   +    return $this->get('uid')->entity;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getOwnerId() {
   +    return $this->getEntityKey('uid');
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setOwnerId($uid) {
   +    $this->set('uid', $uid);
   +    return $this;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setOwner(UserInterface $account) {
   +    $this->set('uid', $account->id());
   +    return $this;
   +  }
   

   Do we have an issue to have a trait for that?

4. +++ b/core/modules/workspace/src/Negotiator/SessionWorkspaceNegotiator.php
   @@ -0,0 +1,53 @@
   +    return $workspace_id ?: $this->container->getParameter('workspace.default');
   

   Couldn't we use 'default' as default workspace parameter always?

5. +++ b/core/modules/workspace/src/Changes/ChangesInterface.php
   @@ -0,0 +1,42 @@
   +/**
   + * Define and build a changeset for a Workspace.
   + */
   +interface ChangesInterface {
   +
   

   Documentation here should IMHO explain what a changeset means. I just assume you'll work on https://www.drupal.org/core/gates#documentation-block-requirements over time.

6. +++ b/core/modules/workspace/src/Negotiator/WorkspaceNegotiatorBase.php
   @@ -0,0 +1,49 @@
   +abstract class WorkspaceNegotiatorBase implements WorkspaceNegotiatorInterface, ContainerAwareInterface {
   +
   +  use ContainerAwareTrait;
   +
   

   I have a hard time undertanding why these things have be to container aware by default

7. +++ b/core/modules/workspace/src/Negotiator/WorkspaceNegotiatorInterface.php
   @@ -0,0 +1,43 @@
   +  /**
   +   * @param \Drupal\Core\Session\AccountInterface $current_user
   +   */
   +  public function setCurrentUser(AccountInterface $current_user);
   +
   +  /**
   +   * @param \Drupal\workspace\WorkspaceManagerInterface $entity_manager
   +   */
   +  public function setWorkspaceManager(WorkspaceManagerInterface $entity_manager);
   

   These things sounds like they should be injected using a constructor

8. +++ b/core/modules/workspace/src/Plugin/Field/WorkspaceFieldItemList.php
   @@ -0,0 +1,93 @@
   +      $revisions = \Drupal::service('entity.query')->get('content_workspace')
   +        ->condition('content_entity_type_id', $entity->getEntityTypeId())
   +        ->condition('content_entity_id', $entity->id())
   +        ->condition('content_entity_revision_id', $entity->getRevisionId())
   +        ->allRevisions()
   +        ->sort('revision_id', 'DESC')
   +        ->execute();
   +
   

   IMHO these queries should opt out of access checking.

9. +++ b/core/modules/workspace/src/Replication/ReplicationInterface.php
   @@ -0,0 +1,28 @@
   +   * @return \Drupal\workspace\Entity\ReplicationLogInterface
   +   */
   +  public function replicate(UpstreamInterface $source, UpstreamInterface $target);
   
   +++ b/core/modules/workspace/src/Replication/ReplicationManager.php
   @@ -0,0 +1,39 @@
   +   * @return mixed
   ...
   +          return $replicator->replicate($source, $target);
   

   I'm wondering whether as a first step it would be enough to just return the object, but not store it in the database?

10. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,253 @@
    +  public function entityTypeCanBelongToWorkspaces(EntityTypeInterface $entity_type) {
    ...
    +  public function getSupportedEntityTypes() {
    

    Given these two things do basically the same (once vs. multiple) the names are quite different.

11. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,253 @@
    +    // If the current user doesn't have access to view the workspace, they
    +    // shouldn't be allowed to switch to it.
    +    // @todo Could this be handled better?
    ...
    +      throw new WorkspaceAccessException('The user does not have permission to view that workspace.');
    

    You mean you want to do something else than throwing an exception?

Just reviewed this with the workspace team, Gabor and moi:

The "Add new workspace" buttons is missing in admin. Otherwise, this MVP looks feature complete.

Switching workspaces is jarring: it happens (too) fast and there's little feedback that it actually happened. Wondering if there's enough (visual) feedback about what just happened when you switch workspaces. A first useful step could be to have a clear visual label that distinguishes the live workspace from all the others.

Another idea was the automatically disappearing message "you are now in workspace Foo". See #2773601: Display "You are now in edit mode" prompt when user enters edit mode for a similar treatment for Edit mode. (Showing messages inside the top tray is not useful when the tray closes between workspace switches)

The top tray could be made more compact (in height) in this version where there's no search available yet.

Feel free to add the "needs usability review" tag again when needed :)

Fixed #46 1 and 2.
Need more information for 3.

Here's an initial architectural review. While going through the code base to get more familiar with it, I also fixed some problems that were standing out on a quick read.

1. What are Upstream plugins and why do we need them? we seem to have only one implementation, \Drupal\workspace\Plugin\Upstream\Workspace and it doesn't do aything other than returning the label of a workspace.

   None of the classes related to them have any kind of useful description: \Drupal\workspace\UpstreamInterface, \Drupal\workspace\UpstreamManager, \Drupal\workspace\Annotation\Upstream, \Drupal\workspace\Plugin\Upstream\Workspace.

   Are they related to the replication stuff somehow?

2. Which brings me to my next point: Do we really need to introduce the replication stuff in this initial patch for workspaces? In the workflow initiative roadmap, adding the replication API seems to belong to a different phase: #2867707: WI: Phase H: Conflict management and local workspace merging support.

   Can we move the work done by \Drupal\workspace\Replication\DefaultReplicator directly into \Drupal\workspace\Form\DeploymentForm somehow, to simplify the initial patch? We can always put them back in a follow-up issue, even for 8.5.x.

3. \Drupal\workspace\WorkspacePointerInterface is not used anywhere, can it be removed?
4. \Drupal\workspace\ParamConverter\EntityRevisionConverter: Content Moderation also provides a similar parameter converter, we need a separate issue to put one in core and remove the one from CM and this one from the patch.
5. The view_revision_trees permission was not used anywhere, so I removed it.
6. The update any workspace from upstream is also not used anywhere, but it sounds useful. Do we keep (and use) it or not?

#51.1: Ok, so upstream plugins are related to replication as I suspected. Would it be possible to only replicate to local workspaces in the initial version of the module, and create a followup to introduce the Upstream plugins?

#51.2: Actually, we could do it somewhere in WorkspaceManager perhaps, and have the form call a method there.

#51.3: Ok, removed in this patch.
#51.4: TODO
#51.5: Cool :)
#51.6: TODO

Here's another round of review and fixes, this time focused mainly on the Workspace and ContentWorkspace entity types:

1. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,175 @@
   + *     "storage_schema" = "Drupal\content_moderation\ContentWorkspaceStorageSchema",
   

   Missing storage schema implementation? This class obviously doesn't exist :)

2. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,175 @@
   +      ->addConstraint('workspace', []);
   

   I can't find any constraint validator in the module, is this supposed to be used or can it be removed?

3. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,175 @@
   +  public static function updateOrCreateFromEntity(ContentWorkspace $content_workspace) {
   +    $content_workspace->realSave();
   +  }
   

   The ContentModerationState entity type went through a lot of fixes since the last time this issue was touched, we need to bring over all those improvements.

4. +++ b/core/modules/workspace/src/Entity/Form/WorkspaceForm.php
   @@ -0,0 +1,131 @@
   +    $form['machine_name'] = [
   +      '#type' => 'machine_name',
   +      '#title' => $this->t('Workspace ID'),
   +      '#maxlength' => 255,
   +      '#default_value' => $workspace->get('machine_name')->value,
   +      '#disabled' => !$workspace->isNew(),
   +      '#machine_name' => [
   +        'exists' => '\Drupal\workspace\Entity\Workspace::load',
   +      ],
   +      '#element_validate' => [],
   +    ];
   

   This proves that #2685749: Add a 'machine_name' widget for string field types with a UniqueField constraint is needed in core.

5. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,140 @@
   + *   local = TRUE,
   

   What is this flag used for?

6. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,140 @@
   +  public static function loadOrCreate($id) {
   +    $entities = \Drupal::entityTypeManager()
   +      ->getStorage('replication_log')
   +      ->loadByProperties(['uuid' => $id]);
   

   \Drupal\workspace\Replication\DefaultReplicator::replicate() calls this method with an ID composed of an md5() hash, but here we store that hash in a UUID field. That seems very weird, why not just have a string ID as the main identifier for the ReplicationLog entity?

7. +++ b/core/modules/workspace/src/Index/SequenceIndex.php
   @@ -0,0 +1,111 @@
   +/**
   + * Class SequenceIndex
   + */
   +class SequenceIndex implements SequenceIndexInterface {
   
   +++ b/core/modules/workspace/src/Index/SequenceIndexInterface.php
   @@ -0,0 +1,40 @@
   +/**
   + * Interface SequenceIndexInterface
   + */
   +interface SequenceIndexInterface {
   

   I'm guessing that this needs to stay in the initial patch, so we really need to describe what it is and how it's supposed to be used.

   Is this really something that needs to be overrideable (i.e. a service in the container)? Can it be a utility class with no interface instead?

   Alternatively, can't we use the key/value sorted set store directly instead of this extra layer of indirection?

8. +++ b/core/modules/workspace/src/KeyValueStore/DatabaseStorageSortedSet.php
   @@ -0,0 +1,250 @@
   +class DatabaseStorageSortedSet implements KeyValueStoreSortedSetInterface {
   
   +++ b/core/modules/workspace/src/KeyValueStore/KeyValueStoreSortedSetInterface.php
   @@ -0,0 +1,76 @@
   +interface KeyValueStoreSortedSetInterface {
   

   \Drupal\workspace\KeyValueStore\KeyValueStoreSortedSetInterface does not extend \Drupal\Core\KeyValueStore\KeyValueStoreInterface, so for example there are no methods to delete an item from this key value store, is that intended? If so, why? :)

9. +++ b/core/modules/workspace/src/Plugin/Field/FieldType/ReplicationHistoryItemList.php
   @@ -0,0 +1,7 @@
   +class ReplicationHistoryItemList extends FieldItemList {}
   

   Why do we need this empty field item list class? Can it be removed?

10. +++ b/core/modules/workspace/src/Plugin/Field/WorkspaceFieldItemList.php
    @@ -0,0 +1,94 @@
    +  protected function getWorkspace() {
    

    Like the point above, we need to make sure that we bring all the fixes that went into the computed field from Content Moderation back here.

11. +++ b/core/modules/workspace/src/WorkspaceCacheContext.php
    @@ -0,0 +1,53 @@
    +    return 'ws.' . $this->workspaceManager->getActiveWorkspace();
    

    Is there any reason for the 'ws.' prefix?

12. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,227 @@
    +  /**
    +   * @var string[]
    +   */
    +  protected $blacklist = [
    +    'content_workspace',
    +    'workspace'
    +  ];
    

    Blacklist of what? :)

13. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,227 @@
    +  public function entityCanBelongToWorkspaces(EntityInterface $entity) {
    +    return $this->entityTypeCanBelongToWorkspaces($entity->getEntityType());
    +  }
    ...
    +  public function entityTypeCanBelongToWorkspaces(EntityTypeInterface $entity_type) {
    

    Do we really need two methods for this? The first one seems pretty useless to me..

For future reference, the TODO list so far is: #50.1, #50.2, #50.4 and #50.6.

This is a new feature, also major.

Hey, folks! The product management team took a look at this in today's meeting, and posted our initial, unfiltered feedback (which still needs prioritization and/or validation) at #2910673: Workspace UI usability review.

In general, the MVP is solid and all the concepts are fully fleshed out. However, let's make sure we start the dialog early on how to resolve some of these identified UX challenges, so that they don't crop up as blockers at the very end.

We can meet in Vienna to go over this some in-person so we're all on the same page about what needs to be fixed up, and by when (e.g. before commit, before stable, etc.).

Keep up the great work! :D

a specific revision can belong to one or more workspaces

I don't see how you could get to this situation via the UI - is this if a change is copied from one workspace to another?

a specific revision can belong to one or more workspaces

I don't see how you could get to this situation via the UI - is this if a change is copied from one workspace to another?

In a dev -> stage -> live workspace scenario, this would happen when you pull changes from dev into stage, because the revision will now exist in all three workspaces.

This basically means that we're implementing the Git logic, while CPS would've discarded/archived the dev workspace instead.

I was talking to @timmillwood today about the next steps for this patch and these are a few points that came up:

- we need to drop the custom toolbox stuff from the patch and use the settings tray implementation
- we should consolidate all those separate workspace forms into a single form
- we need to document better all the "why"s in the code, not only the "what"s (for example, we want something like: "why is this code doing X", instead of just "this code is doing X.")
- we need to write proper integration/kernel tests for all the critical parts of the module, which are the three entry points where an entity can be swapped to a different revision: "views query alter", "entity query alter" and "hook entity load"
- we need to make sure that when we display a view of entities, only two of those entry points are reached: "views query alter" and (maybe) "hook entity load", because right now "entity query alter" fires as well

In the meantime, this patch uses loadRevision() a lot, which means a separate database for each revision load, so I opened #2912562: Add support for loading multiple entity revisions at once to improve the performance in this area.

Here are the general notes that I took during the Friday code sprint in Vienna when meeting together with Andrei and Tim:

## A. General

1. Remove all UI from initial patch
  - Keep upstream and replication plugins in patch
2. Modify Settings Tray to use on-top of page, separate patch

## B. Testing

1. Add test to assert that one revision can belong to multiple workspaces

## C. New name for content_workspace entity

1. Ask Jozef for suggestions
2. Possibly content_workspace_link ?

## D. New name for workspace module

1. workspaces (plural)

## E. Upstream plugins

1. Upstream should extend configurable plugin base
2. More properties on the plugin annotation
  - remote
  - label
  - description

## F. Changes service

1. Use more descriptive method names
2. Better documentation
3. Use documentation from CouchDB

## G. Deployment controller

1. Remove DeploymentController
2. Keep the deployment form as a separate from, add cancel button
3. Deployment form fixes
  - Remove collapsible
  - Alignment issue with the buttons

## H. ContentWorkspaceLinkInterface

## I. ReplicationLog

1. Remove local=TRUE
2. Keep the ReplicationLog entity as revisionable
3. Clean-up base fields and inherit from base class
4. Rename 'ok' field to 'success'
5. Rename instances of "last_seq" to something more descriptive

## J. ReplicationHistoryItem

1. Document why we need this
2. Rename "docs" to "entities" in all properties
3. Remove non-MVP properties
  - missing_found
  - missing_checked

## K. WorkspaceFieldItemList

1. Compare with changes in the equivalent class from Content Moderation

## L. WorkspaceInterface

1. Consider moving methods from the WorkspaceManager to the repository
2. Use trait for created time etc.

## M. Activation form

1. Remove activate form base
2. Needs to extend confirmation form base instead

## N. Switcher form

1. Needs to extend form base
2. Needs to be a select list

## O. SequenceIndex

1. Look into extending base classes?
2. The sequence index should be considered "immutable", document this
3. When a workspace is deleted, the entire key/value collection should be deleted

## P. DatabaseStorageSortedSet

1. Look into extending base classes?

## Q. KeyValue factories

1. Revisit the factor pattern in core for key/value factories

## R. Workspace negotiator

1. Remove container parameter
2. Just return the string 'live'
3. Remove parameter negotiator
4. Remove negotiator base class

## S. EntityRevisionConverter

1. Remove entierly (separate patch already exists)

## T. Workspace deriver

1. Better documentation

## U. ReplicationInterface

1. More documentation and why it's a tagged service

## V. Default replicator

1. Break out the revision diff logic into protected method
2. Use the load multiple revision functionality
3. When replicating to non-live
  - Don't save the entity, only save content_entity_link
4. When replicating to live
  - Set to default revision
  - Set an arbritray property on the entity object so that storage knows to
    not create a new revision in this case
5. Implement more properties of the replication history field

## W. Everything access related

1. Andrei to review
2. Find a better way to handle access exceptions and testing
  - Only check access when switching workspace via forms?
  - Consider the access use case for Relaxed module

## X. Workspace cache context

1. More testing
2. Ask Wim to review

## Y. WorkspaceManager and its interface

1. Remove entityCanBelongToWorkspace() method
2. More documentation
3. Add documentation to the updateOrCreateFromEntity() method

Edit: Annotated headings and lists so that it's easier to refer to.

The title reads like a meta issue, but there's a patch here, so re-titling.

Are the animations from #32 and Dries' workflow initiative blog update still on the cards?

Assuming they are, it would be great if a user could turn them off. Over at #2316205-72: Provide a way to disable animations for a11y I'm proposing that we use the new prefers-reduced-motion media query coming in CSS level 5, so animations are progressive enhancements which respect OS/browser accessibility preferences.

Can the workspaces UI be designed to work without animations?

WCAG 2.1 introduces "Success Criterion 2.2.9 Animations From Interactions." Notably this is at WCAG level AAA so it's NOT a blocker for our a11y gate. It is however a very-very-nice-to-have.

Update: The accessibility plan/policy for supporting prefers-reduced-motion is being coordinated at #2928103: [policy, no patch] Use "prefers-reduced-motion" media query to disable animations now.

@andrewmacpherson, yep, that is still the UI that we want to end up with eventually.

However, for the initial patch that is being targeted for 8.5.0, we will skip the fancy stuff and just provide a minimal UI for switching between workspaces, most likely in the form of a simple block.

Right now we're hard at work on the "backend" part of the module so when we're ready to post patches again for the core review process we will also open all the necessary followups, including the one for the fancy UI, and link them back here.

I now see that we forgot to update this issue in quite a while, so for everyone who wants to follow the active development of the 2.x branch of the Workspace module (the one that we're targeting for inclusion in Drupal 8.5.0 as an experimental module), it's being tracked in this meta issue: #2916089: [meta] Workspace 8.x-2.x

The TL;DR version of that meta issue is that we're adding lots and lots of test coverage and try to simplify the code and also bring it to core quality standards.

After a couple of months of intensive work in the 8.x-2.x branch of the contrib module, we think we're ready to start the core review process again.

The major changes since the last patch that was posted here in #77 are:

• most of the UI parts were removed, they will be handled in a separate issue
• the local replication process has been rewritten and heavily simplified:
  • it now follows the model of the CPS contrib module
  • the sequence index and the changes factory were removed because their main use case was for remote replication
  • the 'workspace' base field attached to each revisionable and publishable entity type in core was removed
• the three main entry points that the modules needs to work properly (entity load, entity queries and views queries) have been re-written with complete test coverage
• the entire code base has been documented and brought up to core standards

If anyone is interested in more details about these changes, they can be found in #2916089: [meta] Workspace 8.x-2.x and all its child issues.

This patch depends on two other core patches, #2929835: [regression] Modules can no longer alter the the table queue of a \Drupal\views\Plugin\views\query\Sql query object and #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002), so I'm posting a "combined" patch as well to see if we have any unexpected test failures.

Needless to say, the "for review" patch is now ready for architectural reviews! :)

I think I got all of them, not sure about InstallUninstallTest because that test refuses to finish on my laptop..

I just want to thank the Workflow Initiative team, @amateescu, @timmillwood, @jeqq and everyone else who has peer-reviewed and challenged all of our ideas along the way! Thank you all!

When reviewing this patch that Andrei just posted, it's difficult to immediately see or understand the vast amount of iterations this solution has gone through over the past year or so, starting with the 8.x-1.x branch of the contrib modules almost 2 years ago...

Kudos to everyone! :)

A few updates today to tackle some UI review points from #2910673: Workspace UI usability review:

• Add a link to the module's configuration page
• Rename 'update' to 'refresh' in the workspace deployment form
• Add back a minimal workspace switcher implementation via toolbar links.

Fixed that problem. When the module is being uninstalled and all the workspace entities are deleted, we don't have any active workspace anymore.

I'm going to post mostly nitpicks and question - sorry if that is not what you're looking for right now.

There's 4 coding standards errors introduced by this patch, all of them are in WorkspaceResourceTestBase, see https://www.drupal.org/pift-ci-job/838505

This is a review of #85. I noticed a couple of @todos in the code, they don't have followups, are they supposed to be fixed in this issue or at a later point?

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -730,7 +730,7 @@ public function testPost() {
   -    $parseable_valid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity(), static::$format);
   +    $parseable_valid_request_body_2 = $this->serializer->encode($this->getNormalizedPostEntity(1), static::$format);
   

   I don't understand why this is needed, this doesn't look like it should be part of this patch.

2. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,134 @@
   +  public function setHistory(array $history) {
   +    $histories = array_merge([$history], $this->getHistory());
   +    $this->set('history', $histories);
   +    return $this;
   +  }
   

   setHistory isn't really the correct name here, I think? It doesn't set history, it appends history. So, addHistory? I'm also not sure why we wrap $history in an additional array. That doesn't feel right, maybe inline documentation can help with that though.

3. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,134 @@
   +    else {
   +      return static::create(['id' => $id]);
   +    }
   

   We already do an early return, so this else is useless. There was also discussion about disallowing assignment in an if-statement in the coding standards queue recently.

   This should do the same, but looks more readable to me, but that's purely preference, feel free to ignore.

   $entity = static::load($id);
   if ($entity === FALSE) {
     $entity = static::create(['id' => $id]);
   }
   
   return $entity;

4. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,198 @@
   +  public function getRepositoryHandlerPlugin() {
   +    if (($upstream = $this->upstream->value) && $upstream !== RepositoryHandlerInterface::EMPTY_VALUE) {
   +      return \Drupal::service('plugin.manager.workspace.repository_handler')->createInstance($upstream);
   +    }
   +  }
   

   This does not conform to the interface, the interface says that this will always return something, but it looks like this doesn't. We should either change the interface or change this to always return a handler.

5. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,198 @@
   +   * @return array
   +   *   An array of default values.
   

   We can make the docs more specific here, this is always an array that contains one item, the current user id.

   @return int[]
     An array containing the current users id.
   

6. +++ b/core/modules/workspace/src/EntityAccess.php
   @@ -0,0 +1,210 @@
   +   * Constructs a new EntityAccess.
   

   Constructs a new EntityAccess instance. ?

7. +++ b/core/modules/workspace/src/EntityAccess.php
   @@ -0,0 +1,210 @@
   +   * Note: This approach assumes that a site will have only a small number
   +   * of workspace entities, under a dozen. If there are many dozens of
   +   * workspaces defined then this approach will have scaling issues.
   

   This can be wrapped closer to 80 cols.

8. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
   @@ -0,0 +1,154 @@
   +    $workspace = $this->entity;
   +    $upstream_label = $workspace->getRepositoryHandlerPlugin()->getLabel();
   ...
   +      '#url' => $this->entity->toUrl('collection'),
   

   I'm not sure if the intermedairy variable $workspace is needed. But if it is, the #url should also use that variable.

9. +++ b/core/modules/workspace/src/Plugin/Deriver/LocalWorkspaceRepositoryHandlerDeriver.php
   @@ -0,0 +1,56 @@
   +   * Constructs a new LocalWorkspaceRepositoryHandlerDeriver..
   

   should be only one period.

10. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,239 @@
    +  public function getActiveWorkspace($object = FALSE) {
    

    This doesn't apply to the interface I think, it looks like this function can return void.

@borisson_, we want to get as many eyes as we can on this patch, so every bit of review helps! So thank you for starting it! :)

There's 4 coding standards errors introduced by this patch, all of them are in WorkspaceResourceTestBase, see https://www.drupal.org/pift-ci-job/838505

Those 4 coding standards errors are actually false positives, not really sure how that can happen tbh. This is the code in question, and it's perfectly valid according to our CS. The errors are on the 4 lines with the break; statements.

    switch ($method) {
      case 'GET':
        return "The 'view workspace layla' permission is required.";
        break;
      case 'POST':
        return "The 'create workspace' permission is required.";
        break;
      case 'PATCH':
        return "The 'edit workspace layla' permission is required.";
        break;
      case 'DELETE':
        return "The 'delete workspace layla' permission is required.";
        break;
    }

I noticed a couple of @todos in the code, they don't have followups, are they supposed to be fixed in this issue or at a later point?

A few of them are already tracked in the issue queue of the contrib module, for example we still have #2927169: Fix views queries open for finishing the views query alter implementation, #2931700: Allow workspaces to be deleted for allowing workspaces to be deleted, etc. So I would say that all the @todos will be handled in this patch, by working through the open issues from this list: https://www.drupal.org/project/issues/workspace?version=8.x-2.x-dev

1. That change is needed here because the base REST test class assumes that all content entity types have sequential IDs, so when \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPost() wants to check the creation of duplicate entities (search for // 500 when creating an entity with a duplicate UUID. in that class), entity types which don't have their IDs generated automatically by the storage layer will raise an exception because the value of their ID field is NULL.
2. Nice catch! Renamed the method and documented why we need to wrap the passed-in values in another array.
3. I also think your suggestion looks cleaner :) Fixed.
4. We can not change it to always return a handler because the default (Live) workspace usually does not have a "parent"/upstream workspace. Fixed the documentation in the interface.
5. Fixed.
6. Fixed.
7. Fixed.
8. You're right, the intermediary variable is not needed. Fixed.
9. Fixed.
10. This is an excellent point, and a problem that I've thinking about for quite a while. The default workspace should *always* be available, even when it's not present in the database. Not yet sure what's the best solution, but I'm considering hard-coding the default workspace in the storage directly, so it can be returned as the fallback active workspace at any point in time, even when uninstalling the module.

This is an excellent point, and a problem that I've thinking about for quite a while. The default workspace should *always* be available, even when it's not present in the database. Not yet sure what's the best solution, but I'm considering hard-coding the default workspace in the storage directly, so it can be returned as the fallback active workspace at any point in time, even when uninstalling the module.

What we do for facets when no facet source is available is something similar, I think. http://cgit.drupalcode.org/facets/tree/src/Entity/Facet.php#n720, we don't save that object but returning at least an object there makes sure the rest of the code doesn't have to do constant null checking.

So I guess returning a default, with default values is a good idea.

Not sure about those coding standards, it looks solid to me as well. Should we open a new issue in the coder queue for this?

This fixes #90.10 by making the workspace manager always return an entity object for the active workspace.

As for the bogus coding standards errors, yep, an issue in the code queue sounds good :)

This review was for #90, I hope not too much changed

1. +++ b/core/modules/workspace/src/Annotation/RepositoryHandler.php
   @@ -0,0 +1,55 @@
   +/**
   + * Defines a RepositoryHandler annotation object.
   ...
   +class RepositoryHandler extends Plugin {
   

   🔧 What about adding a @see to the interface, which seems to be the central place where people can read more about it.

2. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,71 @@
   + * @ContentEntityType(
   + *   id = "content_workspace",
   ...
   +class ContentWorkspace extends ContentEntityBase {
   

   ❓I'm curious whether we can improve the name. Maybe something like content_workspace_assosiation. The name, at least for me, doesn't communicate its uses.

3. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,71 @@
   + * @internal
   + *   This entity is marked internal because it should not be used directly to
   + *   alter the workspace an entity belongs to.
   + */
   

   👍Nice!

4. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,71 @@
   +    $fields['content_entity_type_id'] = BaseFieldDefinition::create('string')
   ...
   +    $fields['content_entity_id'] = BaseFieldDefinition::create('integer')
   ...
   +    $fields['content_entity_revision_id'] = BaseFieldDefinition::create('integer')
   

   ❓Should these fields actually have some constraints ensuring that these values are valid? Maybe a constraint on the entity level would be needed.

5. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,134 @@
   +/**
   + * The replication log entity type.
   ...
   +class ReplicationLog extends ContentEntityBase implements ReplicationLogInterface {
   
   +++ b/core/modules/workspace/src/ReplicationLogInterface.php
   @@ -0,0 +1,76 @@
   +/**
   + * Defines an interface for the Replication log entity type.
   + */
   +interface ReplicationLogInterface extends ContentEntityInterface {
   

   🔧 It would be nice to communicate what this is used for

6. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,134 @@
   +   */
   +  public function setHistory(array $history) {
   +    $histories = array_merge([$history], $this->getHistory());
   +    $this->set('history', $histories);
   +    return $this;
   +  }
   

   🔧 The actual method appends entries, so what about naming the function: appendHistory

7. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,134 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function loadOrCreate($id) {
   
   +++ b/core/modules/workspace/src/ReplicationLogInterface.php
   @@ -0,0 +1,76 @@
   +  /**
   +   * Loads an existing replication log or creates one if necessary.
   +   *
   +   * @param string $id
   +   *   The ID of the replication log entity.
   +   *
   +   * @return $this
   +   */
   +  public static function loadOrCreate($id);
   

   I'm curious, does it really makes sense to have a static method on an interface, given you'll always call it without an instance?

8. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,198 @@
   +      ->setLabel(new TranslatableMarkup('Workaspace ID'))
   

   🙃 I love this typo: "Hey I need some sleep. I just worked a space ton of stuff."

9. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,198 @@
   +      ->addPropertyConstraints('value', ['Regex' => ['pattern' => '/^[\da-z_$()+-\/]*$/']]);
   

   ❓This is more than the usual machine name stuff, any particular reason for that? Maybe adding some comment simply clarifies that.

10. +++ b/core/modules/workspace/src/Entity/Workspace.php
    @@ -0,0 +1,198 @@
    +   * {@inheritdoc}
    +   */
    +  public function getRepositoryHandlerPlugin() {
    +    if (($upstream = $this->upstream->value) && $upstream !== RepositoryHandlerInterface::EMPTY_VALUE) {
    +      return \Drupal::service('plugin.manager.workspace.repository_handler')->createInstance($upstream);
    +    }
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getLocalRepositoryHandlerPlugin() {
    +    return \Drupal::service('plugin.manager.workspace.repository_handler')->createInstance('local_workspace:' . $this->id());
    +  }
    +
    

    ❓I'm curious, could we leverage the plugin collection code out there?

11. +++ b/core/modules/workspace/src/EntityAccess.php
    @@ -0,0 +1,210 @@
    +    return AccessResult::allowedIfHasPermission($account, 'bypass entity access workspace ' . $active_workspace->id())
    +      ->orIf(
    +        AccessResult::allowedIf($active_workspace->getOwnerId() == $account->id())
    +          ->andIf(AccessResult::allowedIfHasPermission($account, 'bypass entity access own workspace'))
    +      );
    

    ... This owner permission makes things cacheable by user. I don't see this reflected yet. See #2809177: Introduce entity permission providers and related PR on the entity github issue.

12. +++ b/core/modules/workspace/src/EntityAccess.php
    @@ -0,0 +1,210 @@
    +    foreach ($this->entityTypeManager->getStorage('workspace')->loadMultiple() as $workspace) {
    ...
    +   */
    +  protected function createWorkspaceViewPermission(WorkspaceInterface $workspace) {
    +    $perms['view workspace ' . $workspace->id()] = [
    +      'title' => $this->t('View the %workspace workspace', ['%workspace' => $workspace->label()]),
    +      'description' => $this->t('View the %workspace workspace and content within it', ['%workspace' => $workspace->label()]),
    +    ];
    +
    

    ❓So do I understand this correctly that this introduces three permissions by workspace, which itself is a content entity, potentially used per editor or even more? It feels like the permission system is not designed to handle that amount of permissions to be honest. You could also add concerns for adding content dependencies in the user.roles.* configurations.

13. +++ b/core/modules/workspace/src/EntityQuery/QueryAggregate.php
    @@ -0,0 +1,27 @@
    +class QueryAggregate extends BaseQueryAggregate {
    

    It is a bit counterintuitive that for aggregations we just need to add the joins, but no further expression. Maybe some documentation helps to clarify this.

14. +++ b/core/modules/workspace/src/EntityQuery/QueryTrait.php
    @@ -0,0 +1,73 @@
    +    $active_workspace = $this->workspaceManager->getActiveWorkspace();
    +    if ($active_workspace !== WorkspaceManager::DEFAULT_WORKSPACE && $this->workspaceManager->entityTypeCanBelongToWorkspaces($this->entityType)) {
    

    What about adding a activeWorkspaceIsDefault helper. This seems to be useful not just here.

15. +++ b/core/modules/workspace/src/EntityQuery/SqlQueryFactory.php
    @@ -0,0 +1,53 @@
    +  public function get(EntityTypeInterface $entity_type, $conjunction) {
    +    $class = QueryBase::getClass($this->namespaces, 'Query');
    +    return new $class($entity_type, $conjunction, $this->connection, $this->namespaces, $this->workspaceManager);
    +  }
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getAggregate(EntityTypeInterface $entity_type, $conjunction) {
    +    $class = QueryBase::getClass($this->namespaces, 'QueryAggregate');
    +    return new $class($entity_type, $conjunction, $this->connection, $this->namespaces, $this->workspaceManager);
    +  }
    

    ❓Could we check here whether we have a non default workspace and otherwise return to the core implementations?

16. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,154 @@
    +    // We can not deploy if we do not have an upstream workspace.
    +    if (!$workspace->getRepositoryHandlerPlugin()) {
    +      throw new BadRequestHttpException();
    +    }
    +
    

    I'm curious whether we could send along some helpful message here. On top of that I'm also curious: Is a 400 exception, aka. a client error, semantically really the right thing?

17. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,154 @@
    +      $repository_handler = $workspace->getRepositoryHandlerPlugin();
    +      $repository_handler->replicate($workspace->getLocalRepositoryHandlerPlugin(), $repository_handler);
    +      $this->messenger->addMessage($this->t('Successful deployment.'));
    
    +++ b/core/modules/workspace/src/RepositoryHandlerBase.php
    @@ -0,0 +1,61 @@
    +abstract class RepositoryHandlerBase extends PluginBase implements RepositoryHandlerInterface {
    ...
    +  /**
    +   * Replicates content from a source repository to a target repository.
    +   *
    +   * @param \Drupal\workspace\RepositoryHandlerInterface $source
    +   *   The repository handler to replicate from.
    +   * @param \Drupal\workspace\RepositoryHandlerInterface $target
    +   *   The repository handler to replicate to.
    +   *
    +   * @return \Drupal\workspace\ReplicationLogInterface
    +   *   The replication log for the replication.
    +   */
    +  abstract public function replicate(RepositoryHandlerInterface $source, RepositoryHandlerInterface $target);
    

    ❓There seems to be a reason why this method is not on the interface. For me at least though is not obvious, given that its actually called directly.

18. +++ b/core/modules/workspace/src/Form/WorkspaceForm.php
    @@ -0,0 +1,158 @@
    +    \Drupal::service('plugin.manager.workspace.repository_handler')->clearCachedDefinitions();
    

    Maybe comment why this cache clearing is needed.

19. +++ b/core/modules/workspace/src/Negotiator/SessionWorkspaceNegotiator.php
    @@ -0,0 +1,74 @@
    +  public function applies(Request $request) {
    +    // This negotiator only applies if the current user is authenticated,
    +    // i.e. a session exists.
    +    return $this->currentUser->isAuthenticated();
    +  }
    

    Theoretically a anonymous user might have a session. I guess its a fair limitatation to not allow anonymous users to access different workspaces. Otherwise page cache might be way more tricky.

20. +++ b/core/modules/workspace/src/Negotiator/SessionWorkspaceNegotiator.php
    @@ -0,0 +1,74 @@
    +   * {@inheritdoc}
    +   */
    ...
    +    $workspace_id = $this->tempstore->get('active_workspace_id');
    +    return $workspace_id ?: WorkspaceManager::DEFAULT_WORKSPACE;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    ...
    +    $this->tempstore->set('active_workspace_id', $workspace->id());
    +  }
    +
    

    Is tempstore really the right place? I would have put it directly onto the session, given that this plugin talks about sessions in the first place. Some nitpick about the method names. What about adding 'active' to them?

21. +++ b/core/modules/workspace/src/Plugin/Field/FieldWidget/WorkspaceUpstreamWidget.php
    @@ -0,0 +1,130 @@
    +      foreach ($upstream_plugin_definitions as $plugin_id => $plugin_definition) {
    +        // Do not include the local workspace itself as an option.
    +        if ($plugin_id !== 'local_workspace' . PluginBase::DERIVATIVE_SEPARATOR . $items->getEntity()->id()) {
    +          $upstream_options[$category][$plugin_id] = $plugin_definition['label'];
    +        }
    +      }
    

    Why is this not using the isRemote method/annotation

22. +++ b/core/modules/workspace/src/Plugin/RepositoryHandler/LocalWorkspaceRepositoryHandler.php
    @@ -0,0 +1,257 @@
    +    $session_id = \md5((\microtime(TRUE) * 1000000));
    

    Let's use sha256, otherwise some people will complain :) Note: I don't really see a reason for the * 10000, it feels like you basically want to generate a random unique string. Don't we have better helper methods for that?

23. +++ b/core/modules/workspace/src/Plugin/Validation/Constraint/UpstreamValidator.php
    @@ -0,0 +1,34 @@
    +    if (!$entity->isNew() && $value === 'local_workspace' . PluginBase::DERIVATIVE_SEPARATOR . $entity->id()) {
    +      $this->context->addViolation($constraint->message);
    +    }
    

    ❓This seems to not protect against source and target being both remote workspaces, but both the same

24. +++ b/core/modules/workspace/src/WorkspaceAccessControlHandler.php
    @@ -0,0 +1,53 @@
    +   */
    +  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    +    /** @var \Drupal\workspace\WorkspaceInterface $entity */
    ...
    +      ->orIf(
    +        AccessResult::allowedIf($entity->getOwnerId() == $account->id())->addCacheableDependency($entity)
    +          ->andIf(AccessResult::allowedIfHasPermission($account, $operations[$operation]['own']))
    +      )
    

    As written in the other place, we need additional cacheable dependencies ...

25. +++ b/core/modules/workspace/src/WorkspaceCacheContext.php
    @@ -0,0 +1,53 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getContext() {
    +    return $this->workspaceManager->getActiveWorkspace();
    +  }
    ...
    +  public function getCacheableMetadata($type = NULL) {
    +    return new CacheableMetadata();
    +  }
    

    Shouldn't the workspace negotiation inform the system how they determined this value? For example the session one depend on the current active session.

26. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,239 @@
    +    $this->blacklist[] = $entity_type->id();
    +    return FALSE;
    

    Should we key this value by the entity_type->id() as well, as otherwise this array just grows over time?

27. +++ b/core/modules/workspace/src/WorkspaceManager.php
    @@ -0,0 +1,239 @@
    +    // @todo Could this be handled better?
    +    if (!$workspace->access('view') && ($workspace->id() != static::DEFAULT_WORKSPACE)) {
    +      $this->logger->error('Denied access to view workspace {workspace}', ['workspace' => $workspace->label()]);
    +      throw new WorkspaceAccessException('The user does not have permission to view that workspace.');
    +    }
    

    Good question. Maybe access checking should be done explicitly using its own method?

28. +++ b/core/modules/workspace/tests/src/Functional/EntityResource/WorkspaceResourceTestBase.php
    @@ -0,0 +1,214 @@
    +abstract class WorkspaceResourceTestBase extends EntityResourceTestBase {
    

    Is there a follow up to allow deploying using a REST api?

29. +++ b/core/modules/workspace/tests/src/Functional/WorkspaceBypassTest.php
    @@ -0,0 +1,127 @@
    +    $session = $this->getSession();
    +    $this->assertEquals(403, $session->getStatusCode());
    ...
    +    $session = $this->getSession();
    +    $this->assertEquals(200, $session->getStatusCode());
    ...
    +    $session = $this->getSession();
    +    $this->assertEquals(403, $session->getStatusCode());
    
    +++ b/core/modules/workspace/tests/src/Functional/WorkspaceIndividualPermissionsTest.php
    @@ -0,0 +1,97 @@
    +    $this->drupalGet("/admin/config/workflow/workspace/{$bears->id()}/edit");
    +    $this->assertEquals(200, $session->getStatusCode());
    ...
    +    $this->drupalGet("admin/config/workflow/workspace/{$bears->id()}/activate");
    +    $this->assertEquals(200, $session->getStatusCode());
    ...
    +    $this->drupalGet("admin/config/workflow/workspace/{$packers->id()}/activate");
    +    $this->assertEquals(403, $session->getStatusCode());
    

    🔧 You could also use $this->assertSession()->statusCodeEquals(403)

30. +++ b/core/modules/workspace/tests/src/Functional/WorkspaceTestUtilities.php
    @@ -0,0 +1,187 @@
    +/**
    + * Utility methods for use in BrowserTestBase tests.
    + *
    + * This trait will not work if not used in a child of BrowserTestBase.
    + */
    +trait WorkspaceTestUtilities {
    

    👏

31. +++ b/core/modules/workspace/workspace.module
    @@ -0,0 +1,685 @@
    +  // Don't alter the loaded entities if the entity type can not belong to a
    +  // workspace.
    +  if (!$workspace_manager->entityTypeCanBelongToWorkspaces($entity_type_manager->getDefinition($entity_type_id))) {
    +    return;
    +  }
    +
    +  // Don't alter the loaded entities if the active workspace is the default one.
    +  $active_workspace = $workspace_manager->getActiveWorkspace();
    +  if ($active_workspace == WorkspaceManager::DEFAULT_WORKSPACE) {
    +    return;
    +  }
    ...
    +  // Only run if the entity type can belong to a workspace and we are in a
    +  // non-default workspace.
    +  if (!$workspace_manager->entityTypeCanBelongToWorkspaces($entity->getEntityType())
    +     || $workspace_manager->getActiveWorkspace() === WorkspaceManager::DEFAULT_WORKSPACE) {
    +    return;
    +  }
    

    🙃 These two patterns look totally different but do the same.

32. +++ b/core/modules/workspace/workspace.module
    @@ -0,0 +1,685 @@
    +function _workspace_views_query_alter_entity_type(ViewExecutable $view, Sql $query, EntityTypeInterface $entity_type) {
    

    I didn't had yet the mental capacity to review this.

33. +++ b/core/modules/workspace/workspace.routing.yml
    @@ -0,0 +1,27 @@
    +    _permission: 'administer workspaces+edit any workspace'
    

    👍 I really like this permission name

34. +++ b/core/modules/workspace/workspace.services.yml
    @@ -0,0 +1,26 @@
    +  plugin.manager.workspace.repository_handler:
    +    class: Drupal\workspace\RepositoryHandlerManager
    +    parent: default_plugin_manager
    

    Given that its plugin IDs depends on content entities (which is a bit weird to be honest), I think we should try to use a different cache bin, given that otherwise we might have a lot of entries in the APCU cache. There are various issues out there related with that.

35. +++ b/core/modules/workspace/workspace.services.yml
    @@ -0,0 +1,26 @@
    +  logger.channel.workspace:
    +    parent: logger.channel_base
    +    arguments: ['cron']
    

    ❌ This should be 'workspace' instead of 'cron'

Whoa, that's quite an awesome review! I fixed most of the points in this commit from the 8.x-2.x branch of the contrib module, but ran out of steam for now so I'll post a more detailed reply and a new patch tomorrow :)

This is looking really great!

One question/comment:

1. +++ b/core/modules/workspace/workspace.module
   @@ -0,0 +1,678 @@
   +function workspace_entity_load(array &$entities, $entity_type_id) {
   ...
   +function workspace_entity_presave(EntityInterface $entity) {
   ...
   +function workspace_entity_insert(EntityInterface $entity) {
   ...
   +function workspace_entity_update(EntityInterface $entity) {
   ...
   +function workspace_views_query_alter(ViewExecutable $view, QueryPluginBase $query) {
   ...
   +function workspace_rest_resource_alter(&$definitions) {
   ...
   +function workspace_toolbar() {
   

   Can the heavy lifting in these hooks be moved to a class (or classes, as makes sense), similar to the way workspace_entity_access is already doing? The content_moderation module (and I think workflows) does this extensively...

2. +++ b/core/modules/workspace/workspace.module
   @@ -0,0 +1,678 @@
   +function _workspace_views_query_alter_entity_type(ViewExecutable $view, Sql $query, EntityTypeInterface $entity_type) {
   ...
   +function _workspace_ensure_content_workspace_table($entity_type_id, Sql $query, $relationship) {
   ...
   +function _workspace_ensure_revision_table(EntityTypeInterface $entity_type, Sql $query, $relationship) {
   ...
   +function _workspace_get_revision_table_join($relationship, $table, $field, $content_workspace_table) {
   ...
   +function _workspace_move_entity_table(Sql $query, $content_workspace_table, $alias) {
   

   That way, all these private functions could be moved into the respective classes as protected methods, which will make for easier testing...

This is looking awesome, huge thumbs up to the team who worked on this!

1. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,71 @@
   + * @internal
   + *   This entity is marked internal because it should not be used directly to
   + *   alter the workspace an entity belongs to.
   

   It has come up before that adding @internal isn't currently enough to fully allow this to change/be removed, if that's the intention. Maybe worth a link off to https://www.drupal.org/node/2917276?

2. +++ b/core/modules/workspace/src/Entity/ContentWorkspace.php
   @@ -0,0 +1,71 @@
   +    $fields['content_entity_type_id'] = BaseFieldDefinition::create('string')
   +      ->setLabel(t('Content entity type ID'))
   +      ->setDescription(t('The ID of the content entity type this workspace is for.'))
   +      ->setRequired(TRUE)
   +      ->setRevisionable(TRUE);
   

   This should have "max_length" set to EntityTypeInterface::ID_MAX_LENGTH. This becomes a huge pain to write an update hook for and is important if you start adding indexes to these fields.

3. +++ b/core/modules/workspace/src/Form/WorkspaceForm.php
   @@ -0,0 +1,158 @@
   +      $redirect = $this->currentUser()->hasPermission('administer workspaces') ? $workspace->toUrl('collection') : Url::fromRoute('<front>');
   

   We can check toUrl('collection')->access() instead of using the permission.

4. +++ b/core/modules/workspace/src/Form/WorkspaceSwitcherForm.php
   @@ -0,0 +1,146 @@
   +    /** @var \Drupal\workspace\WorkspaceInterface $workspace */
   +    $workspace = $this->entityTypeManager->getStorage('workspace')->load($id);
   

   Is there a reason Workspace::load isn't good enough here? Would this help with the hinting too?

5. +++ b/core/modules/workspace/src/WorkspaceManager.php
   @@ -0,0 +1,234 @@
   +      && is_a($entity_type->getClass(), EntityPublishedInterface::class, TRUE)
   

   Can we use $entity_type->entityClassImplements() here?

6. +++ b/core/modules/workspace/src/WorkspaceServiceProvider.php
   @@ -0,0 +1,30 @@
   +    // Switch core's SQL entity query factory to our own so we can reliably
   +    // alter entity queries.
   +    // @todo Do the same for the pgsql entity query backend override.
   +    $definition = $container->getDefinition('entity.query.sql');
   +    $definition->setClass('Drupal\workspace\EntityQuery\SqlQueryFactory');
   +    $definition->addArgument($container->getDefinition('workspace.manager'));
   

   Recently reviewed #2835542: Utilize service decorator instead of taking over entire EFQ service. I wonder this should attempt to decorate whatever is already in the container instead of extending the existing class. Two modules trying to replace this service does rare enough to not be concerned about this for now.

7. +++ b/core/modules/workspace/workspace.install
   @@ -0,0 +1,47 @@
   +  // Set the owner of these default workspaces to be first user which which has
   +  // the 'administrator' role. This way we avoid hard coding user ID 1 for sites
   +  // that prefer to not give it any special meaning.
   +  $admin_roles = \Drupal::entityTypeManager()->getStorage('user_role')->getQuery()
   +    ->condition('is_admin', TRUE)
   +    ->execute();
   +  if (!empty($admin_roles)) {
   +    $query = \Drupal::entityTypeManager()->getStorage('user')->getQuery()
   +      ->condition('roles', $admin_roles, 'IN')
   +      ->condition('status', 1)
   +      ->sort('uid', 'ASC')
   +      ->range(0, 1);
   +    $result = $query->execute();
   +  }
   

   Are there any use cases which might depend on anonymously owned workspaces? Are we doing this so user "0" never owns a workspace? Would would be the impact of that?

8. +++ b/core/modules/workspace/workspace.module
   @@ -0,0 +1,678 @@
   +/**
   + * Implements hook_entity_load().
   + */
   +function workspace_entity_load(array &$entities, $entity_type_id) {
   

   It would be nice to be consistent with hook bridges, but 100% not a blocker.

9. +++ b/core/modules/workspace/workspace.module
   @@ -0,0 +1,678 @@
   +    $entity->_initialPublished = TRUE;
   

   There was an issue somewhere related to formalising the use of properties on entities stored for a single request, but I can't seem to track it down.

10. +++ b/core/modules/workspace/workspace.module
    @@ -0,0 +1,678 @@
    +function workspace_views_query_alter(ViewExecutable $view, QueryPluginBase $query) {
    ...
    +function _workspace_views_query_alter_entity_type(ViewExecutable $view, Sql $query, EntityTypeInterface $entity_type) {
    ...
    +function _workspace_ensure_content_workspace_table($entity_type_id, Sql $query, $relationship) {
    ...
    +function _workspace_ensure_revision_table(EntityTypeInterface $entity_type, Sql $query, $relationship) {
    ...
    +function _workspace_get_revision_table_join($relationship, $table, $field, $content_workspace_table) {
    ...
    +function _workspace_move_entity_table(Sql $query, $content_workspace_table, $alias) {
    

    The views stuff easily feels like the most complex part of the module. I wonder if there is a way to pull this out into some hook bridges or a views.inc file to make it a bit more manageable. It seems like testing this thoroughly could also be such a huge undertaking, that it's almost worth delegating to a follow up?

@jhedstrom and @Sam152, thank you for taking a look as well!

Let's get the easy stuff out of the way first :)

Re #95:

1. Sure, added the usual @see mentions to that annotation class.

2. Yup, we wanted to rename this entity type for quite a while, but let's figure out what's the best name for it because there's a lot of code which needs to be updated.

Your suggestion is content_workspace_assosiation, and we were thinking about content_workspace_link or workspace_content_link because that signals a bit better the fact that this entity type is what links a piece of content to a workspace.

Any other suggestions are welcome!

3. Nice and clear ;)

4. I don't think those fields from the content_workspace entity type need either individual nor composite validation. There is no form exposed for that entity type and it is excluded from REST support which means there no code path which would call validate() on it.

5. Of course, added more documentation to ReplicationLogInterface.

6. That method was already renamed in #91 to addHistory(), but I like your suggestion better so renamed it again to appendHistory().

7. I think it makes sense to have that static method on the interface because if someone swaps the entity class and doesn't extend the existing one, they still need to implement the interface, including that method.

8. How did you spot that?! That typo was sitting there since December 2015 :D Fixed!

9. Just discussed this point with @timmillwood earlier today and he told me that initial reason for that unusual regex validation was to conform to the CouchDB specification for allowed characters in a database name. However, it makes more sense to use the more standard Drupal machine name pattern since the local replication that is being added to core is not really tied to the CouchDB protocol in any way.

10. Yup, we're actively working on updating that part of the code in #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace, and the new config entity type does use the plugin collection pattern.

11. Right, fixed :)

12. You are right, and just above the code you referenced there's a comment saying that the current permission model is designed for a handful of workspaces and that it doesn't scale to dozens of workspaces. I'm not yet sure what we can do about this, maybe we should have a workspace_permission submodule (or in contrib) which provides a different model than the existing "3 permissions per workspace".

However, adding content entity dependencies to user.roles.* configuration is a very good point. We are actively discussing if we should change the Workspace entity type back to a config entity, like it was initially. Stay tuned for an update on this :)

13. 15. and #98.6: The entity query stuff will be handled together in a followup comment/patch.

14. Heh, I had the same thought and I added a isDefaultWorkspace() helper method in #94.

16. You're right, a 4xx status code is not correct for that specific problem, changed it to a 500 status code and added a message describing the problem.

17. There is no reason for that method to not be on the interface, so I moved it there :)

18. That cache clearing was totally not needed, removed!

19. Right, I removed the part of the comment which was mentioning sessions.

20. Yes, I think tempstore is a good place for persistent storage of the active workspace. Imagine a content editor logging out at the end of the working day, and then logging back in the next morning. Storing it in the tempstore instead of the actual PHP session allows them to get back instantly to the same editorial state from the previous work session.

21. We don't use isRemote() there because there might be multiple "local" implementations of a repository handler, for example "local_couchdb" or something like that, and that code only wants to remove the workspaces from the local Drupal installation.

22. Yeah, I also thought that was weird, I just didn't get to fix it until now. The CouchDB specification says that the session_id of a replication log entry is an UUID, so I simply changed the field type and implementation to use an actual UUID :)

23. That's because the source is *always* a local workspace.

24. Fixed as well by adding cachePerUser().

25. I don't think it needs to do that. Theme negotiator have a very similar pattern for the negotiation and yet \Drupal\Core\Cache\Context\ThemeCacheContext::getCacheableMetadata() return an empty CacheableMetadata() just like we do.

26. That array can't grow over time because the first check that we do in that method is a in_array() :)

27. Not sure what you mean there, where would that method live and what would it do exactly?

28. Yes, we have #2926472: Add test for basic remote replication open for that.

29. Sure thing, fixed!

30. ;)

31. Right, changed the code in workspace_entity_load() to match the others.

32. It's actually a pretty simple concept once you read the code and the comments a few times. But I'll let you get to it in your on pace :)

33. Me too :P

34. This will also change with #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace, and there will be way less repository handler plugin definitions.

35. Oops, copy/pasta fail :) Fixed!

Re #97 and #98.8 and .10: Sure thing, we'll change that code to follow the hook bridge approach in a followup comment/patch.

Re #98:

1. This entity type is way less risk-prone to "intrusion" than the one from Content Moderation because there is no user-facing code which can interact with it, everything happens behind the scenes. While in CM, a user can actually "act" on the content_moderation_state entity type through the moderation_state base field from each moderated entity type.

Since we're still in pre-alpha (actually, pre-initial commit stage), we'll need to discuss whether we want to keep the "link" entity type approach or move it to custom database tables before we reach beta status.

2. Good spot! Fixed.

3. Sure, good idea. Fixed.

4. Because I wanted to use the injected services more. In that form we don't really need the entity type manager for anything else so I changed the local variable to simply store the workspace storage object.

5. Of course :) Fixed.

6. See above, will be handled in a followup comment.

7. Yup, we're doing all that just so a workspace is never owned by the anonymous user. I'm not really sure of the impact of that scenario, but I simply wanted to don't have to think about it :P

8. and 10. See above.

9. That would be super nice to have, let's hope we find that issue at some point :)

In conclusion, the following points were left for a followup comment/patch:

#95: 2, 10, 12, 13, 15, 27, 28, 34.
#97 and #98: 6, 8, 10.

You are right, and just above the code you referenced there's a comment saying that the current permission model is designed for a handful of workspaces and that it doesn't scale to dozens of workspaces. I'm not yet sure what we can do about this, maybe we should have a workspace_permission submodule (or in contrib) which provides a different model than the existing "3 permissions per workspace".

However, adding content entity dependencies to user.roles.* configuration is a very good point. We are actively discussing if we should change the Workspace entity type back to a config entity, like it was initially. Stay tuned for an update on this :)

I'm actually fine with workspaces being content entities, I just don't believe in adding permissions by default being useful. I think there is an obvious space for contrib modules/custom workflows providing more features. Note: it is not impossible to actually have content entities dependencies (see block_content module as example), though, as far as I understood it, workspaces are something you would throw away after you've done a specific bit of content work.
These kind of ideas (how should editors actually use it) would be ideally documented already, maybe in the issue summary, or on the handbook page.

14. Heh, I had the same thought and I added a isDefaultWorkspace() helper method in #94.

👍

20. Yes, I think tempstore is a good place for persistent storage of the active workspace. Imagine a content editor logging out at the end of the working day, and then logging back in the next morning. Storing it in the tempstore instead of the actual PHP session allows them to get back instantly to the same editorial state from the previous work session.

Good point. Maybe document this particular usecase in the code? Not sure whether this is too specific though.

. I don't think it needs to do that. Theme negotiator have a very similar pattern for the negotiation and yet \Drupal\Core\Cache\Context\ThemeCacheContext::getCacheableMetadata() return an empty CacheableMetadata() just like we do.

You know, this could be simply a bug?

I think there is an obvious space for contrib modules/custom workflows providing more features.

Ok, I've added a @todo in my list to open an issue to discuss whether we want to keep the per-workspace permissions or not :)

Good point. Maybe document this particular usecase in the code? Not sure whether this is too specific though.

Good idea, I'll do that in a follow-up patch.

You know, this could be simply a bug?

I guess you're right, another @todo point added :)

In the meantime, this patch should fix the failures from #99 and make that access checking code a bit more readable.

though, as far as I understood it, workspaces are something you would throw away after you've done a specific bit of content work.

This isn't necessarily true if you wanted to support the following:
- 'revert' to the state of a previous workspace (which I don't think core should support but contrib CPS does)
- 'preview/review' of the site in a previous workspace state (ditto)
- diffs between old workspace states relative to each other (more or less ditto too, not sure if CPS supports that though)

Regardless of that though, you'd have to actively delete a workspace unless we auto-delete after publishing to avoid them accumulating, and even if we do it makes sense to me to remove the per-workspace permission from the initial patch.

I have this high up on my list to review - I have been discussing workspaces in a fair bit of depth on and off with amateescu though prior to the patch being here.

Ok, removed the per-workspace permissions for now and documented why we use the private tempstore in the session negotiator. Also, rebased the dependencies to include #2885469-93: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002), which should fix the test failure from #102.

Here's a new update with the following updates:

• Document why aggregate entity queries do not need to add any further expressions.
• Move code from hook implementations to dedicated hook bridges.
• Rename the "content_workspace" entity type to "workspace_association".

which fixes these review points: #95.2, .13, #97 and #98: 8, 10.

#95.12 has been fixed in #105.

What's left to do:
#95.10 and .34: #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace
#95.15 and #98.6: #2934352: Consider using a service decorator for the entity query alter
#95.25: #2934354: Expose cacheability metadata in WorkspaceCacheContext
#95.28: #2926472: Add test for basic remote replication

At this point, all the reviews so far were either addressed or have dedicated followups, so we're ready for more eyes on the "for-review" patch! :)

I found the issue mentioned in #98.9.

@Sam152, thanks for that link, following for now :)

#2929835: [regression] Modules can no longer alter the the table queue of a \Drupal\views\Plugin\views\query\Sql query object was committed so we no longer need to include it in the combined patch, and #2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002) changed drastically so we have a new blocker: #2935076: EntityResourceTestBase::getNormalizedPostEntity() wrongly assumes that all entity types have auto-generated sequential IDs. However, the change needed for the "for-review" patch is minimal, as can be seen in this interdiff.

Just lost a long review as I was posting it, here's a second attempt, hopefully remembered most of the points.

1. +++ b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
   @@ -93,6 +94,10 @@ public function testInstallUninstall() {
    
   +    // Delete any workspaces so the workspace module can be uninstalled.
   +    $workspaces = Workspace::loadMultiple();
   +    \Drupal::entityTypeManager()->getStorage('workspace')->delete($workspaces);
   +
   

   Do we actually create the 'live' workspace as a config entity? If not why is this necessary? If so why not hard-code it somewhere? At the moment this means a manual install and uninstall isn't possibly no?

2. +++ b/core/modules/workspace/src/Annotation/RepositoryHandler.php
   @@ -0,0 +1,60 @@
   +
   +  /**
   +   * Whether the repository handles a remote destination or not.
   +   *
   +   * @var bool
   +   */
   +  public $remote;
   +
   

   Why is this necessary? It's taking the abstraction of repositories but still embedding the concept of local workspace deployment into the API and makes me wonder about the abstraction a bit.

3. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,132 @@
   +
   +    $fields['history'] = BaseFieldDefinition::create('replication_history')
   +      ->setLabel(new TranslatableMarkup('Replication log history'))
   +      ->setDescription(new TranslatableMarkup('The version id of the test entity.'))
   +      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED);
   +
   

   c&p error, it's not a test entity.

4. +++ b/core/modules/workspace/src/Entity/ReplicationLog.php
   @@ -0,0 +1,132 @@
   +
   +    $fields['source_last_sequence'] = BaseFieldDefinition::create('string')
   +      ->setLabel(new TranslatableMarkup('Last processed checkpoint'))
   +      ->setDescription(new TranslatableMarkup('The last processed checkpoint. Shortcut to the source_last_sequence in the last history item.'));
   +
   

   The comment doesn't make it clear what this is. From reading down the patch, it's for partial deployments. For local deployments we should be doing everything in a single transaction anyway, in which case a partial deployment is impossible.

5. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,206 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getRepositoryHandlerPlugin() {
   +    if (($upstream = $this->upstream->value) && $upstream !== RepositoryHandlerInterface::EMPTY_VALUE) {
   +      return \Drupal::service('plugin.manager.workspace.repository_handler')->createInstance($upstream);
   +    }
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getLocalRepositoryHandlerPlugin() {
   +    return \Drupal::service('plugin.manager.workspace.repository_handler')->createInstance('local_workspace:' . $this->id());
   +  }
   +
   

   Again there is support for both local and general workspace handlers in the API, why is this necessary? And if it is, why are local workspaces handled by the same system?

6. +++ b/core/modules/workspace/src/Entity/WorkspaceAssociation.php
   @@ -0,0 +1,73 @@
   +      ->setDescription(new TranslatableMarkup('The ID of the content entity this workspace is for.'))
   

   'this workspace is for' seems odd, wouldn't it be 'in this workspace' or 'associated with this workspace'.

7. +++ b/core/modules/workspace/src/EntityAccess.php
   @@ -0,0 +1,174 @@
   +   *
   +   * Note: This approach assumes that a site will have only a small number of
   +   * workspace entities, under a dozen. If there are many dozens of workspaces
   +   * defined then this approach will have scaling issues.
   +   *
   

   Can we remove this?

8. +++ b/core/modules/workspace/src/EntityOperations.php
   @@ -0,0 +1,251 @@
   +
   +    // When a new published entity is inserted in a non-default workspace, we
   +    // actually want two revisions to be saved:
   +    // - An unpublished default revision in the default ('live') workspace.
   +    // - A published pending revision in the current workspace.
   +    if ($entity->isNew() && $entity->isPublished()) {
   +      // Keep track of the publishing status for workspace_entity_insert() and
   +      // unpublish the default revision.
   +      $entity->_initialPublished = TRUE;
   +      $entity->setUnpublished();
   +    }
   +  }
   

   Hello old friend..

9. +++ b/core/modules/workspace/src/EntityQuery/Query.php
   @@ -0,0 +1,62 @@
   +   */
   +  public function prepare() {
   +    $this->traitPrepare();
   +
   +    // If the prepare() method from the trait decided that we need to alter this
   +    // query, we need to re-define the the key fields for fetchAllKeyed() as SQL
   +    // expressions.
   +    if ($this->sqlQuery->getMetaData('active_workspace_id')) {
   +      $id_field = $this->entityType->getKey('id');
   +      $revision_field = $this->entityType->getKey('revision');
   +
   +      // Since the query is against the base table, we have to take into account
   +      // that the revision ID might come from the workspace_association
   +      // relationship, and, as a consequence, the revision ID field is no longer
   +      // a simple SQL field but an expression.
   +      $this->sqlFields = [];
   +      $this->sqlExpressions[$revision_field] = "COALESCE(workspace_association.content_entity_revision_id, base_table.$revision_field)";
   +      $this->sqlExpressions[$id_field] = "base_table.$id_field";
   +    }
   +
   +    return $this;
   

   Really surprisingly tidy.

10. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,152 @@
    +    // We can not deploy if we do not have an upstream workspace.
    

    Now 'target'.

11. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,152 @@
    +
    +    $upstream_label = $this->entity->getRepositoryHandlerPlugin()->getLabel();
    +
    +    $elements['submit']['#value'] = $this->t('Deploy to @upstream', ['@upstream' => $upstream_label]);
    

    Same here with upstream/target.

12. +++ b/core/modules/workspace/src/Negotiator/SessionWorkspaceNegotiator.php
    @@ -0,0 +1,85 @@
    +/**
    + * Defines the session workspace negotiator.
    + *
    + * This implementation uses the private tempstore of a user to store the ID of
    + * the active workspace in order to make it persistent between login/logout
    + * actions.
    + */
    

    This seems a bit overkill, there's not much we store between sessions.

13. +++ b/core/modules/workspace/src/Plugin/Field/FieldType/ReplicationHistoryItem.php
    @@ -0,0 +1,145 @@
    + *
    + * For each replication a history should be maintained. The only required field
    + * is session_id which is a unique ID for the replication. The recorded_sequence
    + * field is another important field, it stores the sequence ID of the last
    + * entity replicated. It is where replication is started from next time, and
    + * therefore defaults to 0, denoting to start from the first sequence ID. All
    + * other fields are for informational purposes which can be used for user
    + * messages, logs, or an audit trail.
    + *
    

    See questions above - why session ID instead of unique ID, and why is sequence ID necessary?

14. +++ b/core/modules/workspace/src/Plugin/Field/FieldType/ReplicationHistoryItem.php
    @@ -0,0 +1,145 @@
    +      'columns' => [
    +        'entity_write_failures' => [
    +          'type' => 'int',
    +          'unsigned' => TRUE,
    +          'not null' => FALSE,
    +        ],
    

    Why the logging here and not using the logger?

15. +++ b/core/modules/workspace/src/Plugin/RepositoryHandler/LocalWorkspaceRepositoryHandler.php
    @@ -0,0 +1,269 @@
    +        }
    +        else {
    +          // If the target workspace is not the default workspace, the content
    +          // workspace link entity can simply be updated with the target
    +          // workspace.
    +          $workspace_association->setNewRevision(TRUE);
    +          $workspace_association->workspace->target_id = $target_workspace->id();
    +          $workspace_association->save();
    +        }
    +      }
    

    Discussed on slack, but this is probably worth splitting out to its own dedicated plugin.

16. +++ b/core/modules/workspace/src/Plugin/RepositoryHandler/LocalWorkspaceRepositoryHandler.php
    @@ -0,0 +1,269 @@
    +
    +    // Only switch to the target workspace and save entities if there are some
    +    // to save.
    +    if (!empty($entities)) {
    +      // Before saving set the active workspace to the target.
    +      $this->workspaceManager->setActiveWorkspace($target_workspace);
    +      // Save each revision on the target workspace.
    +      foreach ($entities as $entity) {
    +        $entity->save();
    +      }
    +    }
    

    This should happen in a transaction so we can roll back if any of the entity saves fail. Additionally there currently appears to be no protection for saving items in a workspace over more recently updated entities on live - that could happen either via a pre-deploy audit or when saving but seems like we need one or the other?

17. +++ b/core/modules/workspace/src/RepositoryHandlerInterface.php
    @@ -0,0 +1,65 @@
    +  /**
    +   * Returns whether the repository handler is remote or not.
    +   *
    +   * @return bool
    +   *   TRUE if the repository handler is remote, FALSE otherwise.
    +   */
    +  public function isRemote();
    

    Why do we need to know here?

Thanks for reviewing, @catch!

Let's get rid of the easy stuff first. Re #110:

1. Yes, we create a content entity for the Live workspace, see workspace_install(). The reason for that is so that users can change and/or translate its label. I did think about hardcoding the default workspace in its storage handler, but I just didn't get to actually do it so far. Would you prefer if we do that instead of creating it in workspace_install()?

And the module can be uninstalled, there's even a test for that: WorkspaceUninstallTest :)

2. and 17.: The 'remote' definition key was meant as a helper for the repository handler base class to provide a default configuration form element for the endpoint URL for remote targets, but @timmillwood also didn't like it very much and now I realize that this could be handled better with an abstract RemoteRepositoryHandler base class. Removed the 'remote' definition and its getter.

3. Fixed.

4. Updated the description text to mention that the field is only useful for remote deployments.

5. Adding the getLocalRepositoryHandlerPlugin() method is a change that I rushed in at some point. I agree that it's wrong and will be removed in #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace.

6. Fixed.

7. Yes, we can remove that comment since we no longer provide so many per-workspace permissions. Fixed.

8. Oh hai! Long time no see..

9. Took a while to get to that tidy code, but yes, EQ is much easier to deal with than it was in D7.

10. and 11. Are you proposing to rename the upstream field from the workspace entity to target? I guess I'm ok with that, just need a confirmation :)

12. Do you mean that it's overkill to use the private tempstore instead of the actual PHP session? I thought the comment clarifies why we chose that approach. So that editors can come back to work the following day, login and see the site in exactly the same "state" they left it in the previous day.

13. It is called session_id to match the CouchDB spec (http://docs.couchdb.org/en/2.1.1/replication/protocol.html#retrieve-repl...) but it is actually an UUID, see the bottom of \Drupal\workspace\Plugin\RepositoryHandler\LocalWorkspaceRepositoryHandler::replicate(). And sequence ID is not necessary for local deployments, it's only there to have a unified "logging" facility for all kind of deployments.

14. That's because the ReplicationLog entity type provides us with a well-defined schema for logging deployments, that we can for example inspect with an entity query, while core's standard logger does not have this capability.

15., 16. Both points will be handled in #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace. That's the issue where we want to clarify and finish the API around repository handlers.

After fixing/clearing up all the other review points above, would you agree that #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace is the only (architectural) beta blocker for the module?

This interdiff also contains a change requested in #2910673-6: Workspace UI usability review, and restricts the upstream/target workspace to Live for now in the UI.

#111.1 - I think it makes more sense to hard-code it, the label could still be translatable via string translation. Live isn't really a workspace at all, so better for me for it be as unlike a workspace as possible, it's really another name for 'none' in select lists and etc.

#111.10-11 - I think target is right, or at least it's much closer to what's happening than remote if we're sticking to git-centric terminology. Not sure if the UX team has seen this though - to the extent that the terminology is exposed in the UI it'd be good for them to see it.

#111.12 - to me it seems like overkill, I might not want to go back to exactly the same workspace if I've logged out, in which case I'd need to go back to live. Or maybe someone else has finished working on the worspace and published it in the interim, and I'd get redirected to a 403 after login.

#111.13 - uuid seems better. Couch DB presumably doesn't have to worry about confusion with PHP session IDs.

#111.14 - why is that any different to other systems using the log component though? We don't log anything for edits on the live site, but we do have the full revision history, which we'll still have here. It feels like a feature that's awkwardly between revision logs and the logger.

#111.15-16 - following that issue now, when we discussed that on slack I wasn't sure about the relationship between the plugin and the config entity (seemed inverted compared to our usual pattern for menu links etc.) but haven't reviewed code yet.

I didn't spot other major architectural issues but really need another look through.

Regarding #112.13 and #112.14:

The case for ReplicationLog to exists as a purpose-built system is because it's an important piece for a generic replication system to work. For example, in the CouchDB replication protocol these log entities are exposed over an HTTP API (as implemented by the Relaxed module in contrib) and used by clients to optimize replications, and figure out from where to start/continue/stop a replication. These ReplicationLog entities contain useful information about the sequence numbers etc.

One can easily argue that ReplicationLog entities are primarily useful for remote replication (as local replications are relatively straight forward). However, in order to design generic interfaces in a way that makes sense I would argue that we should keep ReplicationLog for this purpose. For example RepositoryHandlerInterface::replicate() returns ReplicationLogInterface. That makes sense, and provides the foundation for contrib to build remote replications on top. Remote replication is the primary use case that is funding this initiative.

Regarding the name session_id, that's a CouchDB-ism and I'm fine with renaming this to something more generic that makes sense for Drupal. The serialization for CouchDB compatibility over HTTP can map this in contrib.

@catch All this said, I don't want this to hold up progress on getting workspaces into core. So if my arguments are not convincing enough, let's figure out what the interfaces would look like without ReplicationLog. Then we can find ways around this in contrib later :)

Regarding #45:

"Switching workspaces is jarring: it happens (too) fast and there's little feedback that it actually happened. Wondering if there's enough (visual) feedback about what just happened when you switch workspaces. A first useful step could be to have a clear visual label that distinguishes the live workspace from all the others."

We solved this problem on our installation by injecting a workspace class in the body tag and have provided this solution as a feature request.

https://www.drupal.org/project/multiversion/issues/2933302

A small update with fixes for these review points from #110: 10, 11, 12, 13

- Rename 'upstream' to 'target' and remove the custom widget and validation constraint
- Use the PHP session instead of the private tempstore in the session workspace negotiator
- Rename the 'session_id' field from ReplicationLog to 'session_uuid'
- Clean up all the functional tests

For #110.1: I've been trying to remove the Live workspace in #2935780: Remove the concept of a 'live' default workspace but I'm not sure that it's possible.

#2885469: Regression: manually setting the ID field for newly-created content entities is not possible anymore (public follow-up to SA-2017-002) was committed in the meantime, which means we only have one more (small) dependency for this patch: #2935076: EntityResourceTestBase::getNormalizedPostEntity() wrongly assumes that all entity types have auto-generated sequential IDs

@dixon_ #113

Regarding #112.13 and #112.14:

The case for ReplicationLog to exists as a purpose-built system is because it's an important piece for a generic replication system to work. For example, in the CouchDB replication protocol these log entities are exposed over an HTTP API (as implemented by the Relaxed module in contrib) and used by clients to optimize replications, and figure out from where to start/continue/stop a replication. These ReplicationLog entities contain useful information about the sequence numbers etc.

One can easily argue that ReplicationLog entities are primarily useful for remote replication (as local replications are relatively straight forward).

To me it feels like a feature that's specific to replicating workspaces remotely, which is not really generic (it might be generic for remote replication).

However, in order to design generic interfaces in a way that makes sense I would argue that we should keep ReplicationLog for this purpose. For example RepositoryHandlerInterface::replicate() returns ReplicationLogInterface. That makes sense, and provides the foundation for contrib to build remote replications on top.

Is there a particular reason that the log couldn't be set as a property and returned via a getter? Then it'd be straightforward to extend the interface in contrib to add that getter.

Also a general question on remote replication. Are workspaces replicated as-is (i.e. as pending revisions attached to a workspace) then 'published' using local replication? That's the only way I could think to rule out network issues or similar interrupting mid-replication and leaving the live site in a partial state.

@amateescu #111.
I'm not sure how #2931067: Add a ContentRepository config entity so we can store fully configured repository handlers, which can be reused for the upstream values of a workspace affects #110.16 (except maybe touching the same code?), it's an implementation rather than API detail that should only affect the method and maybe error handling.

@catch, yes, it was about touching the same code :)

This should fix the REST test fails, a new field was added by #2891215: Add a way to track whether a revision was default when originally created.

Those new failures are being handled in #2936704: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead, let's wait a bit for that issue before adding deprecation messages for the workspace routes.

@catch

#111.1 - I think it makes more sense to hard-code it, the label could still be translatable via string translation. Live isn't really a workspace at all, so better for me for it be as unlike a workspace as possible, it's really another name for 'none' in select lists and etc.

I agree it would be nice to hard-core the live workspace. But unfortunately I think this is a bit too hard to achieve. We've already achieved zero performance impact with the live workspace by not joining/querying with it. But the live workspace entity is still needed to not create unmaintainable exceptions in the code everywhere. Tim and I attempted this in the past and failed, and Andrei attempted it recently here: #2935780: Remove the concept of a 'live' default workspace.

So, I would suggest we keep the live workspace entity for this initial patch. This pattern would be similar to how the anonymous user entity is kept in the DB. If we later find clean ways to remove the live workspace entity I'm all for it!

To me it feels like a feature that's specific to replicating workspaces remotely, which is not really generic (it might be generic for remote replication).

Is there a particular reason that the log couldn't be set as a property and returned via a getter? Then it'd be straightforward to extend the interface in contrib to add that getter.

I understand your viewpoint here, and I don't disagree. One can definitely implement the necessary logging in different ways.
I'm fine with removing the ReplicationLog entity from this patch.

Also a general question on remote replication. Are workspaces replicated as-is (i.e. as pending revisions attached to a workspace) then 'published' using local replication?

Right now (in the first iteration of the contrib modules) they are replicated as-is to the target workspace (wether that's the "live" workspace or another workspace on the remote site). But that's mostly due to specific implementation details in those first versions of the contrib modules. We haven't yet written remote replication on-top of this core patch. The flow you describe here certainly makes a lot of sense for robustness!

Time for a "small" update here with all the work from the past couple of weeks. Here's the shortlog of the changes:

- Allow workspaces to be deleted
- Adding a new published entity in a non-default workspace does not create two workspace association entries.
- Do not allow concurrent edits in different workspaces
- Delete workspace_association entries when a workspace is pushed.
- Clean up the workspace switcher block and a few tests.
- Small test code cleanup
- Design a proper API for repository handlers.
- Work around the fact that core's REST test suite assumes that all content entity types have serial IDs.
- WorkspaceActivateForm cancel redirects to manage workspaces
- Remove the ReplicationLog entity type.

I emphasized a few changes that I think are the most important. The only @todo from @catch's review is #110.16 :)

We implement a workaround for #2935076: EntityResourceTestBase::getNormalizedPostEntity() wrongly assumes that all entity types have auto-generated sequential IDs so we don't have to roll combined patches anymore, but #2936704: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead is still a problem so this patch will probably have the same fails as #118.

Another quick update which should fix all the outstanding review points and a couple of PostgreSQL and SQLite problems. Here's the changelog:

- Fix SQLite support.
- Cache the active workspace in WorkspaceManager
- Decorate core's entity query services instead of replacing them and fix PgSQL support (fixes #98.6)
- Wrap workspace deployments in a single database transaction (fixes #110.16)
- Remove workaround for #2935076: EntityResourceTestBase::getNormalizedPostEntity() wrongly assumes that all entity types have auto-generated sequential IDs

In the meantime, #2935076: EntityResourceTestBase::getNormalizedPostEntity() wrongly assumes that all entity types have auto-generated sequential IDs and #2936704: Remove REST route deprecations from DeprecationListenerTrait::getSkippedDeprecations(), use ExpectDeprecationTrait::expectDeprecation() instead were committed so we're down to 0 dependencies!

IMO, this is quite ready for RTBC, there's just one pending question: should we rename the module to Workspaces (plural), to match Views, Workflows, etc.?

My local branch had a commit which was reverting #2935076: EntityResourceTestBase::getNormalizedPostEntity() wrongly assumes that all entity types have auto-generated sequential IDs, fixed that and also two additional things:

- Fix error translation string
- Remove dependency on the User module

It seems that the dependency on the User module is actually required for some reason.

Review in progress, note to self - up to WorkspaceNegotiator

1. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,220 @@
   + *     "storage" = "Drupal\Core\Entity\Sql\SqlContentEntityStorage",
   

   i don't think this is needed, as its the default

2. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,220 @@
   + *   base_table = "workspace",
   + *   revision_table = "workspace_revision",
   + *   data_table = "workspace_field_data",
   + *   revision_data_table = "workspace_field_revision",
   

   these look to be using the defaults - not needed?

3. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,220 @@
   +  public function getRepositoryHandlerPlugin() {
   ...
   +      return \Drupal::service('plugin.manager.workspace.repository_handler')->createInstance($target, $configuration);
   

   Are we sure we want content entities instantiating plugins. Could this be done from outside the entity?

4. +++ b/core/modules/workspace/src/Entity/WorkspaceAssociation.php
   @@ -0,0 +1,76 @@
   +    $fields['workspace'] = BaseFieldDefinition::create('entity_reference')
   ...
   +    $fields['content_entity_type_id'] = BaseFieldDefinition::create('string')
   ...
   +    $fields['content_entity_id'] = BaseFieldDefinition::create('integer')
   ...
   +    $fields['content_entity_revision_id'] = BaseFieldDefinition::create('integer')
   

   Is this repeating the design of content moderation? I thought that now we can delete/purge base fields we would not need these intermediate entities anymore?

   Is this needed because one content entity revision can live in multiple workspaces?

5. +++ b/core/modules/workspace/src/EntityAccess.php
   @@ -0,0 +1,170 @@
   +        AccessResult::allowedIf($active_workspace->getOwnerId() == $account->id())->cachePerUser()->addCacheableDependency($active_workspace)
   

   we should use === here.

6. +++ b/core/modules/workspace/src/EntityAccess.php
   @@ -0,0 +1,170 @@
   +    $perms['bypass entity access workspace ' . $workspace->id()] = [
   

   I think this is the first instance we have of permissions per content entity. How big is this going to get? Our permissions system is pretty stretched as is.

7. +++ b/core/modules/workspace/src/EntityOperations.php
   @@ -0,0 +1,254 @@
   +      ->condition('workspace', $active_workspace->id(), '=')
   

   nit, = not needed

8. +++ b/core/modules/workspace/src/EntityOperations.php
   @@ -0,0 +1,254 @@
   +        if ($entities[$result['content_entity_id']]->getRevisionId() == $result[$max_revision_id]) {
   

   nit, should use === here?

9. +++ b/core/modules/workspace/src/EntityTypeInfo.php
   @@ -0,0 +1,113 @@
   +          // An entity can only be edited in one workspace.
   +          $workspace_id = reset($workspace_ids);
   

   Seems like one workspace per content entity, so wondering about the intermediate entity?

10. +++ b/core/modules/workspace/src/EntityTypeInfo.php
    @@ -0,0 +1,113 @@
    +          if ($workspace_id != $this->workspaceManager->getActiveWorkspace()->id()) {
    

    nit, these are strings, lets use ===

11. +++ b/core/modules/workspace/src/Form/WorkspaceActivateForm.php
    @@ -0,0 +1,117 @@
    +    catch (\Exception $e) {
    +      watchdog_exception('workspace', $e);
    +      $this->messenger->addError($e->getMessage());
    +    }
    

    this is a fairly broad-brush catch here - what exceptions are we expecting?

    can we be more granular here?

12. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,200 @@
    +    // We can not push or pull if we do not have a valid target.
    +    if (!$repository_handler) {
    +      throw new HttpException(500, 'The specified repository handler plugin does not exist.');
    +    }
    

    This is fairly extreme, can we not handle this with a 404 in the routing system?

13. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,200 @@
    +      $workspace->getRepositoryHandlerPlugin()->push();
    ...
    +      $workspace->getRepositoryHandlerPlugin()->pull();
    

    Like I said above, I'm not sure its the entity's job to instantiate plugins.

    I think a separate service would be fine, or even the plugin manager itself.

    $repoHandlerPlugin->instantiateForWorkspace($workspace)

    And then change the getRepositoryHandlerPlugin method to just return the ID instead.

14. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,200 @@
    +    catch (\Exception $e) {
    ...
    +    catch (\Exception $e) {
    

    same here, very broad exception being caught

15. +++ b/core/modules/workspace/src/Form/WorkspaceDeployForm.php
    @@ -0,0 +1,200 @@
    +      $this->messenger->addMessage($this->t('Deployment error'), 'error');
    ...
    +      $this->messenger->addMessage($this->t('refresh error'), 'error');
    

    This isn't very useful to help debug what went wrong - can we add some more detail about what was being deployed etc?

16. +++ b/core/modules/workspace/src/Form/WorkspaceForm.php
    @@ -0,0 +1,158 @@
    +  protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    return array_merge([
    +      'label',
    +      'id',
    +    ], parent::getEditedFieldNames($form_state));
    

    can we comment why this is needed?

17. +++ b/core/modules/workspace/src/Form/WorkspaceSwitcherForm.php
    @@ -0,0 +1,144 @@
    +    $workspaces = $this->workspaceStorage->loadMultiple();
    +    $workspace_labels = [];
    +    foreach ($workspaces as $workspace) {
    +      $workspace_labels[$workspace->id()] = $workspace->label();
    +    }
    +
    +    $active_workspace = $this->workspaceManager->getActiveWorkspace();
    +    unset($workspace_labels[$active_workspace->id()]);
    

    this occurs a few times, merit in having a convenience method on the storage handler or manager?

18. +++ b/core/modules/workspace/src/Form/WorkspaceSwitcherForm.php
    @@ -0,0 +1,144 @@
    +    // Ensure the workspace by that ID exists.
    +    if (!$this->workspaceStorage->load($id)) {
    +      $form_state->setErrorByName('workspace_id', $this->t('This workspace does not exist.'));
    +    }
    

    When would this happen? If the user submitted a form with a missing workspace, the select field validation should prevent this, and show the 'invalid value' error?

19. +++ b/core/modules/workspace/src/Form/WorkspaceSwitcherForm.php
    @@ -0,0 +1,144 @@
    +    catch (\Exception $e) {
    

    same comment about broadness

Thanks for the review, @larowlan!

Re #131:

1. That's right, we can remove it.
2. They are needed even when the default table names are used, at least until #2232465: Deprecate table names from entity definitions is fixed. We recently had to add a missing table entry for block_content in #2858109: The BlockContent entity type does not specify its revision data table because it was causing problems in #2820848: Make BlockContent entities publishable.
3. I don't see any problems with instantiating a plugin from a content entity, Commerce does that as well :) And we do want the Workspace entity type to be the main entry point for pushing/pulling changes. In fact, I made that even more clear by adding the push()/pull() methods to WorkspaceInterface.
4. Yup, it is repeating the design of Content Moderation. And even though we can purge base field now, we still have to use a separate entity type because we want to update those values without "touching" the main entity. This comes back to the fact that we don't have a clear "metadata API" :) #2098191: Add a meta-data API
5. We can't use strict equality checks on fields that store non-string values...
6. Even though workspaces are content entities, they are not meant to linger in the database forever, the common practice is to delete a workspace after you are "done" with that specific set of changes. So I'm not too worried about that point. Of course, custom code can always override this permission model.
7. Fixed.
8. Nope, same as above.
9. "one workspace per content entity" is just a limitation of this initial patch, until we have a conflict resolution implementation.
10. We can use strict equality checks on strings, even though I don't really see the point in that. Fixed.
11. Good point! We are expecting a WorkspaceAccessException there, fixed.
12. I've re-worked the way we handle null/missing plugins to use the fallback plugin pattern, so that code is no longer needed.
13. See point 3. above. But you're right, the plugin manager should provide an easy way of instantiating a plugin from a workspace, so I added a createFromWorkspace() to the repository handler manager.
14. This is intentionally broad, we want to catch any exception thrown by RepositoryHandlerInterface::push()/pull(). We could introduce a separate RepositorySyncException exception, which would be thrown by RepositoryHandlerInterface::push()/pull() and only catch that one. Would you prefer it this way?
15. The list of entities that are being pushed or pulled is already displayed by that form, and when an error happens we don't know exactly which entity couldn't be saved, so not sure what to do for this point..
16. The documentation of \Drupal\Core\Entity\ContentEntityForm::getEditedFieldNames() tells exactly what that method does and when/why you need to override it.
17. I tried various searches but I couldn't find any other place where we need this exact list of workspace_id => workspace_label mapping, can you point out which places you found?
18. Very nice catch, that whole validation is indeed not needed. Fixed.
19. As above, we need to catch only WorkspaceAccessException. Fixed.

One of the things I admire most about the Workflow Initiative is that y'all take the most arcane, complicated stuff in Drupal core -- the kind of stuff that makes me want to give up programming and become a dirt farmer -- and write elegant, clear, understandable code to fix it up. It's glorious. I didn't get very far into this review (it'd take a day to read the entire thing), but this is fantastic work. I don't think I found anything serious, and I definitely don't think I'm qualified to RTBC, but I did find a few minor things and have some random questions (forgive me if they were already addressed earlier in this thread).

1. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,226 @@
   +      ->addPropertyConstraints('value', ['Regex' => ['pattern' => '/^[a-z0-9_]*$/']]);
   

   This regex will match an empty string. Shouldn't that * be a + to prevent that?

2. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,226 @@
   +    $fields['target'] = BaseFieldDefinition::create('string')
   +      ->setLabel(new TranslatableMarkup('Target workspace'))
   +      ->setDescription(new TranslatableMarkup('The workspace to push to and pull from.'))
   +      ->setRevisionable(TRUE)
   +      ->setRequired(TRUE)
   +      ->setDefaultValue('live');
   

   Should this be an entity reference?

3. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,226 @@
   +  public function getRepositoryHandler() {
   +    return \Drupal::service('plugin.manager.workspace.repository_handler')->createFromWorkspace($this);
   +  }
   

   This seems like it should cache the repository handler as $this->repositoryHandler or something.

4. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,226 @@
   +  public function setCreatedTime($created) {
   +    $this->set('created', (int) $created);
   +    return $this;
   +  }
   

   Nit: $this->set() already returns $this.

5. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,226 @@
   +  public function setOwner(UserInterface $account) {
   +    $this->set('uid', $account->id());
   +    return $this;
   +  }
   

   Same here.

6. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,226 @@
   +  public function setOwnerId($uid) {
   +    $this->set('uid', $uid);
   +    return $this;
   

   And here.

7. +++ b/core/modules/workspace/src/Entity/WorkspaceAssociation.php
   @@ -0,0 +1,76 @@
   + *   base_table = "workspace_association",
   + *   revision_table = "workspace_association_revision",
   

   Why aren't the data and revision data tables defined?

8. +++ b/core/modules/workspace/src/Entity/WorkspaceAssociation.php
   @@ -0,0 +1,76 @@
   +    $fields['content_entity_id'] = BaseFieldDefinition::create('integer')
   +      ->setLabel(new TranslatableMarkup('Content entity ID'))
   +      ->setDescription(new TranslatableMarkup('The ID of the content entity associated with this workspace.'))
   +      ->setRequired(TRUE)
   +      ->setRevisionable(TRUE);
   

   I'm probably being overly paranoid, but won't this field type depend on the nature of the referenced content entity? For example, what if it defines its ID field as a string (like the Workspace entity type itself does)? How can we account for that? Should we even account for that, even if it's just by throwing an exception?

   On the other hand, Content Moderation uses this same pattern, so it's probably not really an issue. But, I figured I'd mention it anyway. It's just that there's no official guidance (as far as I know) which says that content entity IDs need to be integers, so this assumption could produce some major problems in exotic use cases.

9. +++ b/core/modules/workspace/src/EntityAccess.php
   @@ -0,0 +1,170 @@
   +    // correct, ie, if you're "in" a given workspace then you get ALL THE PERMS
   

   Nit: 'ie' --> 'i.e.'

10. +++ b/core/modules/workspace/src/EntityAccess.php
    @@ -0,0 +1,170 @@
    +   * Derives the delete permission for a specific workspace.
    

    This seems inaccurate.

11. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    if ($results) {
    +      foreach ($results as $key => $result) {
    +        if ($entities[$result['content_entity_id']]->getRevisionId() == $result[$max_revision_id]) {
    +          unset($results[$key]);
    +        }
    +      }
    +    }
    

    This seems like a good candidate for array_filter().

12. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    if (!$this->workspaceManager->entityTypeCanBelongToWorkspaces($entity->getEntityType())
    +       || $this->workspaceManager->getActiveWorkspace()->isDefaultWorkspace()) {
    +      return;
    +    }
    

    The entity_load hook checked if $this->workspaceManager->getActiveWorkspace() returned a truthy value before calling isDefaultWorkspace(). Why doesn't this method do the same thing? (Although, I prefer this cleaner, more streamlined code.)

13. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    if (!$this->workspaceManager->entityTypeCanBelongToWorkspaces($entity->getEntityType())
    +       || $this->workspaceManager->getActiveWorkspace()->isDefaultWorkspace()) {
    +      return;
    +    }
    

    This same code has been repeated three times now; maybe it should be a helper method? :)

14. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    // Handle the case when a new published entity was created in a non-default
    +    // workspace and create a published pending revision for it.
    +    if (isset($entity->_initialPublished)) {
    +      // Operate on a clone to avoid changing the entity prior to subsequent
    +      // hook_entity_insert() implementations.
    +      $pending_revision = clone $entity;
    +      $pending_revision->setPublished();
    +      $pending_revision->isDefaultRevision(FALSE);
    +      $pending_revision->save();
    +    }
    

    This seems like it could cause infinite recursion. Why doesn't it? Can there be a comment explaining how that is prevented?

15. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    // Only run if the entity type can belong to a workspace and we are in a
    +    // non-default workspace.
    +    if (!$this->workspaceManager->entityTypeCanBelongToWorkspaces($entity->getEntityType())
    +       || $this->workspaceManager->getActiveWorkspace()->isDefaultWorkspace()) {
    +      return;
    +    }
    

    Should probably be moved to a helper method.

16. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    // Only track new revisions.
    +    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
    +    if ($entity->getLoadedRevisionId() != $entity->getRevisionId()) {
    

    I don't understand how comparing revision IDs will determine if the revision is "new". Doesn't RevisionableInterface have an isNewRevision() method for this purpose? Why won't it suffice? Can the reasoning be documented?

17. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    else {
    +      $workspace_association = WorkspaceAssociation::create([
    +        'content_entity_type_id' => $entity->getEntityTypeId(),
    +        'content_entity_id' => $entity->id(),
    +      ]);
    +    }
    

    Nit: You could reuse $storage->create() here to keep things a little more cleanly encapsulated.

18. +++ b/core/modules/workspace/src/EntityOperations.php
    @@ -0,0 +1,254 @@
    +    // Add the revision ID and the workspace ID.
    +    $workspace_association->set('content_entity_revision_id', $entity->getRevisionId());
    +    $workspace_association->set('workspace', $this->workspaceManager->getActiveWorkspace()->id());
    

    It'd be cool if there was a method on WorkspaceAssociation to set the content entity type, ID, and revision ID in a single call. Something like $workspace_association->track($entity).

19. +++ b/core/modules/workspace/src/EntityQuery/PgsqlQueryFactory.php
    @@ -0,0 +1,53 @@
    +   * Constructs a SqlQueryFactory object.
    

    Not quite accurate! :)

Thanks for the review (and the kind words), @phenaproxima!

1. Very nice catch! Fixed.
2. The target of a workspace is a repository handler plugin, not another workspace, so we can not use an entity reference field :)
3. I'm pretty sure the container already caches its services, so I'm not sure that we really need to double that static cache here..
4. Sure, let's save a line of code :)
5. Idem.
6. Idem.
7. Because the workspace association entity type is not translatable and we're not tracking individual translations like Content Moderation does.
8. Yeah.. this is an existing pain point for core. Like Content Moderation, there's a similar restriction in the Comment module, which can only track entity types with integer IDs. Nothing we can do about it in workspaces..
9. Fixed.
10. Fixed.
11. Sure thing, fixed.
12. Entity loading had some special requirements in the past, but now that we're caching the active workspace in the WorkspaceManager, it can use the standard check as well.
13. Introduced \Drupal\workspace\WorkspaceManager::shouldAlterOperations() for this repeating code.
14. Added a comment explaining why this can't cause an infinite recursion :)
15. Yup, see the reply to 13. above.
16. I tried using isNewRevision() there but there was a problem. I don't really remember the details, but the test fails can be seen at #2938944-9: Adding a new published entity in a non-default workspace does not create two workspace association entries.
17. Yup, we can do that. Fixed.
18. There aren't any other places which should operate on a WorkspaceAssociation entity type so far, so I'd wait for a couple more use cases before introducing that helper.
19. Indeed! Fixed :)

I didn't get trough the entire patch, but I have some nits.

1. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,223 @@
   +      ->setDefaultValueCallback('Drupal\workspace\Entity\Workspace::getCurrentUserId')
   ...
   +  /**
   +   * Default value callback for 'uid' base field definition.
   +   *
   +   * @see ::baseFieldDefinitions()
   +   *
   +   * @return int[]
   +   *   An array containing the ID of the current user.
   +   */
   +  public static function getCurrentUserId() {
   +    return [\Drupal::currentUser()->id()];
   +  }
   

   Node and Media also have this, this is the 3th occurance of this exact code + docblock in core, should we open a followup to figure out if it makes sens to move this to a common class, or a trait?

2. +++ b/core/modules/workspace/src/Entity/Workspace.php
   @@ -0,0 +1,223 @@
   +      ->setDescription(new TranslatableMarkup('The UNIX timestamp of when the workspace has been created.'));
   

   For nodes, this is t('The time that the node was created.'), so I think we can also do this here: The time that the workspaces was created.

3. +++ b/core/modules/workspace/src/EntityOperations.php
   @@ -0,0 +1,248 @@
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity being saved.
   ...
   +    /** @var \Drupal\Core\Entity\RevisionableInterface|\Drupal\Core\Entity\EntityPublishedInterface $entity */
   

   Instead of adding the inline comment here, can we just add this in the @param instead?

4. +++ b/core/modules/workspace/src/EntityOperations.php
   @@ -0,0 +1,248 @@
   +    // Force a new revision if the entity is not replicating.
   +    if (!$entity->isNew() && !isset($entity->_isReplicating)) {
   

   This comment doesn't add a lot of value, I think it can be removed.

5. +++ b/core/modules/workspace/src/EntityOperations.php
   @@ -0,0 +1,248 @@
   +    // Only run if the entity type can belong to a workspace and we are in a
   +    // non-default workspace.
   

   This comment is repeated in every method of this class, that smells a little weird. I think it is valuable though, no idea how/if that can be improved.

6. +++ b/core/modules/workspace/src/EntityQuery/QueryFactory.php
   @@ -0,0 +1,53 @@
   +class QueryFactory extends BaseQueryFactory {
   ...
   +   * Constructs a SqlQueryFactory object.
   

   Mismatch in docs vs class name.

7. +++ b/core/modules/workspace/src/EntityTypeInfo.php
   @@ -0,0 +1,113 @@
   +  public function formAlter(array &$form, FormStateInterface $form_state, $form_id) {
   

   By doing early returns in this method we can make it easier to grok.

8. +++ b/core/modules/workspace/src/Plugin/RepositoryHandler/LiveRepositoryHandler.php
   @@ -0,0 +1,196 @@
   +class LiveRepositoryHandler extends RepositoryHandlerBase implements RepositoryHandlerInterface, ContainerFactoryPluginInterface {
   ...
   +   * Constructs a new DefaultRepositoryHandler.
   

   docblock vs classname.

9. +++ b/core/modules/workspace/src/Plugin/Validation/Constraint/EntityWorkspaceConflictConstraint.php
   @@ -0,0 +1,20 @@
   +  public $message = 'The content is being edited in the %label workspace. As a result, your changes cannot be saved.';
   

   The DeletedWorkspaceConstraint has a docblock here, we should either remove that one, or add a similar one here.

Thanks @borisson_ for taking another look! :)

Re #135:

1. Good point, opened #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface for that.
2. Fixed.
3. I tried that but then the @param type hint will not match the actual function signature, and we can't change that one because that's what we receive from hook_entity_presave()..
4. Not sure about this one, I don't think it hurts to have it there. I moved it closer to the code that it's actually doing what it says :)
5. IMO it's fine to keep it as-is.
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.

+++ b/core/modules/workspace/src/Entity/Workspace.php
@@ -0,0 +1,223 @@
+
+    $fields['target'] = BaseFieldDefinition::create('string')
+      ->setLabel(new TranslatableMarkup('Target workspace'))
+      ->setDescription(new TranslatableMarkup('The workspace to push to and pull from.'))
+      ->setRevisionable(TRUE)
+      ->setRequired(TRUE)
+      ->setDefaultValue('live');

I'm still struggling with this a fair bit. Going to try to approach it from the point of view of use cases rather than architecture to get the issues across.

User A starts workspace A, modifying entity 2.

User B starts workspace B modifying entity 3, this change is pretty minor.

User A decides they want to add an entity reference from entity 2 to entity 3, and they want to incorporate the changes from workspace B so they can see how it'll look together.

User A would then need to either 'pull' from workspace B, or go to workspace B and 'push' to workspace A.

However with the current architecture, both of them have 'live' stored as a target.

So if I want to merge, do I need to edit the workspace, change the target, do a push or pull, then edit the workspace again, and change the target back to live?

It feels to me like merging, publishing to live, and replication content-hub style are really three different operations that could each happen to any workspace - so you could have a 'merge' form and a 'publish' form as different things, rather than dependent on configuration.

With the example of replication I asked about whether it would first replicate as a draft workspace then deploy two live as a two step operation that could take advantage of transactions for the publishing step - this again means the target of the workspace would need to change in order to do that (or switch the target of the workspace when it gets replicated itself?).

I haven't re-reviewed the entire patch, but this is the one thing that I had trouble with the last time I did a significant review and I'm not sure I got it across last time properly.