Remove the concept of a 'live' default workspace

I think per the comment:

This not only created on demand, it is a memory-only representation of the default workspace, meaning that it's not saved to a storage anywhere, so there's no chance of conflicting IDs.

This can be closed now as fixed or not?

The only other approach I can see for this is to not have a live workspace entity at all. From a mental model point of view, instead of switching to "Live" you would essentially switch out of "Stage". That may also clean up some of WorkspaceInterface, for example publish makes no sense for the live workspace, and isDefaultWorkspace would become irrelevant.

This seems like a pretty big shift though, so I suppose the question should be: has there been any kind of significant issue or risk with the installed live workspace disappearing or being deleted? How recoverable is workspaces if that is deleted?

Just a quick review / feelings after a first read of the patch:

+++ b/core/modules/workspaces/src/Form/WorkspaceDeactivateForm.php
@@ -0,0 +1,49 @@
+class WorkspaceDeactivateForm extends WorkspaceActivateForm implements WorkspaceFormInterface {

While it's true that this is what submitting this form does, I cant stop thinking about the difference is between deactivating a workspace vs switching to live.

Deactivating the workspace is what is actually happening in the system, in the code.

Switching to live is what the user actually wants to do, what the action means for the user anyway.

So I think naming everything as the user action would make sense to me. When someone tries to look for the form's code, it would be easier to find if it was called SwitchToLiveWorkspaceForm or something along those lines, but as I said, an argument could be made for keeping it like this - thoughts?

+++ b/core/modules/workspaces/src/Form/WorkspaceDeactivateForm.php
@@ -0,0 +1,49 @@
+/**
+ * Provides a form that deactivates a workspace.
+ */

This form description would also change if we decide to change the class name.

If not, I think Provides a form to deactivate a workspace. could be clearer.

diff --git a/core/modules/jsonapi/jsonapi.module b/core/modules/jsonapi/jsonapi.module
index 0e76517708..d0a58ecf9c 100644
--- a/core/modules/jsonapi/jsonapi.module
+++ b/core/modules/jsonapi/jsonapi.module
@@ -321,8 +321,6 @@ function jsonapi_jsonapi_user_filter_access(EntityTypeInterface $entity_type, Ac
  */
 function jsonapi_jsonapi_workspace_filter_access(EntityTypeInterface $entity_type, $published, $owner, AccountInterface $account) {
   // @see \Drupal\workspaces\WorkspaceAccessControlHandler::checkAccess()
-  // \Drupal\jsonapi\Access\TemporaryQueryGuard adds the condition for
-  // (isDefaultWorkspace()), so this does not have to.
   return ([
     JSONAPI_FILTER_AMONG_ALL => AccessResult::allowedIfHasPermission($account, 'view any workspace'),
     JSONAPI_FILTER_AMONG_OWN => AccessResult::allowedIfHasPermission($account, 'view own workspace'),
diff --git a/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php b/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php
index 5c6c9c8d4e..dd3b0a8e3d 100644
--- a/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php
+++ b/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php
@@ -15,7 +15,6 @@
 use Drupal\jsonapi\Query\EntityCondition;
 use Drupal\jsonapi\Query\EntityConditionGroup;
 use Drupal\jsonapi\Query\Filter;
-use Drupal\workspaces\WorkspaceInterface;
 
 /**
  * Adds sufficient access control to collection queries.
@@ -306,20 +305,6 @@ protected static function getAccessCondition($entity_type_id, CacheableMetadata
         // @see \Drupal\user\UserAccessControlHandler::checkAccess()
         $specific_condition = new EntityCondition('uid', '0', '!=');
         break;
-
-      case 'workspace':
-        // The default workspace is always viewable, no matter what, so if
-        // the generic condition prevents that, add an OR.
-        // @see \Drupal\workspaces\WorkspaceAccessControlHandler::checkAccess()
-        if ($generic_condition) {
-          $specific_condition = new EntityConditionGroup('OR', [
-            $generic_condition,
-            new EntityCondition('id', WorkspaceInterface::DEFAULT_WORKSPACE),
-          ]);
-          // The generic condition is now part of the specific condition.
-          $generic_condition = NULL;
-        }
-        break;
     }
 
     // Return a combined condition.

Yay!

+++ b/core/modules/workspaces/workspaces.post_update.php
@@ -10,3 +10,11 @@
+
+/**
+ * Remove the default workspace.
+ */
+function workspaces_post_update_remove_default_workspace() {
+  $workspace = \Drupal::entityTypeManager()->getStorage('workspace')->load('live');
+  $workspace->delete();
+}

While it makes sense to clean up, I can't shake this question out of my head: can this be dangerous for sites live using this workspace?

Also, should we bother checking that we indeed loaded the live workspace before trying to delete it?

Thanks @amateescu for the explanation, makes sense.

Patch is looking pretty good to me, just a couple more questions:

1. +++ b/core/modules/workspaces/workspaces.module
   @@ -198,9 +199,9 @@ function workspaces_toolbar() {
   diff --git a/core/modules/workspaces/workspaces.post_update.php b/core/modules/workspaces/workspaces.post_update.php
   
   diff --git a/core/modules/workspaces/workspaces.post_update.php b/core/modules/workspaces/workspaces.post_update.php
   index 2ca0ced399..0df5add2cb 100644
   
   index 2ca0ced399..0df5add2cb 100644
   --- a/core/modules/workspaces/workspaces.post_update.php
   
   --- a/core/modules/workspaces/workspaces.post_update.php
   +++ b/core/modules/workspaces/workspaces.post_update.php
   
   +++ b/core/modules/workspaces/workspaces.post_update.php
   +++ b/core/modules/workspaces/workspaces.post_update.php
   @@ -10,3 +10,12 @@
   
   @@ -10,3 +10,12 @@
     */
    function workspaces_post_update_access_clear_caches() {
    }
   +
   +/**
   + * Remove the default workspace.
   + */
   +function workspaces_post_update_remove_default_workspace() {
   +  if ($workspace = \Drupal::entityTypeManager()->getStorage('workspace')->load('live')) {
   +    $workspace->delete();
   +  }
   +}
   

   Do we need an update path test for this? My guess is it isn’t necessary, since we are just deleting one entity, but well doesn’t hurt to ask I suppose.

2. Should we document the changes to interfaces on the CR in case anyone is making use of them?

Re #22:
1. Yup, I can't think of anything else to test either by looking at the post_update function. The only fear I had with it was explained on #22 #20.

2. Thanks, had a read and the new sections are clear.

+++ b/core/modules/workspaces/src/Negotiator/SessionWorkspaceNegotiator.php
--- a/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php
+++ b/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php

+++ b/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php
+++ b/core/modules/workspaces/src/Negotiator/WorkspaceNegotiatorInterface.php
@@ -47,4 +47,9 @@ public function getActiveWorkspace(Request $request);

@@ -47,4 +47,9 @@ public function getActiveWorkspace(Request $request);
    */
   public function setActiveWorkspace(WorkspaceInterface $workspace);
 
+  /**
+   * Unsets the negotiated workspace.
+   */
+  public function unsetActiveWorkspace();
+
 }

Does this need documenting in the CR?

Thanks @amateescu - we now have a clear patch, a detailed CR and IS, so I'm RTBCing since I can't think of anything else to improve or that should be added.

Overall looks like a good simplification, couple of small things below:

1. +++ b/core/modules/workspaces/src/Access/ActiveWorkspaceCheck.php
   @@ -0,0 +1,50 @@
   +    $required_value = filter_var($route->getRequirement('_has_active_workspace'), FILTER_VALIDATE_BOOLEAN);
   

   Minor but do we really need to validate what's in routing yaml? And if so why not assert() to tell module developers they did something wrong?

2. +++ b/core/modules/workspaces/src/WorkspaceManager.php
   @@ -247,11 +273,23 @@ public function executeInWorkspace($workspace_id, callable $function) {
   +   * {@inheritdoc}
   +   */
   +  public function executeOutsideWorkspace(callable $function) {
   +    $previous_active_workspace = $this->getActiveWorkspace();
   +    $this->doSwitchWorkspace(NULL);
   

   Minor again, should $function be $callable?

3. +++ b/core/modules/workspaces/src/WorkspaceManagerInterface.php
   @@ -28,6 +28,14 @@ public function isEntityTypeSupported(EntityTypeInterface $entity_type);
    
   +  /**
   +   * Determines whether a workspace is active in the current request.
   +   *
   +   * @return bool
   +   *   TRUE if there is an active workspace, FALSE otherwise.
   +   */
   +  public function hasActiveWorkspace();
   +
      /**
   

The naming seems a bit ambivalent in that it could also mean 'is there an active workspace on the site', but I don't have a better idea.

However I think the return could be:

TRUE if a workspace is active, FALSE otherwise.

Only other idea would be reversing it to something like isLive()?

#28 1, 2, and 3 are all fair enough.

Committed d881bd0 and pushed to 8.8.x. Thanks!

The change record was still in draft state so I published it