Problem/Motivation

When one taxonomy term has two parents and one of those parents is deleted, the term is also deleted even though it still has another parent.

This bug was originally reported in #14 of #726490: Taxonomy term record for term with multiple parents is not deleted when parent is deleted.

To reproduce:

  1. Create "Term 1" with no parent
  2. Create "Term 2" with no parent
  3. Create "Term 3" with parents "Term 1" AND "Term 2"
  4. Delete "Term 2"

Expected result: "Term 3" survives with a parent of "Term 1".
Actual result: "Term 3" was deleted with the deletion of "Term 2". Only "Term 1" remains.

Proposed resolution

Will be attaching a patch in comment #1 that fixes the bug.

Remaining tasks

Reviewing, testing, and committing the patch or an alternate solution.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guschilds’s picture

Status: Active » Needs review
FileSize
802 bytes

Attached is a patch that fixes the bug described above. Following the steps to reproduce above should now produce the expected result.

This bug stands in the way of reproducing #726490: Taxonomy term record for term with multiple parents is not deleted when parent is deleted, so that issue should be addressed after this one.

Yorgg’s picture

This patch fixed the issue in my test.

Berdir’s picture

Issue tags: +Needs tests

This will need a test.

sidharthap’s picture

I tested it and the patch #1 works perfect.

wadmiraal’s picture

Status: Needs review » Active

Writing a test for this.

wadmiraal’s picture

Attached is a unit test for this. It fails before applying the patch from #1, and passes after applying it.
Note: this one should fail.

wadmiraal’s picture

This patch includes both the patch from #1 and #6.
Note: This patch should pass.

wadmiraal’s picture

IGNORE THIS ONE - chose the wrong file, my bad.

wadmiraal’s picture

Same patch as in #7, but remove some left over whitespace.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

Test from #6 fails like expected, and #9 is essentially the same patch as #7, with extra space removed.
This one looks like safe to RTBC

YesCT’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermUnitTest.php
    @@ -33,6 +33,28 @@ function testTermDelete() {
       /**
    +   * Test deleting a parent of a term with multiple parents does not delete
    +   * the child term.
    +   */
    

    "All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc."

    https://drupal.org/node/1354#functions

    and technically, it's a core-gates blocker.
    but are we really that strict with tests? I've run into this a few times recently. Should we mark needs work for things like this even if they are in tests?

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermUnitTest.php
    @@ -33,6 +33,28 @@ function testTermDelete() {
    +    $this->assertTrue(!empty($child_term), 'Child term is not deleted if only one of its parents is removed');
    ...
    +    $this->assertTrue(empty($child_term), 'Child term is deleted if all of its parents are removed');
    

    this next thing is a total nit and not a blocker.

    other asserts with sentences, use a period at the end.

    Since this needs work anyway, we might as well change that.

ohhh and we love interdiffs. :) here are some instructions for making an interdiff. https://drupal.org/documentation/git/interdiff

wadmiraal’s picture

FileSize
652 bytes

Interdiff correcting the test block comment.

wadmiraal’s picture

Status: Needs work » Needs review
wadmiraal’s picture

FileSize
1.33 KB

Forgot the 2nd point, sorry.

wadmiraal’s picture

FileSize
1.33 KB

Corrected the 2nd point as well.

wadmiraal’s picture

Ok, sorry, I didn't get (duh) that I had to make a new patch.
Patch attached, same as #9 with interdiff correction from #14 (and #15 - guess I have some caching issues).

wadmiraal’s picture

guschilds’s picture

Thank you so much for creating this test!

valthebald’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs backport to D7

Patch looks fine for me, and I'd like to see that patch ported to D7 too (lower priority I suppose)

guschilds’s picture

Issue tags: -Needs backport to D7

Thanks for reviewing. I just tested for this bug multiple times in D7 and it is not reproducible. It seems taxonomy_term_delete() in taxonomy.module properly handles the removal of orphans. Just to be sure, I then tested D8 again and the bug still exists. Looks like this should be good to go.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.