Taxonomy term record for term with multiple parents is not deleted when parent is deleted

That looks like a good patch to make.

In Drupal 7/8 the function was renamed to taxonomy_term_delete()

Here's a patch.

10oclock, I think you need to review the current tests and potentially write a new one for this case.

I created a patch against current 8.x,
with extended comments and a tid condition to avoid side effects.

Re-submitting previous patch for Testbot.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -763,11 +763,20 @@ function taxonomy_term_delete($tid) {
+            	db_delete('taxonomy_term_hierarchy')

Should use proper softabs here

And we still needs tests.

Proper patch, we still need tests.

Tabs and 'else if' should be 'elseif'.

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

I didn't try out the patch, but here's some feedback on styling.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -763,11 +763,20 @@ function taxonomy_term_delete($tid) {
+            // If the child has multiple parents,
+            // we do not delete it, but we remove the parent reference.

Comment needs better wrapping (move some of 2nd line to first line).

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+    * Test taxonomy behaviour when deleting terms in a hierarchy with ¶
+    * a linear structure (only one parent per term).

1) "behaviour" => "behavior" for American English.

2) Extra space at end of line (after "with").

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     // $term[3] is a child of 2.
+     $term[3]->parent = array($term[2]->tid);
+     taxonomy_term_save($term[3]);

For consistency, add new line after this section.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     // $term[4] is a child of 3.
+     $term[4]->parent = array($term[3]->tid);
+     taxonomy_term_save($term[4]);

For consistency, add new line after this section.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     $this->assertFalse($termOne, 'Child of term was automatically deleted on parent deletion.');

Exceed 80 characters.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     $terms = taxonomy_term_load_multiple(array($term[3]->tid, $term[4]->tid, $term[5]->tid));

Exceed 80 characters.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     $this->assertTrue(empty($terms), 'Children of term were automatically deleted on parent deletion.');

Exceed 80 characters.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+   * Test taxonomy behaviour when deleting terms in a hierarchy with

1) "behaviour" => "behavior"

2) Sentence can wrap better (move part of 2nd line up to 1st line).

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     // $term[2] is a child of 0 and 1.
+     $term[2]->parent = array($term[0]->tid, $term[1]->tid);
+     taxonomy_term_save($term[2]);

Add space after section for consistency.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     // Ensure that a child term does NOT get auto-deleted when it has another parent,

Exceeds 80 characters.

+++ b/core/modules/taxonomy/taxonomy.test
@@ -484,6 +484,97 @@ class TaxonomyTermUnitTest extends TaxonomyWebTestCase {
+     $this->assertFalse(empty($children), 'Child of term was not deleted since it had another parent.');

Exceeds 80 characters.

This issue was fixed in D8 when taxonomy terms became entities. Please see issue 1361232. Need to roll a patch for D7.

Sorry didn't mean to close the bug.

The 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

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

Tested the patch and it did fix the issue mentioned in #14

Here is the testing process

Added the following:

1. term1 - no parent
2. term2 - no parent
3. term3 - parent of term 1 and 2

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

I looked over the comments on this issue and it appears that the "Needs tests" tag can be removed.

druderman, "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

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

Following 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:

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"

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.

I'll try to take a stab at this.

Im sprinting in Prague looking at this one.

After much confusion I'll reassign this back to me.

#20: taxonomy-multiple-parents-726490-20.patch queued for re-testing.

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

Updated issue summary.

27: taxonomy-multiple-parents-726490-26.patch queued for re-testing.

15: taxonomy-multiple-parents-726490-15.patch queued for re-testing.

9: multiple_parents-726490-9.patch queued for re-testing.

8: multiple_parents-726490-8.patch queued for re-testing.

Patch Submitted for D8

I 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

updated patch. Please review now.

#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...".

Attached is a backport to D7.

I've marked it as do-not-test so it doesn't get tested against 8.x.

Attached is a backport to D6.

I've marked it as do-not-test so it doesn't get tested against 8.x.

+++ b/core/modules/taxonomy/taxonomy.module
@@ -837,6 +837,8 @@ function taxonomy_taxonomy_term_delete(Term $term) {
   if (\Drupal::config('taxonomy.settings')->get('maintain_index_table')) {
     // Clean up the {taxonomy_index} table when terms are deleted.
     db_delete('taxonomy_index')->condition('tid', $term->id())->execute();
+    //Remove the reference to this term by other terms.
+    db_delete('taxonomy_term_hierarchy')->condition('parent', $term->id())->execute();
   }

This 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?

Here is another way to fix it, incase we don't want to use hook_taxonomy_term_delete.

I m reviewing this @amsterdam2014

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

Awe, I spent so much time coming up with the name "unparent", haha.

:) Unparent
With great power comes great responsibility. Sorry man

So, we just need tests.

I will write the test on for this

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

Fix a line that shouldn't be commented out.

Clearing cache worked locally. But is it acceptable?

Fix the exception.

1. +++ b/core/modules/taxonomy/src/TermStorageInterface.php
   @@ -24,6 +24,13 @@
   +   * Removes reference to terms from term_hierarchy parent
   

   Use sentence style (period).

2. +++ b/core/modules/taxonomy/src/TermStorageInterface.php
   @@ -24,6 +24,13 @@
   +   *  Array of parent terms that needs to be removed from hierarchy
   

   "Array of parent terms that need to be removed from hierarchy." - no plural on 'needs' and use sentence style (period).

Did changes in this patch as @pingers suggested.

Latest patch (#62) looks good

I want to get an opinion on the change in #57.

@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:

1. +++ b/core/modules/taxonomy/src/Entity/Term.php
   @@ -77,7 +77,7 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
   -    $storage->deleteTermHierarchy(array_keys($entities));
   +    $storage->deleteTermsFromTermHierarchy(array_keys($entities));
   

   Let's just call both deleteTermHierarchy and deleteTermHierarchyByParent here.

2. +++ b/core/modules/taxonomy/src/Entity/Term.php
   --- a/core/modules/taxonomy/src/TermStorage.php
   +++ b/core/modules/taxonomy/src/TermStorage.php
   
   +++ b/core/modules/taxonomy/src/TermStorageInterface.php
   @@ -24,6 +24,13 @@
   +   * Removes reference to terms from term_hierarchy parent.
   +   * @param array $pids
   

   Needs an empty line between method description and @param doc.

3. +++ b/core/modules/taxonomy/src/TermStorage.php
   @@ -106,6 +106,23 @@ public function deleteTermHierarchy($tids) {
   +  /**
   +   * Removes the terms and reference to parents from term_hierarchy.
   +   */
   +  public function deleteTermsFromTermHierarchy($tids) {
   

   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

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

Made the suggested changes from #66.

I 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

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

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

Reviewed and passed in D8

I think we can do better by reorganising updateTermHierarchy and deleteTermHierarchy. This way we can optimise the delete with an or and encapsulate the update nicely. Also having deleteTermHierarchy and deleteTermHierarchyByParent is very confusing.

See patch attached for what I'm on about.

Patch no longer applies.

Rerolled.

Can we have a test only patch to see the bug?

Test only.

RTBC if red and green. Thanks.

Test only green :(

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

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -116,6 +116,16 @@ function testTaxonomyTermHierarchy() {
+    $parents = $taxonomy_storage->loadTree($this->vocabulary->id(), $term3->id());

$term is deleted so using its id here doesn't make sense.

Well, 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.

No one seems concerned that a resetCache is required in the test after deleting a term (see change in #57)?

That is concerning, and probably means that a cache tag on the child needs to be invalidated instead.

Hi,

We have tested the mentioned patch and reviewed that patch. Everything seems working fine. Thanks for the patch.

Hi,

We have tested the mentioned patch and reviewed that patch. Everything seems working fine. Thanks for the patch.

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -116,6 +116,16 @@ function testTaxonomyTermHierarchy() {
+    // Check the hierarchy after deleting one parent of multiple parents.
+    $taxonomy_storage->resetCache();
+    $parents = $taxonomy_storage->loadTree($this->vocabulary->id(), $term3->id());
+    $this->assertTrue(count($parents) == 0, 'Term 3 has no lineage.');

We 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:

commit 367c98f45c3fd1718d1605a600f6ed367a03862c
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Fri Oct 21 06:28:02 2016 -0700

    Issue #2761059 by govind.maloo, arunkumark, tstoeckler: Term::postDelete() needlessly uses entity_delete_multiple()

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.

I'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.

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

@alexpott thanks for clarifying. I will work on it today most probably in the evening.

I'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.

Thanks @netlooker,

Can you try adding the tests mentioned in #95?

@lokapujya ok, I will try and keep you posted.

I'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 :)

You 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?

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -111,6 +111,21 @@ function testTaxonomyTermHierarchy() {
+    $this->assertTrue(isset($parents[$term1->id() && count($parents) == 1]), 'Existing parent found successfully.');

The [] brackets are wrong. As is, the condition is inside the [].

+++ b/core/modules/taxonomy/src/Tests/TermTest.php
@@ -111,6 +111,21 @@ function testTaxonomyTermHierarchy() {
+    // Check the hierarchy after deleting for the existing parent ($term1).

Also, 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.

Thanks for the valuable feedback. I will implement your suggestions.

@lokapujya I've added given suggestions and created interdiff between #84 and the latest version of the patch.

Rerolled the patch.

Adding some tags

Backported to D7

@larowlan #120 - Backported to D7

I'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.