Trash for taxonomy?

Created on 5 December 2024, 9 months ago

Hi guys,

This project is really great and working well but sadly, we discovered that it cannot be used for taxonomies and their terms. Are there any plans to include this functionality into the plugin at all, or is this an impossible task?

Thanks!

✨ Feature request
Status

Active

Version

3.0

Component

Code

Created by

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

Merge Requests

Comments & Activities

  • Issue created by @mambrus
  • šŸ‡·šŸ‡“Romania amateescu

    The problem with taxonomy terms is their hierarchy, i.e. when "trashing" a term, what do you do with its children?

    One option could be to execute \Drupal\taxonomy\Entity\Term::postDelete() directly on the trashed term, with the downside that restoring the deleted term from trash won't restore the hierarchy for its previous child items.

    A possible fix for that could be to store additional "metadata" somewhere, which would be used in this case to store the child item IDs, and on restore you could ask the user if they want to re-parent those terms to the newly restored parent.

    It's not an impossible problem, it just needs a bit of time and thought :)

  • Thank you, @amateescu, I can see that the hierarchy thing is probably a bit complicated :)

    I think the question here also is what should happen with children when the parent is trashed. For me, the logical thing would be to also trash its child nodes (recursively, if they have their own child nodes), so they don't get orphaned, nor added to an arbitrary new parent. If they, however do have another parent, then, just as in the example here, I'd leave them be and only store metadata of the parent that's being trashed. Otherwise, we'd probably need to store metadata of all the trashed children (and possibly their children) as well.

    If trashing works like this, then we wouldn't even need to ask whether to re-parent the restored children once their parent gets restored, since they'd be restored alongside of it. Of, if they had another parent, then they'd get re-attached to the restored parent as well.

    I checked how Drupal's current delete on parent terms work and it seems that children terms are indeed auto-removed with the parent. So that functionality logically doesn't change from the default Drupal behavior.

    What do you think?

  • šŸ‡·šŸ‡“Romania amateescu

    we wouldn't even need to ask whether to re-parent the restored children once their parent gets restored, since they'd be restored alongside of it.

    There's a tricky edge case here brought up by @Fabianx: what if a child term was trashed before its parent. In that case it shouldn't be automatically restored alongside the other children of that parent term. I've been thinking about this problem for quite a while, and I think it could be accomplished by ensuring that we only restore child terms with the deleted timestamp >= than the one of the parent term.

  • šŸ‡¬šŸ‡§United Kingdom khaled.zaidan

    Joining this thread as we're also interested in trash for taxonomy terms.

    I like the idea of the storing this additional metadata. Maybe part of the metadata we have a list of any additional entities trashed as a result of trashing this current entity. Let's call it `trash_chain` or something. We only add direct children in that chain. Any grandchildren would only appear in their direct parents, so handling this trash chain is a recursive bit.

    This way, when we trash an entity (taxonomy term or anything else), any pre-trashed children simply wouldn't be added to the trash chain. And it would also work for any other entity type with hierarchy (e.g. menu items) or any dependency at all. At one point, this might become more flexible to allow custom dependencies.

  • šŸ‡¬šŸ‡§United Kingdom khaled.zaidan

    We might be interested in contributing time and effort to get this metadata bit added.

    I'm thinking an additional serialised column `deleted_metadata`. It gets added/removed together with the `deleted` column.

    And in there we store this `trash_chain` list of entities (entity type + id for each).

    The trash chain could be hard-coded for now just to get these few entity types sorted and to see how well the approach works. We can later transition to using plugins (ideally) or just hooks.

    Maybe we can also allow enabling these entity types as "Experimental" so there's a caveat that they might not work perfectly, yet.

    What do you think @amateescu?

  • First commit to issue fork.
  • Pipeline finished with Failed
    7 months ago
    Total: 268s
    #415697
  • šŸ‡®šŸ‡³India ajits India

    I started adding the support for trash to taxonomy. I have created an MR that enables trashing the terms. However, there is a challenge with the term hierarchy, as mentioned in the previous comments.

    Any term being trashed resets its parent to root by design. See <a href="https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/taxonomy/src/Entity/Term.php?ref_type=heads#L137">Term::preSave</a>.

    I had a chat with @amateescu, and we concluded that any other temporary storage like key/value will not be compatible with the core workspaces module. This data needs to stay with the term itself. Something like @khaled.zaidan mentioned above. I like the idea of saving the immediate child terms for the current term. Maybe in an implementation of hook_pre_trash_delete for taxonomy terms?

  • šŸ‡¬šŸ‡§United Kingdom khaled.zaidan

    That's possible.

    My think is it would be nice if it's not specific to one entity type. I think this can apply to any entity type that there would be dependencies to an entity being trashed.

    So rather than implementing hook_pre_trash_delete for a specific entity type, I was thinking that it's a separate thing (hook or plugin) that allows these dependencies to built, and then we trash those. Maybe that can be done in a hook_pre_trash_delete(), but I think it's safer to do it separately before that hook has been invoked, to ensure our trash chain is always handled first (this might be important if the a child entity being deleted needs information from its parent).

    And maybe `trash_chain` isn't the best name xD I'm thinking maybe `trash_dependent_entities`?

    Here's some suggested pseudo-code to the `TrashStorageTrait`:

    $field_name = 'deleted';
    $revisionable = $this->getEntityType()->isRevisionable();
    
    foreach ($to_trash as $entity) {
      // Get all entity dependent entities.
      $dependent_entities = $this->invokeHook('trash_entity_dependencies', $entity);
    
      // First trash the dependencies. They might require an active parent to be successfully trashed.
      foreach ($dependent_entities as $dependent_entity) {
        $dependent_entity->delete();
      }
    
      // Allow code to run before soft-deleting.
      $this->invokeHook('pre_trash_delete', $entity);
    
      $entity->set($field_name, \Drupal::time()->getRequestTime());
    

    And we would something similar for restoring (just the other opposite order: restore parent first and then the dependencies).

  • šŸ‡·šŸ‡“Romania amateescu

    I liked a lot the idea of only storing the direct children of a term when it's trashed, but it still has the downside that we need an additional field to store them in, and I'd really like to avoid it if possible.

    Thankfully, @eclipsegc had another idea: we could use the deleted timestamp that we already have, and restore child terms with the same timestamp as the parent. Since cascading deletes happen in the same request, all the deleted terms will have the same timestamp, so I think this approach is very feasible.

    As for the trash_dependent_entities suggestion above, I'm not sure we need to generalize the idea of "dependent deletes" just yet, because it would only apply to taxonomy terms at the moment. Menu links for example, which also have a hierarchy, are not auto-deleting their children.

    After looking at the MR posted by Ajit, I think there's something else we need to generalize first: we need to introduce "trash handlers", which would be responsible for all the entity type-specific customizations and hook implementations needed. Opened an issue for that: šŸ“Œ Add the concept of trash handlers Active , and postponing this one for now.

  • Status changed to Postponed 2 months ago
  • Heya guys, any news with regards to this functionality? I see that the underlying task is done and ready - which is amazing! It's be very cool if we could get the taxonomy trash working as well :)

  • šŸ‡®šŸ‡³India ajits India

    The trash handlers issue is now fixed. I plan to give this another shot.

  • Pipeline finished with Failed
    11 days ago
    Total: 227s
    #573165
  • šŸ‡®šŸ‡³India ajits India

    I picked this issue up as part of Tag1's sponsored work for open source development. I used Claude code with Cline in VS Code to work through this.

    We cannot rely on the deleted value for the term in restore as mentioned in #13 above. This is because of the way core handles term deletions.
    Say there is a hierarchy of terms A -> B and the term A is deleted:

    • The Drupal\taxonomy\Entity\Term::preSave is called, and then the term is deleted. And Drupal\taxonomy\Entity\Term::postDelete is called to delete the orphaned terms.
    • From the term A's postDelete, the term B->delete() is called.
    • This calls the Drupal\taxonomy\Entity\Term::preSave before deleting. And the function resets the parent to root. The term gets deleted after that. This removes any trace of the previous hierarchy.

    We might have to rethink a way to store the hierarchy while trashing a term. Maybe we go with storing it with the term itself, as mentioned in previous comments.

    I have added test for possible scenarios. Most of them should pass, except restoring multiple terms in the hierarchy.

    Setting this to "Need review" for now to get quick feedback.

  • Pipeline finished with Failed
    11 days ago
    Total: 237s
    #573186
  • Status changed to Needs review about 12 hours ago
  • šŸ‡ÆšŸ‡“Jordan Rajab Natshah Jordan

    Thank you, Very much needed feature.

  • šŸ‡ÆšŸ‡“Jordan Rajab Natshah Jordan

    Tested, Thank you :)
    The diff from the MR is working well

  • šŸ‡·šŸ‡“Romania amateescu

    The MR is still a work in progress, see #18.

  • šŸ‡ÆšŸ‡“Jordan Rajab Natshah Jordan

    Noted; you are right Andrei

    Having more testing rounds, with number of scenarios

    I would love to follow with Drupal Core Behavior when deleting terms

    Given that we have a News Categories vocabulary
    Given that we have a News Categories vocabulary
    And having "Tech root level" as a root term
    And "Tech 1 level 1", "Tech 2 level 1", "Tech 3 level 1" under the "Tech root level" term
    And "Tech 1 level 2", "Tech 1 level 2", "Tech 1 level 2" under the "Tech 1 level 1" term
    And "Tech 2 level 2", "Tech 2 level 2", "Tech 2 level 2" under the "Tech 2 level 1" term
    And "Tech 3 level 2", "Tech 3 level 2", "Tech 3 level 2" under the "Tech 3 level 1" term
    And "Tech 1 level 3", "Tech 1 level 3", "Tech 1 level 3" under each "Tech 1 level 2" term
    And "Tech 2 level 3", "Tech 2 level 3", "Tech 2 level 3" under each "Tech 2 level 2" term
    And "Tech 3 level 3", "Tech 3 level 3", "Tech 3 level 3" under each "Tech 3 level 2" term

    And having "Sport root level" as a root term
    And "Sport 1 level 1", "Sport 2 level 1", "Sport 3 level 1" under the "Sport root level" term
    And "Sport 1 level 2", "Sport 1 level 2", "Sport 1 level 2" under the "Sport 1 level 1" term
    And "Sport 2 level 2", "Sport 2 level 2", "Sport 2 level 2" under the "Sport 2 level 1" term
    And "Sport 3 level 2", "Sport 3 level 2", "Sport 3 level 2" under the "Sport 3 level 1" term
    And "Sport 1 level 3", "Sport 1 level 3", "Sport 1 level 3" under each "Sport 1 level 2" term
    And "Sport 2 level 3", "Sport 2 level 3", "Sport 2 level 3" under each "Sport 2 level 2" term
    And "Sport 3 level 3", "Sport 3 level 3", "Sport 3 level 3" under each "Sport 3 level 2" term

    Tech root level
    ā”œā”€ā”€ Tech 1 level 1
    │   ā”œā”€ā”€ Tech 1 level 2
    │   │   ā”œā”€ā”€ Tech 1 level 3
    │   │   ā”œā”€ā”€ Tech 1 level 3
    │   │   └── Tech 1 level 3
    │   ā”œā”€ā”€ Tech 1 level 2
    │   │   ā”œā”€ā”€ Tech 1 level 3
    │   │   ā”œā”€ā”€ Tech 1 level 3
    │   │   └── Tech 1 level 3
    │   └── Tech 1 level 2
    │       ā”œā”€ā”€ Tech 1 level 3
    │       ā”œā”€ā”€ Tech 1 level 3
    │       └── Tech 1 level 3
    ā”œā”€ā”€ Tech 2 level 1
    │   ā”œā”€ā”€ Tech 2 level 2
    │   │   ā”œā”€ā”€ Tech 2 level 3
    │   │   ā”œā”€ā”€ Tech 2 level 3
    │   │   └── Tech 2 level 3
    │   ā”œā”€ā”€ Tech 2 level 2
    │   │   ā”œā”€ā”€ Tech 2 level 3
    │   │   ā”œā”€ā”€ Tech 2 level 3
    │   │   └── Tech 2 level 3
    │   └── Tech 2 level 2
    │       ā”œā”€ā”€ Tech 2 level 3
    │       ā”œā”€ā”€ Tech 2 level 3
    │       └── Tech 2 level 3
    └── Tech 3 level 1
        ā”œā”€ā”€ Tech 3 level 2
        │   ā”œā”€ā”€ Tech 3 level 3
        │   ā”œā”€ā”€ Tech 3 level 3
        │   └── Tech 3 level 3
        ā”œā”€ā”€ Tech 3 level 2
        │   ā”œā”€ā”€ Tech 3 level 3
        │   ā”œā”€ā”€ Tech 3 level 3
        │   └── Tech 3 level 3
        └── Tech 3 level 2
            ā”œā”€ā”€ Tech 3 level 3
            ā”œā”€ā”€ Tech 3 level 3
            └── Tech 3 level 3
    
    
    Sport root level
    ā”œā”€ā”€ Sport 1 level 1
    │   ā”œā”€ā”€ Sport 1 level 2
    │   │   ā”œā”€ā”€ Sport 1 level 3
    │   │   ā”œā”€ā”€ Sport 1 level 3
    │   │   └── Sport 1 level 3
    │   ā”œā”€ā”€ Sport 1 level 2
    │   │   ā”œā”€ā”€ Sport 1 level 3
    │   │   ā”œā”€ā”€ Sport 1 level 3
    │   │   └── Sport 1 level 3
    │   └── Sport 1 level 2
    │       ā”œā”€ā”€ Sport 1 level 3
    │       ā”œā”€ā”€ Sport 1 level 3
    │       └── Sport 1 level 3
    ā”œā”€ā”€ Sport 2 level 1
    │   ā”œā”€ā”€ Sport 2 level 2
    │   │   ā”œā”€ā”€ Sport 2 level 3
    │   │   ā”œā”€ā”€ Sport 2 level 3
    │   │   └── Sport 2 level 3
    │   ā”œā”€ā”€ Sport 2 level 2
    │   │   ā”œā”€ā”€ Sport 2 level 3
    │   │   ā”œā”€ā”€ Sport 2 level 3
    │   │   └── Sport 2 level 3
    │   └── Sport 2 level 2
    │       ā”œā”€ā”€ Sport 2 level 3
    │       ā”œā”€ā”€ Sport 2 level 3
    │       └── Sport 2 level 3
    └── Sport 3 level 1
        ā”œā”€ā”€ Sport 3 level 2
        │   ā”œā”€ā”€ Sport 3 level 3
        │   ā”œā”€ā”€ Sport 3 level 3
        │   └── Sport 3 level 3
        ā”œā”€ā”€ Sport 3 level 2
        │   ā”œā”€ā”€ Sport 3 level 3
        │   ā”œā”€ā”€ Sport 3 level 3
        │   └── Sport 3 level 3
        └── Sport 3 level 2
            ā”œā”€ā”€ Sport 3 level 3
            ā”œā”€ā”€ Sport 3 level 3
            └── Sport 3 level 3
    
    

    What are the accountable scenarios?

    Deletion / Restore scenarios for root terms

    When I delete "Tech root level"
    Then all its child terms will be deleted.

    When I restore "Tech root level"
    Then all its child terms will be restored.

    Deletion / Restore scenarios for 1st-level child terms

    When I delete "Tech 1 level 1"
    Then all its children ("Tech 1 level 2" and their descendants) will be deleted.

    When I restore "Tech 1 level 1"
    Then all its children ("Tech 1 level 2" and their descendants) will be restored.

    Deletion / Restore scenarios for 2nd-level child terms

    When I delete "Tech 1 level 2"
    Then all its children ("Tech 1 level 3") will be deleted.

    When I restore "Tech 1 level 2"
    Then all its children ("Tech 1 level 3") will be restored.

    Deletion / Restore scenarios for leaf terms

    When I delete "Tech 1 level 3"
    Then only this term will be deleted (no children under it).

    When I restore "Tech 1 level 3"
    Then only this term will be restored.

    Cross-vocabulary isolation

    When I delete "Tech root level"
    Then no terms under "Sport root level" are affected.

    When I delete "Sport root level"
    Then no terms under "Tech root level" are affected.

    Partial deletion within multiple branches

    When I delete "Tech 2 level 1"
    Then only its subtree ("Tech 2 level 2" and "Tech 2 level 3") will be deleted.

    When I restore "Tech 2 level 1"
    Then its subtree ("Tech 2 level 2" and "Tech 2 level 3") will be restored.

    Mixed operations

    When I delete "Tech 1 level 2" and "Tech 3 level 1"
    Then only those branches and their children are deleted, while other Tech terms remain.

    When I restore "Tech 1 level 2" and "Tech 3 level 1"
    Then only those branches and their children are restored.

  • šŸ‡ÆšŸ‡“Jordan Rajab Natshah Jordan

    The tree could be serialized and stored as an extra nested data relation, which restore could read it and apply a restore for it after the root restore

Production build 0.71.5 2024