Creating an unpublished entity in a workspace does not set the workspace field on the revision

This problem was also discovered in 🐛 [PP-1] Prevent content from being deleted when there is an active workspace Postponed . I will bring over the relevant changes from that issue to this one because the fix is a bit more involved than the current patch.

Let's start with explicit test coverage for this bug, and the fixes required for it.

The second patch will fail _other_ tests because we need to update that test coverage to account for the change we're introducing: when creating a new entity in a workspace, either published or unpublished, the initial revision will now be considered as part of the workspace.

Leaving at needs work for now, will followup with the complete patch.

I had these errors while testing the patch, so I added a validation to check for the existence of the element in the array:

• Warning: Undefined array key "test_delete_workspace" in Drupal\workspaces\WorkspaceAssociation->getAssociatedRevisions() (line 179 of /var/www/html/web/core/modules/workspaces/src/WorkspaceAssociation.php) 
• Warning: Trying to access array offset on value of type null in Drupal\workspaces\WorkspaceAssociation->getAssociatedRevisions() (line 179 of /var/www/html/web/core/modules/workspaces/src/WorkspaceAssociation.php) 
• TypeError: array_merge(): Argument #2 must be of type array, null given in array_merge() (line 179 of /var/www/html/web/core/modules/workspaces/src/WorkspaceAssociation.php)

Fixing a problem with undefined $workspace_candidates.

The test-only patch from #15 was flawed, it didn't take into account editing an existing entity in a workspace. Reworked the test coverage to cover all cases, and this shows that we can't check for "initial revisions" (of the entities that are created in a workspace) at the moment.

These are the fixes needed for the test-only patch from #18. As mentioned in #15, some other tests will fail because workspaces are now tracking initial default revisions, and I will follow up with a complete patch.

Here are the changes needed to fix the existing test coverage. This is fully ready for reviews now.

We are using this patch in a production site and is working as expected. I know usually it needs some screenshots but I can confirm is working.

There's some API changes here (new arguments to plugin constructors, new methods).
Whilst we don't have to provide a BC layer, it would be nice to add a change notice, so tagged for that.

We've not had a test-run that shows the new tests fail yet (they didn't pass the custom checks so tests didn't run) - so can we get a red/green set of patches - ie one with just the tests, and one with tests and fixes.

Couple of minor observations

1. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -192,6 +200,42 @@ public function getAssociatedRevisions($workspace_id, $entity_type_id, $entity_i
   +  public function getAssociatedInitialRevisions($workspace_id, $entity_type_id, $entity_ids = NULL) {
   
   +++ b/core/modules/workspaces/src/WorkspaceAssociationInterface.php
   @@ -74,6 +74,23 @@ public function getTrackedEntities($workspace_id, $entity_type_id = NULL, $entit
   +  public function getAssociatedInitialRevisions($workspace_id, $entity_type_id, $entity_ids = NULL);
   

   Can we get typehints here for new code

2. +++ b/core/modules/workspaces/src/WorkspaceManager.php
   @@ -316,31 +316,60 @@ public function purgeDeletedWorkspacesBatch() {
   +          $associated_entity_storage->delete([$associated_entity_storage->load($initial_revision_ids[$revision_id])]);
   

   should we try to do this in bulk outside the foreach loop?

Good point about the CR, wrote one here: https://www.drupal.org/node/3352703 →

Attaching the test-only patch from #18 with the custom checks fixed, as well as the complete one from #20 with the point from #22.1 fixed.

I thought a lot about #22.2 while working on this patch and eventually settled on the argument that this is "just" cleanup code that usually runs on cron, and that it's better to be correct and easy to read rather than fast. Loading multiple default revisions would only save a few cache gets anyway :)

The last submitted patch, 23: 3129762-23-test-only.patch, failed testing. View results →

This is tricky but the changes look solid and agreed the CR is plenty for the changes.

Committed/pushed to 10.1.x, thanks!

I published the change record

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