- Issue created by @twod
- last update
over 1 year ago 6 pass - Status changed to Needs review
over 1 year ago 6:24pm 21 July 2023 - last update
over 1 year ago 6 pass - Status changed to Needs work
over 1 year ago 9:32am 18 September 2023 - πΈπͺSweden twod Sweden
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. - last update
about 1 year ago 4 pass, 1 fail - Status changed to Needs review
about 1 year ago 12:22am 27 September 2023 - πΈπͺSweden twod Sweden
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).
- last update
about 1 year ago 6 pass - last update
about 1 year ago 6 pass This MR needs to be added to handle the trasnlations deletion. As it stands the trasnlated pagges are being permanently deleted even if it states that it will be available in the trash. This occurs sppacialy with a content that has 2 or more trasnlation.
- First commit to issue fork.
- last update
about 1 year ago 4 pass, 4 fail - πΈπͺSweden twod Sweden
@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.
- Status changed to Needs work
about 1 year ago 7:18pm 21 November 2023 - π·π΄Romania amateescu
@TwoD, thanks a lot for working on this! This functionality is very much needed, so it's definitely not going to be rejected :)
I agree with @penyaskito above, the MR looks mostly ok to me as well. The biggest concern I have is the need for creating default revisions when deleting an entity (or a translation), because that would completely break the Workspaces use-case which only creates default revisions when a workspace is published to Live.
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? - πΈπͺSweden twod Sweden
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 callingtrash_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.
- πΈπͺSweden twod Sweden
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.
- Status changed to Needs review
about 1 year ago 2:18am 11 December 2023 - πΈπͺSweden twod Sweden
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. - πΈπͺSweden twod Sweden
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. :)
- π·π΄Romania amateescu
Just merged the latest 3.x, and I'll try to review this again ASAP :)
- π¨π³China lawxen
I'm wondering whether the MR can solve this issue: https://www.drupal.org/project/trash/issues/3414901 π Delete an node with revisons, the node is not in trash, but can't be visited Active
Based on the comment here: https://www.drupal.org/project/trash/issues/3320566#comment-15356737 β¨ Trash is not compatible with Content Moderation Active - π·π΄Romania amateescu
We've added a temporary safeguard for deleting translations in π Prevent deleting translations Needs work , and updated this MR to remove it :)