Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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:
- Create "Term 1" with no parent
- Create "Term 2" with no parent
- Create "Term 3" with parents "Term 1" AND "Term 2"
- 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.
Comments
Comment #1
guschilds CreditAttribution: guschilds commentedAttached 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.
Comment #2
Yorgg CreditAttribution: Yorgg commentedThis patch fixed the issue in my test.
Comment #3
BerdirThis will need a test.
Comment #4
sidharthapI tested it and the patch #1 works perfect.
Comment #5
wadmiraal CreditAttribution: wadmiraal commentedWriting a test for this.
Comment #6
wadmiraal CreditAttribution: wadmiraal commentedAttached is a unit test for this. It fails before applying the patch from #1, and passes after applying it.
Note: this one should fail.
Comment #7
wadmiraal CreditAttribution: wadmiraal commentedThis patch includes both the patch from #1 and #6.
Note: This patch should pass.
Comment #8
wadmiraal CreditAttribution: wadmiraal commentedIGNORE THIS ONE - chose the wrong file, my bad.
Comment #9
wadmiraal CreditAttribution: wadmiraal commentedSame patch as in #7, but remove some left over whitespace.
Comment #10
valthebaldTest 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
Comment #11
YesCT CreditAttribution: YesCT commented"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?
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
Comment #12
wadmiraal CreditAttribution: wadmiraal commentedInterdiff correcting the test block comment.
Comment #13
wadmiraal CreditAttribution: wadmiraal commentedComment #14
wadmiraal CreditAttribution: wadmiraal commentedForgot the 2nd point, sorry.
Comment #15
wadmiraal CreditAttribution: wadmiraal commentedCorrected the 2nd point as well.
Comment #16
wadmiraal CreditAttribution: wadmiraal commentedOk, 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).
Comment #17
wadmiraal CreditAttribution: wadmiraal commentedComment #18
guschilds CreditAttribution: guschilds commentedThank you so much for creating this test!
Comment #19
valthebaldPatch looks fine for me, and I'd like to see that patch ported to D7 too (lower priority I suppose)
Comment #20
guschilds CreditAttribution: guschilds commentedThanks 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.Comment #21
catchCommitted/pushed to 8.x, thanks!