Translation support

Created on 21 July 2023, over 1 year ago

Problem/Motivation

When a translation of a trashable entity is deleted, Trash module does not intervene and just lets it be removed, even though the confirmation dialog says the entity will not actually be deleted.

Steps to reproduce

  1. Enable Trash support for Nodes.
  2. Enable Content Translation and add one or more extra languages.
  3. Create a node
  4. Add one or more translations
  5. Delete a translation. The confirm dialog says it will be trashed, but it actually gets deleted.
  6. Delete the entity via the source language. All existing translations get trashed

Proposed resolution

Add asymmetric trash support for translations by making the 'deleted' field translatable. When an entity is saved, check if a translation was deleted. If so, restore the translation and set the 'deleted' value on that translation.
Trash already depends on the storage backend being SqlContentEntityStorage compatible, which should also mean it has translation support, so assuming 'deleted' can always be set as translatable should be safe?

Remaining tasks

Test with untranslatable entities. Possibly adapt the code so it does not enable translation unless the entity is actually translatable?
Create assymetric vs symmetric translation toggle?

User interface changes

The Trash list now includes a "Language" column and includes only the trashed translations of an entity.
(Multi-language listings aren't well supported by entity queries so we'll have to do with them being bunched together per entity - but that could also be desirable in some cases.)

API changes

None. This "just works" based on the actual translation sent as the $entity argument to existing functions/methods.

Data model changes

The 'deleted' field is now translatable, which does not directly alter it for Nodes or other content entities, but there may be cases where the field moves from one table to another.

✨ Feature request
Status

Active

Version

3.0

Component

Code

Created by

πŸ‡ΈπŸ‡ͺSweden twod Sweden

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @twod
  • Merge request !9Issue #3376216: Translation support β†’ (Open) created by twod
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    6 pass
  • Status changed to Needs review over 1 year ago
  • πŸ‡ΈπŸ‡ͺSweden twod Sweden
  • πŸ‡ΈπŸ‡ͺSweden twod Sweden
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    6 pass
  • Status changed to Needs work over 1 year ago
  • πŸ‡ΈπŸ‡ͺ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.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    4 pass, 1 fail
  • Status changed to Needs review about 1 year ago
  • πŸ‡ΈπŸ‡ͺ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).

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    last update about 1 year ago
    6 pass
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 8
    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.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.4 + Environment: PHP 8.1 & MySQL 8
    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
  • πŸ‡·πŸ‡΄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 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.

  • πŸ‡ΈπŸ‡ͺ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.

  • Pipeline finished with Success
    about 1 year ago
    #59691
  • Pipeline finished with Success
    about 1 year ago
    Total: 204s
    #61041
  • Pipeline finished with Success
    about 1 year ago
    Total: 206s
    #61099
  • Pipeline finished with Failed
    about 1 year ago
    Total: 228s
    #61292
  • Pipeline finished with Success
    about 1 year ago
    Total: 235s
    #61600
  • Pipeline finished with Success
    about 1 year ago
    Total: 212s
    #61603
  • Pipeline finished with Failed
    about 1 year ago
    Total: 283s
    #61867
  • Status changed to Needs review about 1 year ago
  • πŸ‡ΈπŸ‡ͺ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.

  • Pipeline finished with Canceled
    about 1 year ago
    Total: 89s
    #61992
  • Pipeline finished with Failed
    about 1 year ago
    Total: 217s
    #61993
  • Pipeline finished with Failed
    about 1 year ago
    Total: 251s
    #62538
  • Pipeline finished with Failed
    about 1 year ago
    Total: 213s
    #62557
  • Pipeline finished with Failed
    about 1 year ago
    Total: 244s
    #62560
  • Pipeline finished with Failed
    about 1 year ago
    Total: 273s
    #62599
  • Pipeline finished with Success
    about 1 year ago
    Total: 230s
    #62615
  • Pipeline finished with Success
    about 1 year ago
    #62628
  • Pipeline finished with Success
    about 1 year ago
    Total: 218s
    #62652
  • Pipeline finished with Success
    about 1 year ago
    Total: 217s
    #62663
  • Pipeline finished with Success
    about 1 year ago
    Total: 258s
    #62846
  • Pipeline finished with Success
    about 1 year ago
    Total: 277s
    #62847
  • πŸ‡ΈπŸ‡ͺ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

  • Pipeline finished with Success
    10 months ago
    Total: 218s
    #99414
  • Pipeline finished with Success
    10 months ago
    Total: 338s
    #99568
  • Pipeline finished with Success
    10 months ago
    Total: 284s
    #99654
  • Pipeline finished with Failed
    10 months ago
    Total: 186s
    #99748
  • Pipeline finished with Success
    3 months ago
    Total: 298s
    #292499
  • Pipeline finished with Success
    3 months ago
    Total: 369s
    #292569
  • Pipeline finished with Success
    3 months ago
    Total: 252s
    #293140
  • Pipeline finished with Success
    3 months ago
    Total: 226s
    #293158
  • Pipeline finished with Success
    3 months ago
    Total: 223s
    #293160
  • Pipeline finished with Success
    3 months ago
    Total: 230s
    #293163
  • Pipeline finished with Failed
    about 2 months ago
    Total: 224s
    #323865
  • πŸ‡·πŸ‡΄Romania amateescu

    We've added a temporary safeguard for deleting translations in πŸ› Prevent deleting translations Needs work , and updated this MR to remove it :)

Production build 0.71.5 2024