Problem/Motivation
When a taxonomy term has two parents and one of them is deleted, the reference from the child term to the now-deleted parent term still exists in taxonomy_term_hierarchy table. For example (from #20):
To test:
- 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"
After Step 3, the taxonomy_term_hierarchy table will look like so (using theoretical tids that correspond to their names):
tid | parent |
---|---|
1 | 0 |
2 | 0 |
3 | 1 |
3 | 2 |
After Step 4, the taxonomy_term_hierarchy table will look like so:
tid | parent |
---|---|
1 | 0 |
3 | 1 |
3 | 2 |
In the third row "Term 2" is still listed as a parent, even though it no longer exists.
Proposed resolution
When deleting a term, delete all rows within taxonomy_term_hierarchy that list that term as a parent.
If it only has one child term, that child term will be deleted. This works. We don't need to worry about that.
Original report by nargiza
I noticed that if we have a taxonomy structure like this in table term_hierarchy:
tid parent
term1 0
term2 0
term3 term1
term3 term2
where term3 has multiple parents. If we delete, for example, term1 we obtain this result:
tid parent
term2 0
term3 term1
term3 term2
But, I don't understand why the second record is not deleted. I think it is a bug because we don't need this record anymore.
In the taxonomy module (file taxonomy.module), in function taxonomy_del_term(), after these lines:
if (count($parents) == 1) {
$orphans[] = $child->tid;
}
we can put this:
else if (count($parents) > 1)
{
db_query('DELETE FROM {term_hierarchy} WHERE parent = %d', $t);
}
and, in this case, this record will be deleted too.
Comment | File | Size | Author |
---|---|---|---|
#120 | 726490-120.patch | 570 bytes | immaculatexavier |
#109 | 726490-109.patch | 4.17 KB | Munavijayalakshmi |
#108 | interdiff-726490-84-108.txt | 962 bytes | netlooker |
#108 | 726490-108.patch | 4.37 KB | netlooker |
#103 | 726490-103.patch | 4.38 KB | netlooker |
Comments
Comment #1
franzThat looks like a good patch to make.
In Drupal 7/8 the function was renamed to taxonomy_term_delete()
Comment #2
davisbenHere's a patch.
Comment #4
franz10oclock, I think you need to review the current tests and potentially write a new one for this case.
Comment #5
theduke CreditAttribution: theduke commentedI created a patch against current 8.x,
with extended comments and a tid condition to avoid side effects.
Comment #6
theduke CreditAttribution: theduke commentedRe-submitting previous patch for Testbot.
Comment #7
franzShould use proper softabs here
And we still needs tests.
Comment #8
oriol_e9gProper patch, we still need tests.
Tabs and 'else if' should be 'elseif'.
Comment #9
theduke CreditAttribution: theduke commentedI changed to elseif and wrote two new tests for Taxonomy:
testTermDeleteSingleParent checks that orphans get properly deleted when there is only one parent (no testing for this yet).
testTermDeleteMultipleParents checks that a term is not deleted when it has another parent, but that the parent reference gets deleted.
I did look over the taxonomy tests, but do not have much familiarity with the conventions for testing in core.
I hope my tests are alright.
Comment #10
Kristen PolComment #11
Kristen PolI didn't try out the patch, but here's some feedback on styling.
Comment needs better wrapping (move some of 2nd line to first line).
1) "behaviour" => "behavior" for American English.
2) Extra space at end of line (after "with").
For consistency, add new line after this section.
For consistency, add new line after this section.
Exceed 80 characters.
Exceed 80 characters.
Exceed 80 characters.
1) "behaviour" => "behavior"
2) Sentence can wrap better (move part of 2nd line up to 1st line).
Add space after section for consistency.
Exceeds 80 characters.
Exceeds 80 characters.
Comment #12
tmckeown CreditAttribution: tmckeown commentedThis issue was fixed in D8 when taxonomy terms became entities. Please see issue 1361232. Need to roll a patch for D7.
Comment #13
tmckeown CreditAttribution: tmckeown commentedSorry didn't mean to close the bug.
Comment #14
disasm CreditAttribution: disasm commentedThe original issue at the top expected term3 to no longer have a parent of term1 after term1 was deleted, but the current behavior deletes term3. Is this a bug? Should a term only be deleted if it has no other parents?
This was tested in 8.x.
Current behavior:
1. add term1 with no parent
2. add term2 with no parent
3. add term3 with parent of term1 and term2
4. remove term1
result: only term2 exists
Also happens if you remove term2:
1. add term1 with no parent
2. add term2 with no parent
3. add term3 with parent of term1 and term2
4. remove term2
result: only term1 exists
Comment #15
guschilds CreditAttribution: guschilds commentedThe bug originally described no longer exists because a term with two parents is getting incorrectly deleted when only one of its parents is deleted, as described in #14.
This happened because postDelete of a parent term in TermStorageController.php considered a child term with <= 1 parent to be an orphan. Because it is postDelete, the parent term in question has already been removed, so the child term being examined is only an orphan if there are 0 parents.
This is fixed in the attached patch.
Upon fixing this, the bug originally described resurfaced. Records referencing deleted terms as parents still exist. This is because records are only removed by searching the tid column when a term is removed. When a term has multiple parents, and one of them is deleted, it will still list that deleted term as a parent unless it itself is removed later.
The attached patch also deletes all records that list a term being deleted as a parent.
First core patch attempt. :)
Comment #16
mkostrzewa CreditAttribution: mkostrzewa commentedTested the patch and it did fix the issue mentioned in #14
Here is the testing process
Added the following:
Action: deleted term1
Result: only term2 left
and vice versa
After applying patch taxonomy-multiple-parents-726490-15.patch
Action: deleted term1
Result: term2 term3
Action: deleted term2
Result: term1 term3
Comment #17
druderman CreditAttribution: druderman commentedI looked over the comments on this issue and it appears that the "Needs tests" tag can be removed.
Comment #18
franzdruderman, "Needs tests" doesn't refer to manual testing. It refers to adding automated tests to this patch, in the way it is described on the docs: http://drupal.org/project/issues/search/drupal?issue_tags=Needs+tests
Comment #19
franz@guschilds, I think it's better to add a new issue as a spin-off of this one, and when it's fixed, we come back to this.
Comment #20
guschilds CreditAttribution: guschilds commentedFollowing franz's suggestion, I have created #2077387: Taxonomy term is improperly deleted when only one of two parents is deleted to address the bug described in #14-16 of this issue.
Once that bug is addressed, the bug originally described in this issue does resurface in Drupal 8. The attached patch fixes it.
To test:
After Step 3, the taxonomy_term_hierarchy table will look like so (using theoretical tids that correspond to their names):
After Step 4, the taxonomy_term_hierarchy table will look like so:
In the third row "Term 2" is still listed as a parent, even though it no longer exists. That is the bug originally reported.
After implementing the patch in #2077387: Taxonomy term is improperly deleted when only one of two parents is deleted, applying the attached patch here, and re-testing the instructions, the taxonomy_term_hierarchy table will look like so after Step 4:
"Term 2" was successfully removed from the records as a parent.
Comment #22
kasperg CreditAttribution: kasperg commentedI'll try to take a stab at this.
Comment #23
timdavison CreditAttribution: timdavison commentedIm sprinting in Prague looking at this one.
Comment #24
kasperg CreditAttribution: kasperg commentedAfter much confusion I'll reassign this back to me.
Comment #25
kasperg CreditAttribution: kasperg commented#20: taxonomy-multiple-parents-726490-20.patch queued for re-testing.
Comment #27
kasperg CreditAttribution: kasperg commentedHere is a working patch based on the work in #20 which failed testing.
Note that there are no test cases for checking for the erroneous situation as there seems to be no way to retrieve data only from the taxonomy_term_hierarchy table without querying the database directly. That seems to be the wrong way to go about this.
Comment #27.0
kasperg CreditAttribution: kasperg commentedUpdated issue summary.
Comment #28
selwynpolit CreditAttribution: selwynpolit commented27: taxonomy-multiple-parents-726490-26.patch queued for re-testing.
Comment #29
selwynpolit CreditAttribution: selwynpolit commented15: taxonomy-multiple-parents-726490-15.patch queued for re-testing.
Comment #30
selwynpolit CreditAttribution: selwynpolit commented9: multiple_parents-726490-9.patch queued for re-testing.
Comment #31
selwynpolit CreditAttribution: selwynpolit commented8: multiple_parents-726490-8.patch queued for re-testing.
Comment #36
joshi.rohit100Patch Submitted for D8
Comment #37
joshi.rohit100Comment #38
guschilds CreditAttribution: guschilds commentedI just tested the patch from #36 and unfortunately it did not solve the problem. Following the steps outlined in the issue summary and in my description in #20, the deleted term is still listed as a parent in taxonomy_term_hierarchy when its child term had multiple parents.
I've rerolled my patch from #20, which continues to fix the original problem.
Thanks,
Gus
Comment #39
guschilds CreditAttribution: guschilds commentedComment #41
joshi.rohit100updated patch. Please review now.
Comment #42
guschilds CreditAttribution: guschilds commented#41 passes tests and works. Thanks!
The only issue I'm seeing is the lack of a single space between the "//" and the comment text, "Remove...".
Comment #43
guschilds CreditAttribution: guschilds commentedAttached is a backport to D7.
I've marked it as do-not-test so it doesn't get tested against 8.x.
Comment #44
guschilds CreditAttribution: guschilds commentedAttached is a backport to D6.
I've marked it as do-not-test so it doesn't get tested against 8.x.
Comment #45
joachim CreditAttribution: joachim commentedThis is just an eyeball review, and I don't pretend to understand the code...
But it looks like this is inside a conditional check for whether the system maintains an index table.
Surely the presence of an index table is independent of the existence of the term hierarchy table?
Comment #46
lokapujyaHere is another way to fix it, incase we don't want to use hook_taxonomy_term_delete.
Comment #47
nmeegama CreditAttribution: nmeegama commentedI m reviewing this @amsterdam2014
Comment #48
nmeegama CreditAttribution: nmeegama commentedThe patch by @lokapujya works fine just a small fix though or an opinion rather. I Just think that the unparentTerms() function should not be called from the postDelete in the Term Class for the sake of best practices. one function should be called from postDelete that deletes the terms and the parent references from the term_hierarchy table so i created this patch.
Comment #49
lokapujyaAwe, I spent so much time coming up with the name "unparent", haha.
Comment #50
nmeegama CreditAttribution: nmeegama commented:) Unparent
With great power comes great responsibility. Sorry man
Comment #51
lokapujyaSo, we just need tests.
Comment #52
nmeegama CreditAttribution: nmeegama commentedI will write the test on for this
Comment #53
lokapujyaComment #54
lokapujyaSince the test is failing, I think we maybe didn't need the bug fix code afterall. Possibly just need to clear the storage cache. I tried calling resetcache() from post_delete() but it doesn't seem to work.
Comment #56
lokapujyaFix a line that shouldn't be commented out.
Comment #57
lokapujyaClearing cache worked locally. But is it acceptable?
Comment #60
lokapujyaFix the exception.
Comment #61
pingers CreditAttribution: pingers commentedUse sentence style (period).
"Array of parent terms that need to be removed from hierarchy." - no plural on 'needs' and use sentence style (period).
Comment #62
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedDid changes in this patch as @pingers suggested.
Comment #63
Dragooon CreditAttribution: Dragooon commentedLatest patch (#62) looks good
Comment #64
lokapujyaI want to get an opinion on the change in #57.
Comment #65
arunkumark@lokapujya #57 patch works fine and only the problem I notice Comment standards for functions ( https://www.drupal.org/coding-standards/docs#functions ). Patch #62 tested with latest version; its works fine; and I've added some screenshots of the tested patch.
Create Child:
List of terms:
After delete:
Comment #66
alexpottLet's just call both
deleteTermHierarchy
anddeleteTermHierarchyByParent
here.Needs an empty line between method description and @param doc.
This should be on the interface no? But actually I think we should not introduce this because it is hard to explain the difference from
deleteTermHierarchy
Comment #67
lokapujyaNo one seems concerned that a resetCache is required in the test after deleting a term (see change in #57)? If that turns out to be a problem, we could open a new issue.
Comment #68
lokapujyaMade the suggested changes from #66.
Comment #69
leslieg CreditAttribution: leslieg commentedComment #70
disasm CreditAttribution: disasm at AppliedTrust commentedI am removing the Novice tag from this issue because there are no novice action items for this task.
I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid
Comment #71
lokapujyaTalked to Wim Leers about the resetCache() concern that I brought up in #57 and #67. It is due to static cache, and it's only necessary during the test because of use of the variable in the same request.
Comment #72
leslieg CreditAttribution: leslieg as a volunteer commentedTested against latest head version with 726490-68.patch applied
Screenshot of database after deleting term 2 before patch (726490-68.patch-before.png) and after deleting term 2 after patch (726490-68.patch-after.png) are attached.
Both references to term 2 are now correctly deleted from the database.
Comment #73
leslieg CreditAttribution: leslieg as a volunteer commentedReviewed and passed in D8
Comment #74
alexpottI think we can do better by reorganising
updateTermHierarchy
anddeleteTermHierarchy
. This way we can optimise the delete with an or and encapsulate the update nicely. Also havingdeleteTermHierarchy
anddeleteTermHierarchyByParent
is very confusing.See patch attached for what I'm on about.
Comment #75
mgiffordPatch no longer applies.
Comment #76
lokapujyaRerolled.
Comment #78
lokapujyaComment #79
lokapujyaComment #80
jibranCan we have a test only patch to see the bug?
Comment #81
lokapujyaTest only.
Comment #82
jibranRTBC if red and green. Thanks.
Comment #83
oriol_e9gTest only green :(
Comment #84
lokapujyaSince https://www.drupal.org/node/1976298, loadParents() now returns the term objects. So, the DB is still wrong, but loadParents() masks the bug. Modified the test to just load the hierarchy.
Comment #86
jibran$term is deleted so using its id here doesn't make sense.
Comment #87
lokapujyaWell, that's the point. loadTree() shouldn't find anything for that term, but it does.
loadParents() doesn't show the bug anymore. In order to have a test only that fails, I used loadTree(). Instead of loadTree(), the test could do a Select on taxonomy_term_field_data? I was trying to find a way to use the API rather than do a select.
Comment #89
jhedstromThat is concerning, and probably means that a cache tag on the child needs to be invalidated instead.
Comment #91
suresh_ewall CreditAttribution: suresh_ewall as a volunteer and commentedHi,
We have tested the mentioned patch and reviewed that patch. Everything seems working fine. Thanks for the patch.
Comment #92
suresh_ewall CreditAttribution: suresh_ewall as a volunteer and commentedHi,
We have tested the mentioned patch and reviewed that patch. Everything seems working fine. Thanks for the patch.
Comment #95
alexpottWe need more tests here. We're not actually testing the hierarchy after deleting a parent. We also need to check the hierarchy of $term2 which had $term3 and $term1 as parents but now should only have $term1.
We need a reroll too. Given that this has needs a reroll since:
I'm not sure what was reviewed in #91? Was the code applied and core functionality tested? It is great to document what you have done when you rtbc a patch.
Comment #96
netlooker CreditAttribution: netlooker commentedI've applied patch https://www.drupal.org/files/issues/726490-84.patch without any issues. I've also performed all tests for the taxonomy and no issues here as well. It seems that there is no need to reroll the patch at least for the recent D8 version.
Comment #97
alexpott@netlooker this needs to be applied to 8.4.x first and then 8.3.x (since it is a bug fix without BC implication as far as I can see). Although one reason it might not get in to 8.3.x is that we really should have an upgrade path to remove entries where the term ID no longer exists.
Anyways the patch in #84 does not apply to 8.4.x or 8.3.x.
Comment #98
netlooker CreditAttribution: netlooker commented@alexpott thanks for clarifying. I will work on it today most probably in the evening.
Comment #99
netlooker CreditAttribution: netlooker commentedI've rerolled the patch for the 8.4.x branch. I've also performed all tests for the taxonomy and all of them are green.
This patch is also applicable to the 8.3.x branch and taxonomy tests are green as well.
Comment #100
netlooker CreditAttribution: netlooker commentedComment #101
lokapujyaThanks @netlooker,
Can you try adding the tests mentioned in #95?
Comment #102
netlooker CreditAttribution: netlooker commented@lokapujya ok, I will try and keep you posted.
Comment #103
netlooker CreditAttribution: netlooker commentedI've prepared the following patch with the test which checks if there is an existing parent ($term1). I've checked the results and I didn't find any issues. @lokapujya please let me know if it is enough or did I miss something :)
Comment #104
lokapujyaYou need to set to "Needs Review" so that the patch gets tested. Also, can you provide an interdiff so that we can see the changes?
Comment #105
lokapujyaThe [] brackets are wrong. As is, the condition is inside the [].
Comment #106
lokapujyaAlso, I recommend rewording the comment. Maybe change "deleting" to "deletion". I also wouldn't put the variable name in the comment, because then if the code changes, the comment has to get changed too.
Comment #107
netlooker CreditAttribution: netlooker commentedThanks for the valuable feedback. I will implement your suggestions.
Comment #108
netlooker CreditAttribution: netlooker commented@lokapujya I've added given suggestions and created interdiff between #84 and the latest version of the patch.
Comment #109
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedRerolled the patch.
Comment #119
larowlanAdding some tags
Comment #120
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedBackported to D7
Comment #121
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #122
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@larowlan #120 - Backported to D7
Comment #123
LendudeI've manually tested this and this no longer occurs, there are no longer any orphaned records after deleting one of two parents.
Also verified this by running the automated test, it is green on HEAD.
Closing this as outdated for now, if you feel this is still an issue, feel free to re-open this issue.