Track the workspace of a revision in a base field and convert the workspace_association entity type to a custom index

Fixed remaining tests.

Patch #3 only deleted the currently indexed (latest) revision for a workspace. But I suppose it should delete all revisions? I changed that in Workspace::postDelete().

Re-rolled the broken patch. Discovered dorgflow. My life is a lot easier now.

I tried to get hold of the last failing test. But I can't tell what's wrong. It seems like the EntityDefinitionUpdateManager tries to remove the workspace field twice, and this results in a fatal error when uninstalling the module.

I finally was able to track down the problem:

Removing the revision metadata key in workspaces_uninstall causes the entity updated manager to think workspace is a not-revisionable field and tries to remove it from the regular data table, which in return causes the fatal error.

My current approach to solve this is to remove the revision metadata key in system_modules_uninstalled instead. I was able to verify this manually, but unfortunately not in a test case since I was not able to get updated entity information. I'm happy for any suggestions!

Workspaces cause EntityStorageBase::loadMultiple to return entities that weren't requested, i.e. their ids were not on the list, see #3068733: EntityStorageBase::loadMultiple returns unwanted entities when the static cache is warm. The Drupal\workspaces\EntityOperations::entityPreload is refactored in this issue but the problem is still there, so adding the test here as well.

Re-roll

Git is hard :D

Ok, it looks like #3068733: EntityStorageBase::loadMultiple returns unwanted entities when the static cache is warm can be fixed in EntityStorageBase::loadMultiple and no changes to Drupal\workspaces\EntityOperations::entityPreload are required. In this case these issues can probably be dispatched independently, so removing the files added after #16.

Posting an initial review, leaving as NR to encourage others to take a look.

1. +++ b/core/modules/system/system.module
   @@ -1441,3 +1441,22 @@ function system_element_info_alter(&$type) {
   +/**
   + * Implements hook_modules_uninstalled().
   + */
   +function system_modules_uninstalled($modules) {
   +  if (!in_array('workspaces', $modules, TRUE)) {
   +    return;
   +  }
   +
   +  $entity_definition_update_manager = \Drupal::entityDefinitionUpdateManager();
   +  foreach ($entity_definition_update_manager->getEntityTypes() as $entity_type) {
   +    $revision_metadata_keys = $entity_type->get('revision_metadata_keys');
   +    if ($revision_metadata_keys && array_key_exists('workspace', $revision_metadata_keys)) {
   +      unset($revision_metadata_keys['workspace']);
   +      $entity_type->set('revision_metadata_keys', $revision_metadata_keys);
   +      $entity_definition_update_manager->updateEntityType($entity_type);
   +    }
   +  }
   +}
   

   Should there be a @todo and a follow-up to create an API for modules to update schema after they are uninstalled? Maybe also needs to explain why hook_uninstall wasn't used?

2. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,155 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getAssociatedRevisions($workspace_id, $entity_type_id, $entity_ids = NULL) {
   

   Part of the motivation for this issue was to side-step entity API when managing workspaces, but doesn't the workspace field on each entity revision reintroduce that problem to some degree?

3. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,155 @@
   +    $entity_type = $storage->getEntityType();
   +    $workspace_field = $entity_type->get('revision_metadata_keys')['workspace'];
   +    $id_field = $entity_type->getKey('id');
   

   Should this be using the installed definition? Possibly causes issues on a site between deploy => updb or sites or where updates are stuck?

4. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,155 @@
   +    // If the workspace ID is NULL, query for the 'live' workspace.
   

   The docblock doesn't document NULL as a possible value.

5. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,155 @@
   +    // Restrict the result to a set of entity ID's if provided.
   +    if ($entity_ids) {
   +      $query->condition($id_field, $entity_ids, 'IN');
   +    }
   +
   +    return $query->execute();
   

   Why would some code use the index and some code use the revision metadata field? As a broad question, why do both have to exist? Is it possible to answer that question through the implementation somehow?

6. +++ b/core/modules/workspaces/src/WorkspaceAssociationInterface.php
   @@ -0,0 +1,92 @@
   +interface WorkspaceAssociationInterface {
   

   With 7.1 being the new minimum supported version, I wonder if new interfaces can start making use of features like scalar type hints?

7. +++ b/core/modules/workspaces/src/WorkspacePublisher.php
   @@ -100,6 +102,11 @@ public function publish() {
   +            // The default revision is not workspace-specific anymore.
   +            $field_name = $entity->getEntityType()->getRevisionMetadataKey('workspace');
   +            $entity->{$field_name}->target_id = NULL;
   

   Is removing 'live' a blocker for this?

8. +++ b/core/modules/workspaces/tests/src/Functional/Update/WorkspacesUpdateTest.php
   @@ -0,0 +1,105 @@
   +      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.filled.standard.php.gz',
   +      __DIR__ . '/../../../fixtures/update/drupal-8.6.0-workspaces_installed.php',
   

   I'm surprised this works TBH, I've seen these updates blow up when you combine an 8.6 module with 8.0+ updates.

9. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceIntegrationTest.php
   @@ -275,7 +275,7 @@ public function testWorkspaces() {
   -    $expected_workspace_association['add_published_node_in_stage'] = ['stage' => [3, 4, 5, 6, 7]];
   +    $expected_workspace_association['add_published_node_in_stage'] = ['stage' => [3, 4, 5, 7]];
   

   Why did this test case change?

10. +++ b/core/modules/workspaces/workspaces.install
    @@ -67,3 +90,83 @@ function workspaces_install() {
    +        'length' => 128,
    

    How come workspaces have a large ID than EntityTypeInterface::ID_MAX_LENGTH?

11. +++ b/core/modules/workspaces/workspaces.post_update.php
    @@ -5,8 +5,133 @@
    +    $database->schema()->dropTable('workspace_association');
    +    $database->schema()->dropTable('workspace_association_revision');
    

    How come this is required?

Nice, changes and explanations look fine to me. Leaving NR for other reviews.

I checked the code and did some manual testing (including the update process). Could not find any issues with the patch. Looks very good to me!

The revision metadata field is a nice way to keep history and seems an OK trade-off having to maintain a custom index table for that.

1. +++ b/core/modules/workspaces/src/EventSubscriber/EntitySchemaSubscriber.php
   @@ -0,0 +1,147 @@
   +      // through the entity entity update process.
   +      $this->entityLastInsytalledSchemaRepository->setLastInstalledDefinition($entity_type);
   +    }
   +  }
   +
   +  /**
   

   entity entity

2. +++ b/core/modules/workspaces/src/WorkspaceAssociationInterface.php
   @@ -0,0 +1,97 @@
   + * Defines an interface for the workspace_association service.
   

   This could use a bit more documentation - i.e. that it's an index table and the canonical associations are in the revision metadata or similar.

3. +++ b/core/modules/workspaces/src/WorkspaceManager.php
   @@ -267,67 +277,42 @@ public function purgeDeletedWorkspacesBatch() {
   -        $associated_entity_storage = $this->entityTypeManager->getStorage($workspace_association->target_entity_type_id->value);
            // Delete the associated entity revision.
   -        if ($entity = $associated_entity_storage->loadRevision($workspace_association->target_entity_revision_id->value)) {
   +        if ($entity = $associated_entity_storage->loadRevision($revision_id)) {
              if ($entity->isDefaultRevision()) {
                $entity->delete();
              }
   

   I didn't realise we were doing this, but do we really want to have this revision purging logic in core? Is this a specific purge operation or does it happen whenever a workspace is deleted? Also how can a default revision be part of a workspace and why would we ever delete that here?

Got interrupted during the last review, here's a few more things.

1. +++ b/core/modules/system/system.module
   @@ -1441,3 +1441,25 @@ function system_element_info_alter(&$type) {
   +function system_modules_uninstalled($modules) {
   +  // @todo Remove this when modules are able to maintain their revision metadata
   +  //   keys.
   +  //   @see https://www.drupal.org/project/drupal/issues/3074333
   +  if (!in_array('workspaces', $modules, TRUE)) {
   +    return;
   +  }
   

   Oof, but following the follow-up and don't think it should block this.

2. +++ b/core/modules/workspaces/src/EventSubscriber/EntitySchemaSubscriber.php
   @@ -0,0 +1,147 @@
   +      // through the entity entity update process.
   

   entity entity is still here (or maybe another instance?)

3. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,156 @@
   +   */
   +  public function getTrackedEntities($workspace_id, $entity_type_id = NULL, $entity_ids = NULL) {
   +    $query = $this->database->select(static::TABLE, 'wa');
   +    $query
   +      ->fields('wa', ['target_entity_type_id', 'target_entity_id', 'target_entity_revision_id'])
   

   Minor, but there's no join here so why alias?

4. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,156 @@
   +   */
   +  public function getAssociatedRevisions($workspace_id, $entity_type_id, $entity_ids = NULL) {
   +    /** @var \Drupal\Core\Entity\EntityStorageInterface $storage */
   +    $storage = $this->entityTypeManager->getStorage($entity_type_id);
   +    $entity_type = $storage->getEntityType();
   +    $workspace_field = $entity_type->get('revision_metadata_keys')['workspace'];
   +    $id_field = $entity_type->getKey('id');
   +
   +    $query = $storage->getQuery()->allRevisions();
   +
   +    if ($workspace_id !== NULL) {
   +      $query->condition($workspace_field, $workspace_id);
   +    }
   +    else {
   +      // If the workspace ID is NULL, query for the revisions which are not
   +      // associated with any workspace.
   +      $query->notExists($workspace_field);
   +    }
   +
   +    // Restrict the result to a set of entity ID's if provided.
   +    if ($entity_ids) {
   +      $query->condition($id_field, $entity_ids, 'IN');
   +    }
   +
   +    return $query->execute();
   +  }
   

   This looks like it's missing an ->accessCheck(FALSE)

5. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -0,0 +1,156 @@
   +   */
   +  public function getEntityTrackingWorkspaceIds(RevisionableInterface $entity) {
   +    $query = $this->database->select(static::TABLE, 'wa');
   +    $query
   

   Same question with the alias and no join. Or really why not just a static query in these two places since there's nothing dynamic about the query? But then asking that table name is a constant which makes a static query harder, but does it really need to be a constant? The thing that made me think about this in the first place was using the constant, then having to use a literal string for the alias immediately afterwards which slightly undermines having a constant.

6. +++ b/core/modules/workspaces/src/WorkspaceAssociationInterface.php
   @@ -0,0 +1,104 @@
   +   * Deletes all the association entries for the given workspace.
   

   Should be 'associated entities'.

7. +++ b/core/modules/workspaces/src/WorkspaceManager.php
   @@ -267,67 +277,35 @@ public function purgeDeletedWorkspacesBatch() {
        // associated with it, along with the workspace_association entries.
   

   This still says workspace_association entities. Use 'records' instead?

8. +++ b/core/modules/workspaces/workspaces.install
   @@ -67,3 +90,83 @@ function workspaces_install() {
   + */
   +function workspaces_schema() {
   +  $schema['workspace_association'] = [
   

   Since this is owned by the service, would it make sense to do the 'lazy table creation' logic that key/value and database cache backends do? We've slowly been getting rid of core usages of hook_schema().

As for default revisions being part of a workspace, I think that code is quite old and it was probably needed in some initial version of the contrib module which was tracking revisions for the Live workspace. However, since we no longer do that, we can definitely clean it up a bit, very good catch!

When a revision is made 'live'/default, is it definitely never associated with a workspace at that point (except possibly the now-defunct 'live' workspace?) - if so this might be OK, but if not then deleting previously-default revisions feels really destructive. I realise this code is already there but it'd be good to open a spin-off issue to discuss more yeah.

(also patch isn't applying).

Note that in #36 I removed the code which could delete default revisions, so we are only deleting pending deletions now.

This brings up another question for me which is perhaps more relevant to the patch.

When we only stored workspace_association entities, and delete a workspace when it's published, the workspace association entities get deleted too. If you later delete the workspace itself, there are no associations, so the revisions won't get deleted.

But now, the workspace associations remain after the workspace is published, so if you delete an old, published workspace, then those revisions (which are the 'source' revisions for published content, not abandoned drafts) will get deleted. Or if that's not a correct summary what exactly does happen now?

I think I prefer option #2. It should be possible to implement a retrospective revision purge logic if we eventually want to, but we can't bring data back. Marking needs work for that.

All points from previous reviews have been addressed. And to me the patch and the interdiff looks good as well!

I'm not too opinionated on how/if we delete revisions when a workspace is deleted, so I'm happy to discuss other solutions. But I like the current approach in the patch of only deleting the forward revisions when the workspace itself is deleted. It feels like a clean solution aligned with what I think most users would expect when deleting a workspace.

Deleting just the forward revisions seems reasonable here and we can discuss further changes in the follow-up.

Committed 17bd9ce and pushed to 8.8.x. Thanks!

Looks like this broke PostGre on HEAD. https://www.drupal.org/pift-ci-job/1410286

Reverted for now. Will kick off postgres and sqlite test runs here.

Committed 6caa28c and pushed to 8.8.x. Thanks!

This probably needs at least a CR.

+++ b/core/modules/workspaces/src/EntityTypeInfo.php
@@ -84,4 +91,30 @@ public function fieldInfoAlter(&$definitions) {
+      $fields[$field_name] = BaseFieldDefinition::create('entity_reference')
+        ->setLabel(new TranslatableMarkup('Workspace'))
+        ->setDescription(new TranslatableMarkup('Indicates the workspace that this revision belongs to.'))
+        ->setSetting('target_type', 'workspace')
+        ->setInternal(TRUE)
+        ->setTranslatable(FALSE)
+        ->setRevisionable(TRUE);

+++ b/core/modules/workspaces/workspaces.install
@@ -61,3 +84,83 @@ function workspaces_install() {
+      $field_storage = BaseFieldDefinition::create('entity_reference')
+        ->setLabel(t('Workspace'))
+        ->setDescription(t('Indicates the workspace that this revision belongs to.'))
+        ->setSetting('target_type', 'workspace')
+        ->setInternal(TRUE)
+        ->setTranslatable(FALSE)
+        ->setRevisionable(TRUE);

I was wondering should we enforce max_length here as workflow ID has ->setSetting('max_length', 128) but the question is can we enforce lenght on ER field target_id field?

I was wondering should we enforce max_length here as workflow ID has ->setSetting('max_length', 128) but the question is can we enforce lenght on ER field target_id field?

We limit the length on the target_id property to 255 by default or to 32 when referencing a bundle entity type. Instead of hardcoding the value we could retrieve it from the target type ID definition. This will be not workspaces specific then. Could you please open an issue for this?

+++ b/core/modules/workspaces/workspaces.install
@@ -32,6 +34,27 @@ function workspaces_requirements($phase) {
+ * Implements hook_module_preinstall().
...
+function workspaces_module_preinstall($module) {
+  if ($module !== 'workspaces') {
+    return;

This hook should be moved to .module file so this condition is useless, faced in #3118155-20: Standardize hook_module_preinstall to use $is_syncing parameter instead of direct service calls