Trash is not compatible with Content Moderation

Any progress here? We are using content_moderation and would love to have the 'trash' option available.

I am using Trash 3 with the patch from ✨ Translation support Needs review and Content Moderation without any issues like the one described here.
Our node translations have independent moderation states and the patch also allows for [mostly] independent trash states. (Deleting the source translation marks all translations as deleted, and restoring a translation requires that the source translation has also been restored.)
Any help reviewing that would be much appreviated. It may even be benificial if you don't need the actual translation support since your review could confirm that use case is still working, but it also improves revision handling.

I do however have an unrelated issue with revisions at the moment that I'm looking into. Specifically it's when a View is based on say the node revision table instead of the main node table (or pulls in the node revision table). Trash gets a bit confused about which "deleted" field to check and uses the main table name in stead of the revision table name, breaking the query. If I get any further with that I'll post it as a separate issue.

I based a relatively simple way to make Trash work nicer with Content Moderation based on ✨ Translation support Needs review as it already refactors entity operations - which should fix the issue in the OP - and it also refactors the storage trait to make this change easier.

Essentially it check if you're trying to trash or restore a non-default revision. If so, it performs the same operation on the existing default revision as well. That way if you have a draft and delete the entity, the published revision will become inaccessible.
If you then restore the entity it also restores the default revision along with the draft.
Because it already uses $entity->isSyncing(TRUE), Content Moderation will not attempt to change which revision is the default, or force a new one to be created.

It also decorates the moderation state validator to block any changes to the state while an entity is deleted.
Not had time to write any automated tests for this yet, but it seems to even work well with Workspaces (Core often creates links to blocked forms but I think's that's more to do with issues like 🐛 Views entity operations lack cacheability support, resulting in incorrect dropbuttons Needs work and 🐛 Operations links on the translations overview page do not link to the correct route Needs work ).

The PR against the translation branch makes it easier to see only the changes relevant here: https://git.drupalcode.org/issue/trash-3376216/-/merge_requests/1

Another revision（Content moderation） problem:
Delete an node with revisons, the node is not in trash, but can't be visited

Reproduced Steps:

1. Jump to comment: Most recent, Add new
2. Enable content moderation for node:article
3. Create an article and publish it
4. Create an draft for above node.
5. Delete the the node by /node/{nid}/delete
6. The node still in "/admin/content", not in "/admin/content/trash" but can't be viewed or edit

https://git.drupalcode.org/issue/trash-3376216/-/merge_requests/1 solved #5 ✨ Trash is not compatible with Content Moderation Active Thanks @TwoD

@xaqrox I can't reproduce this on a fresh install, tested both with content moderation enabled from the start, and then also with a new content type that initially didn't have moderation enabled. I'm still able to restore, so not sure if I am missing a step, or this has been resolved?

Merged in latest changes to ✨ Translation support Needs review .

Since the MR is not really reviewable because it's created on top of another one, here's a patch with the actual changes proposed in this issue.

And a quick review:

1. +++ b/src/TrashStorageTrait.php
   @@ -51,6 +51,17 @@ trait TrashStorageTrait {
   +          // Mark the default revision deleted, or it will remain accessible.
   +          $default = $this->getTrashManager()->executeInTrashContext('ignore', function () use ($entity) {
   

   This is very problematic in the context of Workspaces, because the current architecture of Trash provides the ability for editors to "stage" content deletions (!) in a workspace alongside other content changes.

   We'll have to come up with a different idea for Content Moderation integration, but I don't have a concrete proposal yet :/

2. +++ b/trash.module
   @@ -377,3 +378,15 @@ function trash_configurable_language_insert(EntityInterface $entity) {
   +function trash_form_content_moderation_entity_moderation_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   
   +++ b/trash.services.yml
   @@ -44,3 +44,8 @@ services:
   +  trash.state_transition_validation:
   

   Instead of decorating this service (+ a temporary form alter), I'm wondering whether we should decorate Drupal\content_moderation\ModerationInformation instead, and return FALSE in isModeratedEntity().

   It seems to me that would broaden the impact of this change to say "a deleted entity can not be moderated", and it should exclude it from all (or most?) moderation code paths.

I tried verifying the steps in issue summary and I cannot reproduce the problem. I don't receive any error after restoring an article from the trash. I tested this on a clean 10.3.x site install.

Can anyone else verify the issue, and maybe update the issue summary with what the problem actually is?

For those that weren't able to reproduce the issue, here are the steps to reproduce with a fresh Drupal 10.3 install.

1. Install and enable the content_moderation and trash module.
2. Edit the default Editorial workflow that installs with Content Moderation. (/admin/config/workflow/workflows/manage/editorial)
3. Under "This workflow applies to" select Basic Page. Save workflow.
4. Create a new Basic Page, save as Draft. Delete the page - it works, no problem.
5. Create a new Basic Page, save as Published. Delete the page - it works, no problem.
6. Create a new Basic Page, save as Published. Edit the page, save as Draft. Delete the page. View the page.

Error appears:

Warning: array_flip(): Can only flip string and integer values, entry skipped in Drupal\Core\Entity\ContentEntityStorageBase->loadMultipleRevisions() (line 667 of core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).

I can also reproduce this using the steps outlined by @chrisck

Also, I noticed the 'deleted' field needs to be uninstalled when the module is uninstalled. (The reverse of trash_update_9003().) Otherwise, the Status page will show an error for "Mismatched entity and/or field definitions" saying the "The Deleted field needs to be uninstalled."

daniel, can you create a new issue for that?

Hm, it’s not a separate issue though. It’s a problem with this issue’s merge request. I would think this MR should be updated so that it both installs and uninstalls the 'deleted' field instead of only installing it and thus causing the issue, no?

Ah, sorry. The MR seems ... massive. It looks like it combined work done in a separate issue for translation support. There's been a lot of progress lately in Trash, so I think at the very least it needs to be updated. The "deleted" field in the update hook doesn't make sense, the module today already provides deleted field. If you install and uninstall the trash module without this MR, does the deleted field linger still? If so, that's when a new issue should be made.

I just ran into this today where the latest revision on a published node that goes with workflows. Can revisions just be deleted like normal instead of trashed?

Actually it looks like if there is a future/forward revision, and you go to Delete content, the deleted flag only applies to the latest revision, and not the whole node (only saved to the node_field_revision table, and not the node_field_data table) which causes the issues.

@dave reid, I tried following your comments in #22, but I'm not quite sure with what you mean by a "future/forward" revision. When I revert to a prior version, a new version is created, like what happens with `git`.

I was finally able to reproduce the bug (Have to click the 'view" link, not just view (as in read) the page after load), but this got me wondering why this issue has stalled when Content Moderation is in core, and trash is part of Drupal CMS.

It also decorates the moderation state validator to block any changes to the state while an entity is deleted.
Not had time to write any automated tests for this yet, but it seems to even work well with Workspaces (Core often creates links to blocked forms but I think's that's more to do with issues like #2473873: Entity operations lack cacheability support, resulting in incorrect dropbuttons and #2451693: Operations links on the translations overview page do not link to the correct route).

I like this idea to block state changes while an entity is considered "Deleted". After all, it's practically the same concept of how you can't modify a file in the "trash" on macOS. Changing the workflow-state of an entity is no different than editing any other field or data attached to an entity, and why would you want to do that if the entity is trashed?

It's not done yet, but there's a plan to refactor content_moderation on top of workspaces (which should work better with trash module) here 📌 Allow for / implement simplified content workflow with workspaces Active .

I like this idea to block state changes while an entity is considered "Deleted".

Something like this would need an issue against the contrib trash module - could open one and link from here in case a different approach comes up in this issue.

Something like this would need an issue against the contrib trash module - could open one and link from here in case a different approach comes up in this issue.

Yes, that's essentially the patch posted in this issue. Was just 👍 to the idea.

Oh thanks for some reason I mis-read this as against content_moderation in core.

amateescu → changed the visibility of the branch 3.x to hidden.

amateescu → changed the visibility of the branch 3320566-lawxen to hidden.

Looked a bit into this and it seems like it's actually a core problem: CM sets the load_latest_revision = TRUE param even on the delete route, which I think is wrong.

Undoing that in a core issue might take a while though, so for expedience let's add a quick fix in Trash for now.

Merged into 3.x, thanks everyone for the discussion and patience with this bug :)

amateescu → closed merge request !20