Introducing the concept of workspaces. Content entities always belong to a workspace (there is one main exception, which is the user entity type). A workspace is a silo/container of content on a site. However, this phase only introduces the underlying concept with one single workspace available, without many supporting APIs around it (see later phases). This phase will not change any UIs or behaviours in core.

See Workspace module for the current contrib implementation.

  • Define the workspace entity itself
  • Introduce the workspace reference field for all content entity types
  • Extend storage handlers to work with workspaces

Architecture overview

  • Workspaces are content entities.
  • A "live" and "stage" workspace is created on installation.
  • The default workspace (the one anonymous users see) is set in workspace.services.yml as a parameter.
  • Workspaces depend on revisionable and publishable content, therefore only Nodes are supported.
  • No queries, entity loads, or anything are altered for the default workspace.
  • Content is linked to a workspace via a ContentWorkspace entity (similar to Content Moderation's ContentModerationState entity).
  • All content added on non-default workspaces is saved as unpublished. The real published states is saved against the ContentWorkspace entity.
  • There is a computed entity_reference field "workspace" added to all entity types that can belong to a workspace.
  • An entity query alter is performed on non-default workspaces to join the ContentWorkspace entity field revision table and only load entities which are in the active (current) or default workspace.
  • A Views query alter is done in the a similar way to the entity query alter, although here we also take into account the published status from the ContentWorksapce entity if the View has a published filter.
  • In hook_entity_load we switch out the entity revision for the correct one for the current active workspace based on data stored in the ContentWorkspace entity. The published status is also switched if needed.
  • Each workspace can have an upstream, this is like a git upstream, when deploying content is replicated to the upstream.
  • Upstreams are plugins. In the workspace module these are derived from workspaces. Contrib could hook into this to make upstreams anything, other sites, non-drupal apps, etc.
  • Replication is handled by a tagged service, again, this allows contrib to hook in and provide different replication.
  • The default replicator looks for changes between two workspaces then loops through all these revisions and saves them on the target workspace.
  • Each replication creates a replication log entity. For the default replicator this is based on an MD5 hash of the source and target workspaces, so when replicating again between those two workspaces we know what replications have already happened.
  • All entity updates create an entity in the sequence index. This index is also used in replication two find the changes since the last replication.
  • The user's active workspace is determined via a negotiator, so it can either come from the container parameter (default workspace), a session value, or a url query parameter.
  • There is complex access checks which will allow you to lock down users to only create content in specific workspaces.
  • The active workspace is switched by either using the toolbar.module integration, or enabling a switcher block.
CommentFileSizeAuthor
#132 interdiff-132.txt21.3 KBamateescu
#132 2784921-132.patch248.05 KBamateescu
#130 interdiff-130.txt363 bytesamateescu
#130 2784921-130.patch245.46 KBamateescu
#128 interdiff-128.txt3.31 KBamateescu
#128 2784921-128.patch245.44 KBamateescu
#125 interdiff-125.txt13.54 KBamateescu
#125 2784921-125.patch247.53 KBamateescu
#123 interdiff-123.txt109.52 KBamateescu
#123 2784921-123.patch243.26 KBamateescu
#118 interdiff-118.txt658 bytesamateescu
#118 2784921-118-combined.patch222.62 KBamateescu
#118 2784921-118-for-review.patch220.54 KBamateescu
#115 interdiff-115.txt36.86 KBamateescu
#115 2784921-115-combined.patch222.53 KBamateescu
#115 2784921-115-for-review.patch220.45 KBamateescu
#111 interdiff-111.txt10.58 KBamateescu
#111 2784921-111-combined.patch253.86 KBamateescu
#111 2784921-111-for-review.patch229.11 KBamateescu
#109 interdiff-109.txt1.5 KBamateescu
#109 2784921-109-combined.patch255.46 KBamateescu
#109 2784921-109-for-review.patch230.71 KBamateescu
#107 interdiff-107.txt67.46 KBamateescu
#107 2784921-107-combined.patch302.92 KBamateescu
#107 2784921-107-for-review.patch231.82 KBamateescu
#105 interdiff-105.txt11.44 KBamateescu
#105 2784921-105-combined.patch295.46 KBamateescu
#105 2784921-105-for-review.patch224.9 KBamateescu
#102 interdiff-102.txt6.26 KBamateescu
#102 2784921-102-combined.patch297.53 KBamateescu
#102 2784921-102-for-review.patch229.88 KBamateescu
#99 interdiff-99.txt40.58 KBamateescu
#99 2784921-99-combined.patch298.57 KBamateescu
#99 2784921-99-for-review.patch230.93 KBamateescu
#94 interdiff-94.txt35.2 KBamateescu
#94 2784921-94-combined.patch298.19 KBamateescu
#94 2784921-94-for-review.patch230.55 KBamateescu
#91 interdiff-91.txt7.65 KBamateescu
#91 2784921-91-combined.patch296.63 KBamateescu
#91 2784921-91-for-review.patch228.99 KBamateescu
#89 interdiff-89.txt1.04 KBamateescu
#89 2784921-89-combined.patch296.36 KBamateescu
#89 2784921-89-for-review.patch228.72 KBamateescu
#87 interdiff-87.txt9 KBamateescu
#87 2784921-87-combined.patch296.23 KBamateescu
#87 2784921-87-for-review.patch228.59 KBamateescu
#85 interdiff-85.txt8.1 KBamateescu
#85 2784921-85-combined.patch289.75 KBamateescu
#85 2784921-85-for-review.patch222.1 KBamateescu
#83 2784921-83-combined.patch286.61 KBamateescu
#83 2784921-83-for-review.patch217.78 KBamateescu
#77 2784921-77.patch214.69 KBtimmillwood
#77 interdiff-2784921-77.txt547 bytestimmillwood
#75 2784921-75.patch214.7 KBtimmillwood
#75 2784921-75.txt9.93 KBtimmillwood
#73 2784921-73.patch214.04 KBtimmillwood
#67 2784921-67.patch203.71 KBtimmillwood
#67 interdiff-2784921-67.txt6.72 KBtimmillwood
#66 2784921-66.patch204.21 KBtimmillwood
#66 interdiff-2784921-66.txt5.85 KBtimmillwood
#64 2784921-64.patch202.72 KBtimmillwood
#62 2784921-62.patch202.73 KBtimmillwood
#62 interdiff-2784921-62.txt8.42 KBtimmillwood
#60 2784921-60.patch200.49 KBtimmillwood
#60 interdiff-2784921-60.txt12.49 KBtimmillwood
#58 2784921-58.patch198.52 KBtimmillwood
#58 interdiff-2784921-58.txt10.47 KBtimmillwood
#56 2784921-56.patch212.52 KBtimmillwood
#56 interdiff-2784921-56.txt5.82 KBtimmillwood
#52 interdiff-52.txt23.94 KBamateescu
#52 2784921-52.patch210.51 KBamateescu
#50 interdiff-50.txt22.27 KBamateescu
#50 2784921-50.patch213.74 KBamateescu
#47 2784921-47.patch213.63 KBpk188
#44 2784921-44.patch214.24 KBtimmillwood
#44 interdiff-2784921-44.txt8.15 KBtimmillwood
#43 2784921-43.patch214.51 KBtimmillwood
#43 interdiff-2784921-43.txt2.8 KBtimmillwood
#41 2784921-41.patch214.53 KBtimmillwood
#41 interdiff-2784921-41.txt5.03 KBtimmillwood
#40 2784921-40.patch215.58 KBtimmillwood
#40 interdiff-2784921-40.txt18.62 KBtimmillwood
#36 2784921-36.patch215.85 KBtimmillwood
#36 interdiff-2784921-36.txt3.3 KBtimmillwood
#34 2784921-34.patch215.73 KBtimmillwood
#32 Peek 2017-05-23 17-05.gif1.2 MBtimmillwood
#32 2784921-32.patch203.38 KBtimmillwood
#32 interdiff-2784921-32.txt60.19 KBtimmillwood
#28 2784921-28.patch203.64 KBtimmillwood
#28 interdiff-2784921-28.txt458 bytestimmillwood
#26 2784921-26.patch203.55 KBtimmillwood
#26 interdiff-2784921.txt150.45 KBtimmillwood
#18 interdiff.txt1.05 KBtimmillwood
#18 2784921-18.patch133.77 KBtimmillwood
#16 interdiff.txt3.44 KBtimmillwood
#16 2784921-16.patch132.72 KBtimmillwood
#14 2784921-14.patch131.05 KBtimmillwood
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Opening this issue to discuss the implementation details for workspaces.

In the Multiversion and Workspace modules we define workspaces as a content entity, then add a workspace field to all entity types to denote which workspace an entity belongs to, then every query has to check the workspace field.

I can't see this passing as a core solution, there are many alternatives, but none that get around adding a condition to each entity query.

timmillwood’s picture

Issue tags: +Workflow Initiative
catch’s picture

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.

Fabianx’s picture

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

timmillwood’s picture

Coming back to this (21 days later) I want to make sure I fully understand how this all works.

  • The "Live" workspace behaves exactly how standard Drupal works, all the same queries, all the same entity api.
  • When content is added to the "stage" workspace how do we prevent it from appearing on "Live"?
  • If an entity is added on Workspace "A" and another added on Workspace "B" we can join the tracking table to know which workspace should show which entity, but on live we would not join the tracking table, so what would stop it showing all entities?

Then... where do we add the under laying concept of workspace? is adding it in a module stable enough? or should it go into core, much like translations are. Yes, to do translations you need the translation modules, but the entity API can do translations without it.

catch’s picture

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.

timmillwood’s picture

So if all content is added as unpublished on all workspaces no matter which workspace the content was added, wouldn't this get confusing to people viewing /admin/content?

Fabianx’s picture

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

timmillwood’s picture

I think for this to work we need to be able to unpublished all of the things. #2810381: Add generic status field to ContentEntityBase is looking to introduce a generic 'status' base field, which we can then add to more entity types.

My current thinking is:

  • Workspace content entity to define workspaces
  • All content would belong in the "live" workspace
  • When content is created or updated in a "non-live" workspace we update a table (or entity) to denote which workspace the new entity/revision belongs to.
  • 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.
catch’s picture

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

timmillwood’s picture

Today I committed a 8.x-2.x branch of the Workspace module. I hope this will form the basis for the core experimental module.

So far I have:

  • Moved parts of Multiversion into Workspace
  • Removed all dependencies
  • Confirmed workspace types can be added
  • Confirmed workspaces can be added
  • Confirmed workspaces can be switched between using the toolbar
  • Confirmed the active workspace state is retained

Todo:

  • More tidy up
  • Make tests pass
  • Mark content as belonging to a workspace

During this time I also need to work out how replicating content will actually work. The CPS approaches above will play a big part in this. I would also like to use the services we have in the Replication module to determine differences between workspaces. This will allow us to be flexible enough for multi-site replication as well as the core single site preview functionality.

To mark content as belonging to a workspace I'd like to use the Content Moderation approach of a content entity and computed entity reference fields. Thus adding a ContentWorkspace entity which references the content entity and the workspace it belongs to.

For the CPS approach to fully work we need BlockContent, Term, and MenuLinkContent entities (at a minimum) to be unpublishable, #2810381: Add generic status field to ContentEntityBase will go a long way to making this happen.

timmillwood’s picture

The 8.2.x branch of Workspace module now has a ContentWorkspace entity type which gets updated when an entity is created or updated. The workspace field is a computed field which returns the Workspace via ContentWorkspace, or the default Workspace.

Trying to work out the best way of getting entities to only show in their correct workspace.

timmillwood’s picture

Status: Active » Needs review
FileSize
131.05 KB

Here's an initial patch for Workspace module.

It will fail on a couple of things (like hook_help), but I will work on tidy ups today.

What it does do is introduce WorkspaceType and Workspace entities, it then has toolbar integration to switch between workspaces. Every entity that is added gets a ContentWorkspace entity to map it to a workspace, much like in Content Moderation how we map an entity to it's moderation state.

One thing I'd like to get done before commit is only showing entities which belong to the current workspace, but still looking for the best way to do this. @catch? @Fabianx?

Replication is something I think should be added as a follow-up, yes, the module is pretty useless without replication, but to do it properly it has a lot of dependencies.

Status: Needs review » Needs work

The last submitted patch, 14: 2784921-14.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
132.72 KB
3.44 KB

Trying to fix some of the failures from #14

Status: Needs review » Needs work

The last submitted patch, 16: 2784921-16.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
133.77 KB
1.05 KB

This should fix the \Drupal\system\Tests\Module\InstallUninstallTest issue.

After speaking to @amateescu and @dixon there was concerns about getting Workspace module in without replication. Personally I'd like to get it in as a starting block so we can then work on multiple little patches, rather than one big one. Although I can understand the concerns with having a module in core that doesn't really do much.

dawehner’s picture

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

@dawehner - I think a mini version of replication would be cool, I remember we did do this in the early days of the contrib module and it did have some big bugs, but is specific use cases it worked ok. So maybe this could be one approach.

The other approach is just develop it in contrib for now, then push to core as one big patch when ready. To do replication properly (following the couchdb protocol) there are many dependencies.

timmillwood’s picture

Status: Needs review » Needs work

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs work » Needs review

I have been working on the workspace module for core in the 8.x-2.x branch of contrib, I'd happily take reviews there https://www.drupal.org/node/11627/git-instructions/8.x-2.x/nonmaintainer

It's still in pretty early stages, but taking influence from CPS no queries are altered on the "live" workspace, on all other workspace we alter queries, and hook into the node load to only show entities that are live or belong to the workspace. Basic replication of content between workspaces is also in place with a test to back this up.

xjm’s picture

Status: Needs review » Postponed

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

xjm’s picture

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

timmillwood’s picture

Status: Postponed » Needs review
FileSize
150.45 KB
203.55 KB

Time for an update, lets see if this works.

Status: Needs review » Needs work

The last submitted patch, 26: 2784921-26.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
458 bytes
203.64 KB

Let's give this another go.

Status: Needs review » Needs work

The last submitted patch, 28: 2784921-28.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

ok, fixed this test fail in contrib. Will add a new patch here in due course, but until then... please review!

timmillwood’s picture

Issue summary: View changes
timmillwood’s picture

Status: Needs review » Needs work

The last submitted patch, 32: 2784921-32.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
215.73 KB

#32 was missing the following files:

	core/modules/workspace/css/workspace.toolbox.css
	core/modules/workspace/icons/ffffff/ws_icon.svg
	core/modules/workspace/js/
	core/modules/workspace/templates/workspace-toolbox.html.twig
	core/modules/workspace/tests/src/Functional/ExistingContentTest.php

Status: Needs review » Needs work

The last submitted patch, 34: 2784921-34.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
215.85 KB

Seems like the default workspaces added in workspace.install aren't installed during InstallUninstallTest, which I've no idea why. However, tried to make this less relevant by altering the access checks.

Hard to test locally because InstallUninstallTest runs so slowly.

Status: Needs review » Needs work

The last submitted patch, 36: 2784921-36.patch, failed testing.

dawehner’s picture

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?

timmillwood’s picture

@dawehner - Thank you so much for the review!

If it makes things easier, the /core/modules/workspace part of this module is the same as the 8.x-2.x branch of https://www.drupal.org/project/workspace, feel free to review / patch in the issue queue there. I'm sure I could talk @_dixon into giving out commit access to some people too.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
215.58 KB

#38.1 - This is kinda inherited from CouchDB which is what the replication protocol is based on. With couchdb you can pass include_docs=true as a parameter to get changes including the documents. Changed to entities here, as it makes more sense. We'll map to include_docs when we come to rewrite Relaxed module in contrib.

#38.2 - Updated the fields to not be translatable. We have a similar issue in Content Moderation #2881645: ContentModerationState 'workflow' column empty for all non-default translations.. The entity type is still translatable (even though it has no translatable fields). Will think about if there is a need for this still.

#38.3 - #1979260: Automatically populate the author default value with the current user.

#38.4 - I'm not sure what you mean.

#38.5 - Updated.

#38.6 - This was to get the default workspace param, changed to inject it to the service.

#38.7 - Fixed.

#38.8 - Fixed.

#38.9 - The ReplicationLog is used at the start of replication to know which sequence ID we last replicated up until. Then when the next replication is done we only replicate changes since the last replication.

#38.10 - getEntityTypesThatCanBelongToWorkspaces() is a bit long isn't it? but maybe we could change the others to isEntityTypeSupported() and isEntitySupported()?

#38.11 - (For context, this was inherited from https://github.com/dickolsson/drupal-multiversion/blame/8.x-1.x/src/Work...) Ideally we should throw a 403, but it can be complex from here.

timmillwood’s picture

A few tidy ups.

Status: Needs review » Needs work

The last submitted patch, 41: 2784921-41.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
214.51 KB

Got hit by the Node save button changes.

timmillwood’s picture

FileSize
8.15 KB
214.24 KB

Adding missing "administer workspaces" permission and fixing issue with two submit buttons on the deployment form, then updating replication test to use the deployment form rather than the API directly.

yoroy’s picture

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

timmillwood’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/workspace/workspace.module
    @@ -0,0 +1,428 @@
    +  // @todo Don't use distinct.
    

    And we're not using distinct anymore.

  2. +++ b/core/modules/workspace/workspace.module
    @@ -0,0 +1,428 @@
    +  //$query->aggregationMethodSimple($entity_type->getDataTable(), $entity_type->getKey('id'));
    

    This can be removed too

  3. +++ b/core/modules/workspace/workspace.module
    @@ -0,0 +1,428 @@
    +  foreach ($view->displayHandlers->getInstanceIds() as $display_id) {
    +    $view->displayHandlers->get($display_id)->setOption('group_by', TRUE);
    +  }
    

    We also saw /admin/content not working, and this is why.

pk188’s picture

Status: Needs work » Needs review
FileSize
213.63 KB

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs review » Needs work

Moving back to "Needs work" because #47 didn't fix anything useful.

amateescu’s picture

FileSize
213.74 KB
22.27 KB

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: Replication and conflict management.

    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?
timmillwood’s picture

#50.1 - The upstream is the place to replicate to, because the replication is flexible (it's a tagged service), the upstream needs to be equally flexible. It might not be a local workspace, it might not even be Drupal at all.

In contrib land the Workspace module generates WorkspacePointer content entities for each place you can replicate to, and entity reference field on the workspace entity links to the workspace pointer. This method isn't ideal, and plugins make more sense, but as you say it need a lot more documentation, and I guess a test module implementing another plugin would be useful.

#50.2 - Yes, I guess we could. The replication has always followed the CouchDB protocol (http://docs.couchdb.org/en/2.0.0/replication/protocol.html), this is a tried and tested protocol for replication so it'd be good if we could follow that as close as possible, but doing it in the form could work initially.

#50.3 - yes, that was left over from the WorkspacePointer entity type, which was removed in favour of the upstream plugin.

#50.4 - yes, that'd be good. In the mean time it can remain here.

#50.5 - ok, that was left over from contrib.

#50.6 - This is a permission to allow users to pull changes down from the upstream. We should use it.

amateescu’s picture

#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: [PP-1] Add a 'machine name' widget for text/string field types 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.

timmillwood’s picture

#521, #52.2, #52.3 - I guess it's obvious I just copied the entity from Content Moderation. I have been thinking, maybe we should take another look at uninstalling base fields (which I can't find the issue for),

timmillwood’s picture

Been playing around with uninstalling base fields in #2905944: Uninstall modules which define base fields so we can remove the ContentWorkspace entity and just add a workspace base field.

timmillwood’s picture

#52.4 - Yes!
#52.5 - To denote it as a local (non-replicating) document, something inherited from CouchDB, maybe we can remove this and have something in contrib http://docs.couchdb.org/en/2.0.0/api/local.html, or maybe we do need it, as ReplicationLog entities belong to workspaces, but don't get replicated to other workspaces.
#52.6 - It could just be a string ID field, it's based on the CouchDB replication ID http://docs.couchdb.org/en/2.0.0/replication/protocol.html#generate-repl...
#52.7 - The initial plan was to move everything from key_value modules directly into core, thus the flexibility was good, but it was decided to put it into Workspace, so maybe we can make it less flexible.
#52.8 - This is inherited from key_value module https://github.com/dickolsson/drupal-key_value/blob/8.x-1.x/src/KeyValue... but I guess it should extend KeyValueSortedSetInterface.
#52.9 - This is an inherited feature from Replication module https://github.com/relaxedws/drupal-replication/tree/8.x-1.x/src/Plugin/...
#52.10 - yes
#52.11 - Just so we're not setting a single integer as a cache context (I guess) it's inherited from Multiversion https://github.com/dickolsson/drupal-multiversion/blob/8.x-1.x/src/Works...
#52.12 - Black list of entity types that don't / can't belong to a workspace. So a workspace can't belong to a workspace.
#52.13 - I guess we can do away with the first one.

timmillwood’s picture

FileSize
5.82 KB
212.52 KB

Documenting more details around the replication tagged services and the upstream plugin.

xjm’s picture

Category: Task » Feature request
Priority: Normal » Major

This is a new feature, also major.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.47 KB
198.52 KB

Initial work to remove the ContentWorkspace entity and move to a WorkspaceReference field, this field will be uninstallable after #2282119: Make the Entity Field API handle field purging.

Status: Needs review » Needs work

The last submitted patch, 58: 2784921-58.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.49 KB
200.49 KB

Missed new files from the patch.

Status: Needs review » Needs work

The last submitted patch, 60: 2784921-60.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
8.42 KB
202.73 KB

Having some issues trying to fix the failing tests, but thought a progress patch might be useful.

Status: Needs review » Needs work

The last submitted patch, 62: 2784921-62.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
FileSize
202.72 KB

Let's try that again!

Status: Needs review » Needs work

The last submitted patch, 64: 2784921-64.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

FileSize
5.85 KB
204.21 KB

Adding some inline comments for hook_query_TAG_alter, hook_views_query_alter, and hook_entity_load implementations, to try and explain what we're trying to do.

Leaving as "Needs work" because only comments have changed, the outstanding issues / test fails still exist.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
6.72 KB
203.71 KB

Mainly a progress patch, but removing last mentions of the content_workspace entity, and trying to progress some of the test failures (even if it's just moving them to fail at a later part in the test).

Status: Needs review » Needs work

The last submitted patch, 67: 2784921-67.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

webchick’s picture

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

timmillwood’s picture

@dixon_ @amateescu and I have been discussing workspaces this morning and the main point which hasn't been clear so far is a specific revision can belong to one or more workspaces. So if we go down the route of a 'workspace' base field, it'll need to be able to handle multiple values.

It's also worth noting that replicating content from one workspace to another will still leave the source workspace usable (it won't be archived) and replication will work slightly differently depending on if you're replicating to the default/live workspace compared to another workspace. When a revision is replicated to live it then becomes the default revision and belongs to all workspaces. When replicating to another workspace the target workspace id will just be added to the workspace field, updating an existing revision and not changing the default revision.

catch’s picture

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?

amateescu’s picture

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.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
214.04 KB

In the Workspace discussions at DrupalCon this morning we discussed to run with the content_workspace linking entity for now. So this patch is a combination of the patch from #56 and the interdiff from #66.

Status: Needs review » Needs work

The last submitted patch, 73: 2784921-73.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
9.93 KB
214.7 KB

Fixing issues with:
- missing index in quickedit
- urls not as string for destination in WorkspaceListBuilder
- Coding standards in workspace.module

Status: Needs review » Needs work

The last submitted patch, 75: 2784921-75.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
547 bytes
214.69 KB

Fixing mistake in quickedit.module

amateescu’s picture

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.

dixon_’s picture

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.

catch’s picture

Title: Introduce the concept of workspaces » Add workspaces experimental module

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

andrewmacpherson’s picture

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.

amateescu’s picture

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

amateescu’s picture

Title: Add workspaces experimental module » Add Workspaces experimental module
FileSize
217.78 KB
286.61 KB

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

Status: Needs review » Needs work

The last submitted patch, 83: 2784921-83-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
222.1 KB
289.75 KB
8.1 KB

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

dixon_’s picture

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

amateescu’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 87: 2784921-87-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs work » Needs review
FileSize
228.72 KB
296.36 KB
1.04 KB

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

borisson_’s picture

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.

amateescu’s picture

@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.
borisson_’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 91: 2784921-91-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs work » Needs review
FileSize
230.55 KB
298.19 KB
35.2 KB

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

dawehner’s picture

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 access handlers 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'

amateescu’s picture

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

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

jhedstrom’s picture

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

Sam152’s picture

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?

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs work » Needs review
FileSize
230.93 KB
298.57 KB
40.58 KB

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

Status: Needs review » Needs work

The last submitted patch, 99: 2784921-99-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dawehner’s picture

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?

amateescu’s picture

Status: Needs work » Needs review
FileSize
229.88 KB
297.53 KB
6.26 KB

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.

Status: Needs review » Needs work

The last submitted patch, 102: 2784921-102-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

catch’s picture

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.

amateescu’s picture

Status: Needs work » Needs review
FileSize
224.9 KB
295.46 KB
11.44 KB

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.

Status: Needs review » Needs work

The last submitted patch, 105: 2784921-105-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
231.82 KB
302.92 KB
67.46 KB

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: Consider whether workspace negotiators should expose cacheability metadata
#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! :)

Sam152’s picture

I found the issue mentioned in #98.9.

catch’s picture

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?

amateescu’s picture

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.

catch’s picture

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

dixon_’s picture

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

James Marks’s picture

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

amateescu’s picture

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: Figure out if we can provide a hardcoded Live workspace instead of creating it during installation 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

catch’s picture

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

Status: Needs review » Needs work

The last submitted patch, 115: 2784921-115-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs work » Needs review
FileSize
220.54 KB
222.62 KB
658 bytes

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

Status: Needs review » Needs work

The last submitted patch, 118: 2784921-118-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

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.

dixon_’s picture

@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: Figure out if we can provide a hardcoded Live workspace instead of creating it during installation.

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!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Status: Needs work » Needs review
FileSize
243.26 KB
109.52 KB

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.

Status: Needs review » Needs work

The last submitted patch, 123: 2784921-123.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

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!

amateescu’s picture

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

Status: Needs review » Needs work

The last submitted patch, 125: 2784921-125.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

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

Status: Needs review » Needs work

The last submitted patch, 128: 2784921-128.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

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

larowlan’s picture

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

amateescu’s picture

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: Remove table names from entity annotations 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.