Workspace association data is not updated when an entity is deleted

1. +++ b/core/modules/workspaces/src/WorkspaceAssociation.php
   @@ -212,9 +212,16 @@ public function postPublish(WorkspaceInterface $workspace) {
   +  public function deleteAssociations($workspace_id = NULL, $entity_type_id = NULL, $entity_ids = NULL, $revision_ids = NULL) {
   

   That's a pretty dangerous function. Do we have other functions in Core that allows deleting all entities of a given type? [e.g. if I call this with just an entity_type_id I would all entities (which are just the associations) across all workspaces for that type]

   So do we really want to have such a dangerous interface or is there a user for that case?

2. +++ b/core/modules/workspaces/workspaces.module
   @@ -143,6 +143,26 @@ function workspaces_entity_predelete(EntityInterface $entity) {
   +  if ($entity->getEntityTypeId() !== 'workspace') {
   +    \Drupal::service('workspaces.association')
   +      ->deleteAssociations(NULL, $entity->getEntityTypeId(), [$entity->id()]);
   +  }
   

   How does this prevent a workspace_association entity to recursively call this?

   Should this not check if the entity is supported by workspaces instead of just `!== workspace`?

   Feels wasteful to do a no-op delete query.

3. +++ b/core/modules/workspaces/workspaces.module
   @@ -143,6 +143,26 @@ function workspaces_entity_predelete(EntityInterface $entity) {
   +    \Drupal::service('workspaces.association')
   +      ->deleteAssociations(NULL, $entity->getEntityTypeId(), NULL, [$entity->getRevisionId()]);
   

   Is it safe to provide revision_id without entity_id?

   (In general I think yeah - just feels strange to me)

   I would probably pass also the entity_id as usually a query is for entity_id + revision_id (as long as appropriate indexes are on there).

4. Overall the patch is great and fixes a major unexpected bug. I think given that delete is always such a dangerous operation, I would add some more safety measures - like needing a entity_type + entity_id or (workspace_id) as required argument.

5.I also think it would be good to see that the entity_type / entity is even eligible to have been tracked by workspaces in the first place.

6. Also how can an entity right now be deleted from something else than the current workspace - e.g. should this not make the workspace_id of the entity being deleted explicit?

Thanks - great patch overall and a great improvement -- just some little details that are IMHO worth discussing.

Without patch:

Once the node is added and before delete

After delete

The node is still on the workspace association table.

With patch:

Once the node is added and before delete

After delete

The node is deleted from the workspace association table.

Given that without this we have data integrity issues - don't we need an update path to fix existing data?

If this is self-healing (as in workspace publishing and deletion removes the records anyway), then I think we should skip an upgrade path here altogether.

If it wasn't, I would probably suggest doing the upgrade path in a follow-up so that no new bad data is created asap anyway.

I think #14 is resolved, so I've gone ahead and committed/pushed to 10.1.x

Didn't backport to 10.0.x or 9.5.x due to the new API surface - given this is self-healing that doesn't seem urgent.

@catch I don't see any commit here and the issue's status is fixed.

@adriancid, it's there, Drupal.org posting back is/was broken:

git log --pretty=oneline | grep 3216107
b13dae8bd0d08a48af2aef2c7a55d35147da11af Issue #3216107 by amateescu, adriancid, alexpott, Fabianx: Workspace association data is not updated when an entity is deleted