Problem/Motivation

It is possible to have two terms with the same names at a same level of the hierarchy. (i.e. two terms with the same name and the same parent term). Duplications can lead to problem in modules that rely on taxonomy to create say menus or url paths.

Proposed resolution

Modify taxonomy_save_term so that it checks if inserting/updating the term will create a duplication and abort the operation if needed.

Remaining tasks

Because of the age of this issue, I recommend the following tasks:

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions Yes
Add steps to reproduce the issue Novice Instructions
Embed before and after screenshots in the issue summary Novice Instructions
Manually test the patch Novice Instructions
Reroll the patch if it no longer applies. Instructions

User interface changes

The patch for taxonomy.admin.inc notifies the admin of the duplication problem when it happens.

API changes

The patch for taxonomy.module modifies taxonomy_save_term so that it checks if inserting/updating the term will create a duplication and abort the operation if needed.

CommentFileSizeAuthor
#5 taxonomy.admin_.inc_.patch2.83 KBtwiinz
#4 taxonomy.admin_.inc_.patch2.32 KBtwiinz
#4 taxonomy.module.patch1.23 KBtwiinz
#3 taxonomy.admin_.inc_.patch2.37 KBtwiinz
#3 taxonomy.module.patch1.29 KBtwiinz
taxonomy.module.patch1.29 KBtwiinz
taxonomy.admin_.inc_.patch3.36 KBtwiinz

Comments

twiinz’s picture

twiinz commented
Title: Possible duplication of terms at a given level hierarchy » Possible duplication of terms at a given level of the hierarchy
macgirvin’s picture

macgirvin commented

+1

This has been a thorn in my side for years - see #41043: taxonomy.module - duplicate terms allowed in same vocabulary; though I found aggregator2 to be quite unstable at the time and don't use it anymore.

If you follow the comments on that issue, you'll find that it 'works as designed' even though I vehemently disagree. I've got no problem with duplicate terms across vocabularies, or within other branches of the tree, but to be on the same branch is pointless and confusing. A proposed solution was to run a perl script to merge these back together again - but they shouldn't happen in the first place and I don't know many site admins that want to start their day looking for duplicate terms which got inserted via an RSS feed and manually running scripts to set things straight.

I note that the way aggregator2 once worked, this patch would give it grief as it blindly created a term and used the returned ID from the create call to categorize external feed postings. Failures weren't checked or acted on properly. Hopefully nobody does this anymore and they actually check for existence of terms before using them or creating them. But I note that since you've found this bug, you've found something that didn't play along - hopefully it was a manual entry and not another module to be fixed. But there may be good reason in the taxonomy API (if that still exists) to just return the existing term ID rather than a duplicate error.

twiinz’s picture

twiinz commented
StatusFileSize
new taxonomy.module.patch1.29 KB
new taxonomy.admin_.inc_.patch2.37 KB

Fixed a typo present in the previous patch (taxonomy.admin.inc).

Sorry about that.

twiinz’s picture

twiinz commented
StatusFileSize
new taxonomy.module.patch1.23 KB
new taxonomy.admin_.inc_.patch2.32 KB

I slept on it and realized that the patch could cause the following problem:

Context:
- Only one term has a parent
- One tries to move this term to the root (which should give a flat vocabulary hierarchy) but the attempt fails because it would duplicate with a term already present at the root.

Expected result:
- The term shouldn't be moved and the admin should be notified (works with previous patch)
- The vocabulary hierarchy should still be set to single, i.e. not flat (doesn't work with previous patch)

Problem:
- Whether the vocabulary hierarchy is flat or single is calculated before any attempt to save the term to the database
- Therefore the following block of code doesn't work as expected because $hierarchy is set to 0 whereas it should still be set to 1

<?php
  // Update the vocabulary hierarchy to flat or single hierarchy.
  if ($vocabulary['hierarchy'] != $hierarchy) {
    $vocabulary['hierarchy'] = $hierarchy;
    taxonomy_save_vocabulary($vocabulary);
  }
?>

Solution:
- When a term can't be moved because it would create a duplicate, we check if it has a parent and update $hierarchy appropriately.

Cleaned the patch a little bit. There should be no more side effects. I'm not native English speaker so the 3 additional calls to drupal_set_message and watchdog might need some love.

twiinz’s picture

twiinz commented
StatusFileSize
new taxonomy.admin_.inc_.patch2.83 KB

Learning more and more about drupal and found out that there was a better way to implement this: form validation functions.

This new patch doesn't require modifying taxonomy_save_term, so there is no more need to patch taxonomy.module.

It doesn't modify the two submit functions neither, it just adds the necessary code to taxonomy_form_term_validate and creates the new function taxonomy_overview_terms_validate.

From a usability point of view it's also a lot better as the user is now notified of the problem as he's editing the forms and can therefore take appropriate action.

This patch also support terms with multiple parents which wasn't the case with the previous one.

This should be a much cleaner cut.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

+1. I'll try and review this when I get some time.

Still applies with offset.

catch’s picture

catch
he/him
Primary language English
commented
Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

This needs to go into Drupal 7 first, and also needs updating for the new database layer. Good change though.

marcingy’s picture

marcingy commented
Version: 7.x-dev » 8.x-dev

Bumping to d8 this is still an issue.

marcingy’s picture

marcingy commented
Issue tags: +Needs backport to D7

Adding tag

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Status: Needs work » Active
Issue tags: +Needs reroll, +Needs tests

Last patch is 5 years old I think it needs reroll and some tests for new functionality. I don't think it is a bug It should be a task.

amytswan’s picture

amytswan commented

I'm at DrupalConLA's code sprint - checking out this issue to reroll the last patch

amytswan’s picture

amytswan commented
Issue tags: -Needs reroll

When I rerolled I discovered, yes, there are issues. Patch is now 7 years old. Here's the output from my Terminal:

git apply --index taxonomy.admin_.inc__4.patch
taxonomy.admin_.inc__4.patch:24: trailing whitespace.
}
taxonomy.admin_.inc__4.patch:25: trailing whitespace.
}
taxonomy.admin_.inc__4.patch:52: trailing whitespace.

error: taxonomy.admin.inc: does not exist in index

Removing "needs reroll" tag.

jcandan’s picture

jcandan commented

Also at DrupalConLA's code sprint. Noticed that the above was kicked by git for trailing whitespace. Will update patch and remove whitespace.

jcandan’s picture

jcandan commented
Issue tags: +Needs reroll

Never mind the whitespace, at the bottom of the terminal output is a error: taxonomy.admin.inc: does not exist in index. It seems that the patch wasn't attempted in a directory where it recognized the path. Attempting reroll.

jcandan’s picture

jcandan commented
Status: Active » Needs review

Can't find file for patching. It's pretty old. I'm pretty sure that, although it might still be an issue in D8 as pointed out by #8, it's not applicable to D8. Recommending we mark as Closed (cannot reproduce).

The last submitted patch, taxonomy.module.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: taxonomy.admin_.inc_.patch, failed testing.

The last submitted patch, 4: taxonomy.admin_.inc_.patch, failed testing.

The last submitted patch, 3: taxonomy.module.patch, failed testing.

The last submitted patch, 3: taxonomy.admin_.inc_.patch, failed testing.

The last submitted patch, 4: taxonomy.module.patch, failed testing.

The last submitted patch, taxonomy.admin_.inc_.patch, failed testing.

jcandan’s picture

jcandan commented
Version: 8.0.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

Actually, marking back to D7, and removing "needs backport to D7". This should have never been bumped to D8. If the issue did exist in D8, then a new ticket should have been created detailing the quickest path to recreate the problem.

jcandan’s picture

jcandan commented
Issue summary: View changes

Update description via dreditor templates. Update Remaining tasks list.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.