Translation support

Just discovered I forgot a few calls so it restores the original translation instead of the removed one.
Will see if I can adapt the tests to catch that as well.

These changes improves the GUI somewhat and better track what happens when you normally delete an entity.
Translations can now be independently deleted, restored or purged without affecting the source language from the language overview page.
Deleting or purging a source language also affects all translations.
Restoring a source language does not automatically restore all translations, but that is easily done from the entity's translation overview page.

The new revisions when deleting are enforced to be the default revision, or you could end up with a translation that could not be edited because the latest (draft) revision was deleted but the default revision was still published.
Content Moderation may have "opinions" on when this is allowed, but since we have the storage trait we can ensure Trash gets the last word. Maybe we should even set the "is syncing" flag for these operations?

One side effect of that is that restoring something where the latest revision was a draft (unpublished) and the default revision was published, will not actually be published after a restore - as the default revision is now the unpublished draft (without the deleted timestamp). You would have to fix that either by publishing the draft or by restoring a previous revision as the default manually.

Toyed with the idea of removing the latest revision completely instead of just clearing the deleted timestamp (for revisionable entities), but ran into complications because you can't delete a default revision and it made me question if it was reasonable to always set the revision before the deleted revision as the new default before removing the "deleted" revision.

This could use some functional tests for the GUI changes in the translation overview and the trash listing itself, but I figured it'd be best to get some feedback first before spending that time in case big changes are requested (or this is rejected completely).

@EZKG, it should already be handing translations in that way, and there are tests included to verify that, but I can take another look soon.

There have been multiple changes since my version and the tests no longer pass so I'll need some time to dig into what's going on.

Restoring a source language does not automatically restore all translations, but that is easily done from the entity's translation overview page.

We now have TrashStorageTrait::restoreFromTrash() (which wasn't available when this MR was created), can we restore all translations there? Or was there another reason for not doing it in the first place?

My reasoning at the time was that we don't easily know when a translation was deleted. If we force all of them to be restored it may unexpectedly restore some that were deleted much earlier, but I suppose the grouping in the trashcan should make that fairly easy to spot, unless you have a lot of languages. Either way, if the confirmation message explicitly lists the translations to be restored I would not be worried about that happening.

However, I would prefer if we then also had to explicitly pass in the translations to be restored to TrashStorageTrait::restoreFromTrash, and that it only saved the entity once even if multiple translations were passed in.

That it now explicitly saves the entities introduced some problems for this issues as it more or less broke the API.
The tests are calling trash_restore_entity($translation) multiple times before saving an entity, and that is no longer possible without creating multiple pointless revisions.
Granted, it was merged before 3.0 so I can forgive that, but it's a twist which means this will need some additional changes.

I'll see if I can implement that.

Didn't implement restoring all translations at once yet, just wanted to get it back to passing the tests while awaiting feedback on the rest of my comments. At least I think it's working the way it did, a bit late here so didn't do much manual testing yet.

Maybe we can make restoring all translations at once optional? I mean when it's done via the trashcan, as it I don't think it make much sense when doing it via the translation overview.

I think this needs another look from someone other than me at this point.
As noted above, I pulled out all remaining revision/content_moderation related code. A bit late I noticed I had also pulled out the lines which set a new revision, but everything was still working as it should - and as @amateescu mentioned, this module should probably not enforce it unless strictly necessary anyway.

I got tired of restoring translations one by one, so I implemented @amateescu's suggstion from a while back and made it so restoring the original language by default also restores all translations - but they can be manually excluded if you don't want that.

Restoring an individual translation now also asks if you would like to restore other translations as well, but then the default is not to do it.

I've got nothing more planned for this right now so just awaiting feedback. :)

I've done a fairly big refactor of the patch to accommodate the feedback by @plach and @amateescu here and via chats.

One major change due to this:
Trying to handle deleting and restoring pending revisions (drafts) was turning into a problem with more and more edge cases.
So, instead of trying to figure out which revision is actually the latest one (via getActive() or otherwise), the patch now uses getCanonical() per translation.
It also also creates a new revision [if the entity is revisionable] on delete, restore, or save with a removed translation.

This has the effect that any pending revisions (drafts) are effectively ignored and repeated trash/restore operations will always act on the published/canonical/default revision per language, keeping what was published and "skipping" drafts.
You can still revert to previously pending revisions from the revisions overview if you wish, but because Content Moderation module does not have a "revert as draft" operation, it will always publish them as the new default revision. Another module could add such an operation if desired.

After a while, this felt more natural to work with than drafts becoming soft-deleted. Content Moderation doesn't allow you to delete drafts from the translation overview, but if you delete a revision from the revisions overview - it's a separate operation which Trash ignores - so they will always be permanently deleted.

I've added several more tests, and refactored some of the existing ones to be a bit more reussable for verifying that base operations still work the same with the steps of enabling only language, then content_translation,content_moderation, and to some degree workspaces.

During this testing we found that a recent change to Workspaces in 11.2 hinders creating drafts in the Live workspace, which limits the amount of testing which can be done without patching core to revert the change from #3499181: Disallow saving the current default revision as a non-default revision - or having Trash work around it.
I did not include such a workaround in this branch because I did not fully understand why that change was introduced and what consequences it may have for Workspaces.

The branch is becoming increasingly large due to the many things it has to cover, including compensating for in-memory caches used by some of the base classes for the "latest revision" - which may be different depending on the current Trash context.
I hope the new tests somewhat compensate for this as the coverage is pretty good.
(Depending on how #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() is resolved, we may need to compensate for that too.)

I apologize for the messy commits, I accidentally did much of the work without realizing I was in the middle of one of several times I had to pull in 3.x again. Love to see the progress, but you did throw me for a few loops. ;)

Btw, I did the changes on D11 and forgot not all of it works for D10 yet, planning to fix that soon. Done.

I would say it's mainly testing that's needed. We already know of a few D11 issues that affect translation handling, and touch interaction with this module, I think #3546214: Can not create a new draft translation in a workspace is the main one. It doesn't directly break this feature, but makes it confusing to work with.

As we just got the cron job and Drush command taken care of I don't think there's anything significant left to code (except maybe some cleanup). Btw, make sure you have updated to a patch which includes that if you do use those, or you may suffer a whole entity being purged instead of just its translation!

At DrupalCon Vienna @amateescu and I discussed possibly splitting this issue up into smaller chunks to make it easier to review and commit. Maybe it could be done by first refactoring to use the general structure from the patch, along with the new cache swap, etc. Then we layer actual translation handling on top, but I'm not yet sure how difficult that would be.

We also briefly looked at (separate from this issue) getting in a new way to handle viewing and manipulating soft-deleted entities via separate routes, instead of trying to deal with passing around the 'in-trash' query parameter. That could affect this implementation slightly.
So, instead of /node/123/?in-trash=1 you would go to /trash/node/123/[view/restore/purge]. We though that may simplify some access logic and the URL rewriting, as well as allowing stricter access control on those routes , and reducing the risk of interfering with other modules's access checks.

These things would then most likely become part of a new major release.

I am currently looking to get more time to work on this module through my job. My team actually decided today to propose that as part of my tasks before the end of the year, but that ship turns a bit slow so I don't have go on that yet. (This would be a good time for me to do that though.)

A huge thanks to the people jumping in and keeping this branch fresh!

I added some more comments to explain the reasoning behind some of it in the PR, but I don't remember if they were already there when you looked at it. Let me know if you want more details on something specific. It's easier for me to get time to document it better than to begin splitting it apart right away. It may also be best to be on the same page about how it works before deciding exactly how to split it.
I'm writing this a bit tired just before going to bed, so there may be caveats, but I hope it's helpful enough for now. 😅

Unfortunately, there is no way to fully guarantee that we do implicitly operate on the correct revisions or translation in the trait's methods as that is often up to the caller. We don't want to "fork" the revision tree, and that should at least not happen via any of the forms in Core, thanks to the new access checks.

The latest version does use RevisionableStorageInterface::createRevision() where possible, except last in ::doPreSave() due to that method always cloning the entity. doPreSave() is expecting the entity to be modified by reference and then passed on to ::doSave(). Given we may create multiple new revisions in doPreSave() - something it really wasn't intended to do - we have to tread carefully to make sure we use the correct revision as the source for each language, and that the passed in active translation is handled last and its reference is preserved. Core has already checked that someone isn't trying to delete the default translation and thrown an exception, so that case we don't have to consider. But someone could have called $entity->save() with either the default translation or any other still existing translation active, so we can't make assumptions about that.

Since we check if the entity has been modified in any way (other than deleting translations), it is then safe for the last part to do the bare minimum to force the parent method to create a new revision for the 'active' translation.

If we do send the default revision for each translation into ::createRevision() (as we should), it technically does not do anything more than set the new revision to be the default, tag it as affecting the passed in language, and invoke the entity_revision_create hooks.
(Those hooks will not be invoked there for the initially 'active' language that $entity->save() was called for, but the preparation of the entity form has already called them for that language before the entity was saved.)

There is a bit of a gray area if something tries to delete a translation from an older revision. I don't think I have actually tested that. If everything else in Drupal actually allows that, there may be hidden dragons. I suppose we could say that behavior is undefined, and in that case you're on your own and should not only deactivate the trash context, but also ensure the other revisions still make logical sense.

The "simple" explanation for that is ContentEntityBase::removeTranslation(). It removes a translation, marks it as removed, and then does nothing else. Hence, we have to wait until $entity->save() is called until we can detect that, revert the translation, tag it as 'deleted' (possibly creating more revisions if more than one translation was removed) and finally let it save.

@amateescu That comes from wanting one revision per deleted translation - as we may not restore/purge all of them at once - and it was easier to deal with them that way since otherwise you would not get the 'deleted' flag set on all translations, leading to inconsistencies.

We basically are undoing what that method does, except we can't rely on it being that form which deleted the translation. Some other code may look like this without causing any errors.

$entity->removeTranslation('sv');
$entity->removeTranslation('de');
$entity->save();

(Things would have been so much easier if addTranslation() and removeTranslation() were "atomic" and saved the entity themselves.)

I'm surprised tests passed after moving the code out of TrashStorageTrait::doPreSave(). I was expecting $entity->original = $this->loadUnchanged($id); from EntityStorageBase::doPreSave() to return NULL in some cases, as the Trash context is still active when that runs, before the hooks are invoked, but maybe it will work in some cases. Or, I missed adding a test case which covered that problem. Will have to investigate and see if I can reproduce that... 😅

Thanks for continuing this! I've not yet had time to look at breaking this apart more. I am however dealing with a D11 upgrade in which we need to port this. More about that below.

I'm not sure we need to create a revision per translation when calling $entity->delete() - still thinking about the implications for this.

Without those revisions we would get very strange effects when trying to restore only the source language (leaving translations) or the source language + only some translations, or when purging just some translations while leaving the others in soft-deleted.

There's too much responsibility placed in the access handler, I think some situations would be better handled by throwing exceptions rather than access restrictions.

I'm not sure where those exceptions would be thrown. The access handler does most of the sanity checks now because it has the context needed to do so without further complicating the storage trait. The access handler also prevents the GUI from showing disallowed operations. If it does not perform those checks and we instead throw an exception elsewhere, those operations would look possible - but the user would instead be greeted by a WSOD. These checks also apply to things like JSON:API and the broken entity links it would otherwise return. The code does throw exceptions for the few cases where permission checks may be bypassed and it would try to perform an unrecoverable illogical operation.

For D11 I noticed the cache interception here no longer works. Thankfully it is because they've moved to a separate memory cache implementation, which can be more cleanly intercepted and abstracted. I'm testing a solution for that which I can post soon, but I'm not yet up to speed with this branch so have some merging to do.
Edit: I see this was dealt with in #3561099: Trash doesn't handle the static entity cache properly when switching contexts, I'll compare that to the solution I did, which took a bit of a different approach and created separate cache entries for each context [for the trash-enabled entity types], trying to avoid invalidating them every time the context changes (which can be very often).

LOL, it's always a race keeping up with you. Thanks! :D

I saw your note about making the trait smaller. One nice thing about keeping most of the logic concentrated there was that it may be easier to override trash handling per entity type with a custom storage class using it and overriding just what needed to be changed, but I get the point about preferably not having to generate classes at all.

Btw, I just hit a problem where updating our nodes generated redirects and redirects are not trashable, but its hooks are still active so the redirect_presave implementation crashed on trying to get a deleted field which does not exist.
The check if (!$this->trashManager->isEntityTypeEnabled('redirect')) {return;}, doesn't work since redirects aren't enabled and the service was never configured. If we instead initialize the dependencies to NULL in the base class we can do if (!$this->trashManager?->isEntityTypeEnabled('redirect')) {return;} and it still works. I suspect something similar would happen if nodes weren't trashable and query_search_node_search_alter is called but haven't verified. I haven't seen an issue for it yet, but maybe you're already aware?

Oh, I see it also removed the un-setting of the disabled handlers, yeah, that should fix it.

Hmm, I have not tested that scenario in a long time. I don't know how it would work with some of the other recent changes.
There are so many combinations to test I'm too tired to think about them all right now, especially witih content_moderation enabled and having drafts in various states. Maybe it works better now that we don't try to mark the draft revision itself as deleted and instead always create a new default revision for each change. That did simplify some things and made some of the flows less confusing.
(Draft revisions thus aren't touched at all and just end up as historical never published revisions.)

It could be worth a try.