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.

Files: 
CommentFileSizeAuthor
#17 taxonomy-multiple-parent-orphan-deletion-unit-test-with-patch-from-1-2077387-17.patch2.17 KBwadmiraal
PASSED: [[SimpleTest]]: [MySQL] 58,660 pass(es).
[ View ]
#17 interdiff-2077387-9-14.txt1.33 KBwadmiraal
#16 taxonomy-multiple-parent-orphan-deletion-unit-test-with-patch-from-1-2077387-16.patch2.17 KBwadmiraal
FAILED: [[SimpleTest]]: [MySQL] 58,580 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#15 interdiff-2077387-9-14.txt1.33 KBwadmiraal
#14 interdiff-2077387-9-14.txt1.33 KBwadmiraal
#12 interdiff-2077387-9-12.txt652 byteswadmiraal
#6 taxonomy-multiple-parent-orphan-deletion-unit-test-2077387-6.patch1.4 KBwadmiraal
FAILED: [[SimpleTest]]: [MySQL] 58,580 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#9 taxonomy-multiple-parent-orphan-deletion-unit-test-with-patch-from-1-2077387-9.patch2.19 KBwadmiraal
PASSED: [[SimpleTest]]: [MySQL] 58,570 pass(es).
[ View ]
#8 taxonomy-multiple-parent-orphan-deletion-unit-test-with-patch-from-1-2077387-7.patch2.19 KBwadmiraal
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]
#7 taxonomy-multiple-parent-orphan-deletion-unit-test-with-patch-from-1-2077387-7.patch2.19 KBwadmiraal
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]
#1 taxonomy-multiple-parent-orphan-deletion-2077387-1.patch802 bytesguschilds
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new802 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,436 pass(es).
[ View ]

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.

This patch fixed the issue in my test.

Issue tags:+Needs tests

This will need a test.

I tested it and the patch #1 works perfect.

Status:Needs review» Active

Writing a test for this.

StatusFileSize
new1.4 KB
FAILED: [[SimpleTest]]: [MySQL] 58,580 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Active» Needs review
StatusFileSize
new2.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,444 pass(es).
[ View ]

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

StatusFileSize
new2.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]

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

StatusFileSize
new2.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,570 pass(es).
[ View ]

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

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

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

StatusFileSize
new652 bytes

Interdiff correcting the test block comment.

Status:Needs work» Needs review

StatusFileSize
new1.33 KB

Forgot the 2nd point, sorry.

StatusFileSize
new1.33 KB

Corrected the 2nd point as well.

StatusFileSize
new2.17 KB
FAILED: [[SimpleTest]]: [MySQL] 58,580 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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).

StatusFileSize
new1.33 KB
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,660 pass(es).
[ View ]

Thank you so much for creating this test!

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)

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.

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.