Problem/Motivation

In #2784921-137: Add Workspaces experimental module @catch presented a use case where a user may wish to pull content from one workspace, and push to a different workspace. In the analogy of git this may be similar to a merge.

The initial patch for the Workspace module in #2784921: Add Workspaces experimental module will only allow content replication to and from the live workspace. Therefore we need to look at how replication will work with other workspaces from both a technical and user point of view too.

In this issue we can discuss the possibly uses cases of content replication, and how users would like to be able to push, pull, and merge content between workspaces.

Proposed resolution

What we could do is change the repository plugins' purpose to only handle cross-site replication, and provide two additional services, one that will handle merging local workspaces and the other one to handle publishing a workspace to 'live'.

We should also have a new merge($workspace_id) method on the workspace entity, which will essentially bring all the revisions of $workspace_id into the current workspace, handle conflicts, etc.

And we probably want a publish() method as well, so we can execute the "publish to live" action from the context of a non-live workspace.

CommentFileSizeAuthor
#17 interdiff-17.txt1.44 KBamateescu
#17 2958752-17.patch41.78 KBamateescu
#15 interdiff-15.txt1.95 KBamateescu
#15 2958752-15.patch38.12 KBamateescu
#11 interdiff-11.txt631 bytesamateescu
#11 2958752-11.patch37.88 KBamateescu
#9 2958752-9.patch37.86 KBamateescu

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Issue summary: View changes
amateescu’s picture

amateescu commented

@dixon, @timmillwood and myself discussed this issue a couple of days ago and I think that @catch brings up a very good point: merging between local workspaces and publishing the pending revision of a workspace to live will always be done in the same way, so there's no reason for them to be plugins.

So what we could do is change the repository plugins' purpose to only handle cross-site replication, and provide two additional services, one that will handle merging local workspaces and the other one to handle publishing a workspace to 'live'.

We should also have a new merge($workspace_id) method on the workspace entity, which will essentially bring all the revisions of $workspace_id into the current workspace, handle conflicts, etc.

And we probably want a publish() method as well, so we can execute the "publish to live" action from the context of a non-live workspace.

amateescu’s picture

amateescu commented
Project: Drupal core » Workspace
Version: 8.6.x-dev » 8.x-2.x-dev
Component: entity system » Code
Priority: Normal » Critical

We can start working on this in the contrib branch, so moving to that queue for now. And promoting to critical because this is a beta blocker for the core Workspace module.

amateescu’s picture

amateescu commented
Project: Workspace » Drupal core
Version: 8.x-2.x-dev » 8.6.x-dev
Component: Code » workspace.module
Priority: Critical » Major
catch’s picture

catch
he/him
Primary language English
commented

So what we could do is change the repository plugins' purpose to only handle cross-site replication, and provide two additional services, one that will handle merging local workspaces and the other one to handle publishing a workspace to 'live'.

+1.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

This issue is a "Discuss" issue, and maybe should've been given the "Plan" category.

With @catch's "+1" in #6 I feel we have consensus for some action. This discussion issue is a beta blocker, but is actioning on this discussion also a blocker for beta? Should we close this and open a follow up to implement the merge functionality?

catch’s picture

catch
he/him
Primary language English
commented
Title: Discuss workspace content replication use cases » Refactor workspace content replication plugin system to three services
Issue summary: View changes

So #3 is very similar to what I thought of when I reviewed the original patch.

One service/method for 'publish a workspace'.

One service/method for 'merge two workspaces'.

Only use plugins for the remote publishing stuff (from a third service?)

Since this issue is short, going to try just retitling it and adding #3 to the issue summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2958752-9.patch37.86 KB

Here's an initial stab at this.

It removes the concept of repository plugins entirely, and provides a "workspace operation factory" service along with a "workspace publisher" class, since that's the only thing we support right now.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/workspace/src/WorkspaceOperationFactory.php
@@ -0,0 +1,50 @@
+    return new WorkspacePublisher($this->entityTypeManager, $this->database, $source);

    Should we cache this here or in WorkspaceDeployForm? Because it's called on line 79 and line 118, each time creating a new instance of the class. 

  2. +++ b/core/modules/workspace/src/WorkspacePublisher.php
@@ -145,9 +103,15 @@ public function push() {
+    $this->sourceWorkspace->label();
...
+    $this->targetWorkspace->label();

    Missing return here.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2958752-11.patch37.88 KB
new interdiff-11.txt631 bytes

1. That class doesn't really have a long instantiation process, so I don't think it needs any caching.

2. Good catch, fixed :)

catch’s picture

catch
he/him
Primary language English
commented 
+++ b/core/modules/workspace/src/WorkspacePublisherInterface.php
@@ -0,0 +1,15 @@
+
+  /**
+   * Publishes the contents of a workspace to the default (Live) workspace.
+   */
+  public function publish();
+

Why a factory that returns an instance of this class, instead of a direct service with a publish($workspace) method?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

@catch - I asked this same question to Andrei in slack, he said:

we need to pass in somehow the workspace object that we need to publish
so methods like `checkConflictsOnTarget()` work without any parameters
so I thought that a factory is the only option

The only other option I thought of was a useWorkspace($workspace) or setWorkspace($workspace) method, but that way you can't ensure it's been done, so would need some form of check in each method that the workspace was set.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented 
+++ b/core/modules/workspace/src/WorkspaceOperationFactory.php
@@ -0,0 +1,50 @@
+class WorkspaceOperationFactory {

+++ b/core/modules/workspace/src/WorkspacePublisher.php
@@ -1,35 +1,24 @@
+class WorkspacePublisher implements WorkspacePublisherInterface {

Do you think we should mark these as @internal?
1) I don't think custom/contrib code should call them directly (at least initially)
2) What if we need to change them before 8.7? I'm thinking once we implement Relaxed module in contrib and find something we didn't think of.

amateescu’s picture

amateescu commented
StatusFileSize
new 2958752-15.patch38.12 KB
new interdiff-15.txt1.95 KB

Good point, let's do that :)

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Reviewed & tested by the community

Awesome, looks good to me!

amateescu’s picture

amateescu commented
StatusFileSize
new 2958752-17.patch41.78 KB
new interdiff-17.txt1.44 KB

I was doing a self-review for this patch and I noticed that there were two outstanding @todo's pointing to this very issue :)

Since we don't have the generic concept of repository handlers anymore, I think the documentation for the return values of WorkspaceOperationInterface::getDifferringRevisionIdsOnTarget() and WorkspaceOperationInterface::getDifferringRevisionIdsOnSource() doesn't need to mention the possible variations (revision IDs vs. revision UUIDs). It's up to each implementation of WorkspaceOperationInterface to return whatever identifier they need and handle it consistently.

amateescu’s picture

amateescu commented
Parent issue: » #2732071: WI: Workspace module roadmap
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

Nice spot @amateescu.

Still RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

OK this looks great. I don't love the factory, but if we think of something better later, we can change it then. Can't think of a good way around it at the moment.

Committed 0968d28 and pushed to 8.6.x. Thanks!

  • catch committed 0968d28 on 8.6.x 
    Issue #2958752 by amateescu, timmillwood, catch: Refactor workspace...
amateescu’s picture

amateescu commented
Component: workspace.module » workspaces.module

Fix component following module rename.

Status: Fixed » Closed (fixed)

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