Make WordPress Core

Opened 16 years ago

Closed 9 years ago

Last modified 7 years ago

#5809 closed defect (bug) (fixed)

Updating a term in one taxonomy affects the term in every taxonomy

Reported by: rmccue's profile rmccue Owned by: boonebgorges's profile boonebgorges
Milestone: 4.2 Priority: normal
Severity: major Version: 2.3
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description (last modified by lloydbudd)

As reported by klawd on #wordpress and reproduced by me, editing a category will affect a tag with the same name.

Steps to reproduce:
1. Create a category called Testing
2. Create a tag called Testing
3. Rename the Testing category to Another Test
4. Check the name of the tag

Attachments (14)

structure.txt (416 bytes) - added by rmccue 16 years ago.
Proposed new structure of tables (by klawd)
5809.diff (838 bytes) - added by greuben 13 years ago.
5809-2.diff (963 bytes) - added by greuben 13 years ago.
garyc40.5809.diff (3.8 KB) - added by garyc40 13 years ago.
create new tag instead of modifying existing one if the user changes its name or slug and the term is in multiple taxonomies
terms.diff (2.7 KB) - added by wonderboymusic 12 years ago.
5809.03.diff (859 bytes) - added by imath 10 years ago.
5809-jesin.diff (2.8 KB) - added by jesin 10 years ago.
Refresh and fix patch terms.diff
5809.patch (16.8 KB) - added by boonebgorges 9 years ago.
5809.2.patch (18.0 KB) - added by boonebgorges 9 years ago.
5809.3.patch (18.4 KB) - added by boonebgorges 9 years ago.
5809.4.patch (18.5 KB) - added by boonebgorges 9 years ago.
5809.5.patch (16.5 KB) - added by boonebgorges 9 years ago.
5809.6.patch (18.1 KB) - added by boonebgorges 9 years ago.
Refreshed for 4.2.0. Introduces wp_get_split_terms(), which can be used to retrieve stored data about previously split terms.
5809.2.diff (18.7 KB) - added by boonebgorges 9 years ago.
Refresh of .6.diff. Splits wp_get_split_term() and wp_get_split_terms() for more predictable return values.

Download all attachments as: .zip

Change History (177)

#1 @rmccue
16 years ago

Oops, I meant:

Steps to reproduce:

  1. Create a category called Testing
  2. Create a tag called Testing
  3. Rename the Testing category to Another Test
  4. Check the name of the tag

@rmccue
16 years ago

Proposed new structure of tables (by klawd)

#2 @rmccue
16 years ago

  • Cc klawd@… added

#3 @lloydbudd
16 years ago

  • Description modified (diff)

#4 @asmodai
16 years ago

  • Version set to 2.5

I can verify that this is a problem in the released 2.5.

#5 @asmodai
16 years ago

The problem is that wp_term_taxonomy refers to wp_terms for the name of the category/tag for both category and tag. One could argue that tagging a post filed in a category with the same name as the tag is redundant. klawd's restructuring makes sense to me though, proper separation might be better overall.

#6 @Denis-de-Bernardy
15 years ago

  • Component changed from General to Taxonomy
  • Owner changed from anonymous to ryan

#7 @tenzochris
14 years ago

Confirmed this is still a problem in 2.8.4 - good to see it's on the list for 2.9, though.

#8 @ryan
14 years ago

  • Milestone changed from 2.9 to Future Release

There won't be taxonomy schema changes, at least for a long time. This will have to be addressed by creating a new term when changing the name of a term present in multiple taxponomies.

#9 @archon810
14 years ago

Hi guys,

I had a similar situation just now and I think this behavior is very confusing and limiting to WP users.

Here's what happened:

  • I created a few posts with tags, such as "HTC".
  • I then decided to add a few categories, one of which is Phones, with a child one called "HTC"
  • The categories were created OK but the slug was not, because it is set as UNIQUE in the DB table. Instead, the current logic falls back on the slug comprised of parent categories, until the slug is made unique (such as phones-htc).
  • Now, the url to the post will have phones/phones-htc (and if I had more problematic slugs like this, it could get even uglier: phones/htc-phones/hero-htc-phones-phones). This is bad for SEO and is straight up confusing to the user who enters a slug but gets another slug when he or she refreshes the page.

In summary, it's a shortcoming of the application to not allow the same category name as the tag name. There are workarounds, of course, such as renaming the slug slightly, but it's still a bug.

Now, as far as the solution, because the wp_terms table doesn't have the term type, it's not possible to make a composite unique key. Therefore, the unique key would need to be dropped and proper behavior would need to be enforced from the application (WP).

Any other suggestions are welcome.

Thanks, guys.

#10 @sainz
14 years ago

  • Severity changed from normal to major
  • Version changed from 2.5 to 2.9.2

I have this problem too.

I use WPMU 2.9.2 and Buddypress 1.2.1.

Thanks!

#11 @nacin
14 years ago

  • Keywords needs-patch added
  • Severity changed from major to normal
  • Version changed from 2.9.2 to 2.3

Please do not change the version number. This tells developers when the bug was introduced, reported, etc. Restoring severity as well.

There was a suggestion made above for how we should address this. This needs a patch.

#12 @kevinB
14 years ago

  • Cc kevinB added

@greuben
13 years ago

@greuben
13 years ago

#13 @greuben
13 years ago

  • Keywords has-patch 2nd-opinion needs-testing added; needs-patch removed

Attached two patches. The first patch creates unique slug at wp_insert_term, the second patch creates unique term at wp_update_term.

Personally, I like the first patch.

#14 @garyc40
13 years ago

  • Owner changed from ryan to garyc40
  • Status changed from new to assigned

@garyc40
13 years ago

create new tag instead of modifying existing one if the user changes its name or slug and the term is in multiple taxonomies

#15 follow-up: @garyc40
13 years ago

Just attached a patch that does what ryan wants in comment 8:

  • If the tax is present in multiple taxes and the name or slug in one tax is changed:


+ create a new term with the new settings

+ reassign posts in the old term to the new term


+ delete the old term in the current tax

  • As a result, the original term in other taxes is untouched

#16 in reply to: ↑ 15 @jghazally
13 years ago

worked for me, :D

#17 @scribu
13 years ago

  • Cc scribu added

#18 @mikeschinkel
13 years ago

  • Cc mikeschinkel@… added

#19 @scribu
13 years ago

Related: #16936

#20 @scribu
13 years ago

Duplicate: #16941

#21 @garyc40
13 years ago

  • Keywords 3.2-early added

This issue has persisted for quite a long time now. I know some plugin that depend on this being fixed. Would be great if it can be in 3.2 :)

If not, please remove the 3.2-early keyword I just added.

#22 @voidtrance
13 years ago

Hi,

Is there any update on this issue? I have a site on which most of the category/tag archive links return 404 errors due to this.
Also, there is no way for me to fix it since WP updates the slugs for both the category and tag at the same time.

Site address: http://www.voidtrance.net

Thank you

#23 @voidtrance
13 years ago

  • Severity changed from normal to major

3.2 does not seem to fix the issue. I still can't change either the category slug or the tag slug without the change affecting both category and tag.

What this means in my case is that the majority of my categories/tags return 404 errors when accessed.

I am raising the Severity to major because to me this is a major issue that should have been fixed by now.

#24 @ab-tools
13 years ago

  • Cc ab-tools added

I just came across exactly the same issue now and can't believe that such a major bug that produces really unexpected behavior has not been fixed for 4 years now!

It looks like as there are already patched available:
so what's the reason why they don't get integrated into the main system for such a long time?

Is there any planning when this will get fixed?

#25 follow-up: @nacin
13 years ago

  • Keywords needs-unit-tests added; 2nd-opinion needs-testing 3.2-early removed

I'm happy to support this, but we need unit tests. The quicker those happen...

#26 @coombesy
12 years ago

I'm reporting this as well. Using wp 3.2.1 MU.

Just built an app that used hierichal custom taxonomy called 'warehouse'.
Now found out I can't have:
Old Stock-> Cameras-> Chargers
and
New Stock-> Chargers
In my new taxonomy

#27 in reply to: ↑ 25 ; follow-up: @havahula
12 years ago

Replying to nacin:

I'm happy to support this, but we need unit tests. The quicker those happen...

I would like to see this make it sooner than later. happy to help with testing.

#28 in reply to: ↑ 27 ; follow-up: @SergeyBiryukov
12 years ago

Replying to havahula:

I would like to see this make it sooner than later. happy to help with testing.

http://codex.wordpress.org/Automated_Testing#Writing_Tests

#29 in reply to: ↑ 28 @havahula
12 years ago

Replying to SergeyBiryukov:

Replying to havahula:

I would like to see this make it sooner than later. happy to help with testing.

http://codex.wordpress.org/Automated_Testing#Writing_Tests

thanks.

#31 @johnbillion
12 years ago

  • Cc johnbillion added

#32 @ryan
12 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

#33 @johnbillion
12 years ago

Any reason given for the maybelater?

#34 @ryan
12 years ago

  • Milestone set to Future Release
  • Resolution maybelater deleted
  • Status changed from closed to reopened

#35 @ryan
12 years ago

nacin seems interested, so have fun. :-)

#36 @lkraav
12 years ago

  • Cc lkraav added

#37 @Viper007Bond
12 years ago

  • Cc viper007bond added

#38 @TomAuger
12 years ago

  • Cc tomaugerdotcom@… added

#39 @scribu
12 years ago

Positive consequence if this gets fixed: #20536

#40 @nacin
12 years ago

I suggest we stop trying to look into wp_terms for an existing name/slug when creating a term. So on a new install, term_id would always equal tt_id — shared terms would no longer occur.

Needs unit tests. With them, I will see to this for 3.5.

#41 @TomAuger
12 years ago

Yep. That sounds right. Would need to be rigorously tested though, as you have pointed out.

#42 @duck_
12 years ago

This is particularly confusing when you have a nav menu with the same name as a tag/category/custom taxonomy term, see #20707.

Last edited 12 years ago by duck_ (previous) (diff)

#43 @sc0ttkclark
12 years ago

  • Cc lol@… added

#44 @husobj
12 years ago

  • Cc ben@… added

#45 @batmoo
12 years ago

  • Cc batmoo@… added

#46 @tott
12 years ago

  • Cc tott@… added

#47 @SergeyBiryukov
12 years ago

#21275 has another patch.

#48 follow-up: @wonderboymusic
12 years ago

can someone point me to where unit tests would be included / uploaded / patched, etc?

#49 follow-up: @wonderboymusic
12 years ago

both counts need to be updated when tt_id is updated in term_relationships - I updated my patch

#50 @anointed
12 years ago

+1 for inclusion sooner than later. I keep getting hit by this 'bug' myself.

#51 @sirzooro
12 years ago

  • Cc sirzooro added

#52 @hsatterwhite
12 years ago

  • Cc whsatterwhite@… added

#53 @SergeyBiryukov
12 years ago

Closed #21557 as a duplicate.

#54 @wonderboymusic
12 years ago

Now that #22023 is resolved, we can finally start moving forward with this ticket. 5809.patch is my proposed patch for 4.1.

A quick recap of the background will be helpful in explaining the patch. #22023 was a blocker for this ticket because splitting existing shared terms into separate terms requires duplicate slugs to appear in the wp_terms table. A tag and a category that share the term 'boone' (cool site, btw) will currently have default archive permalinks example.com/category/boone and example.com/tag/boone. If we want to split the tag and category into separate terms, they both have to have the slug 'boone'. That's why we need to remove the UNIQUE index before splitting existing shared terms.

This is somewhat more complex than it seems, because updates to the database schema don't propagate immediately. On Multisite, the Network Upgrade script needs to be triggered manually, and on large networks like wordpress.com, the upgrade can take weeks. So we have to be delicate about how we go about splitting shared terms: either we wait until we can be reasonably certain that all sites will have performed the necessary schema change (say, 4.2), or we do some checks of the actual db_version before splitting, and act accordingly in either case.

In 5809.patch, I've opted for the latter path. Once the schema change is applied to a given site, there's no reason IMO to wait on cleaning up this mess. As such, my patch does the following:

  • On wp_update_term(), if the schema change *has* been run, detect whether the tt_id shares its term_id with another tt_id. If so, split it off into its own term. If the schema change has *not* been run, do nothing - allow the shared term to stay shared. This solves the current ticket, at least for sites where the database schema change has taken place.
  • On wp_insert_term(), stop creating shared terms. This fixes #21950. If the schema change *has* been run, we allow a duplicate slug to be created. In other words: If you have a tag 'boone', you'll be allowed to create a new category called 'boone', which will create a new item in 'wp_terms'. If the schema has *not* been updated, we cannot allow a duplicate slug to be created; instead, we force a new slug in wp_unique_term_slug(). In other words: If you have a tag 'boone', a category called 'Boone' will get slug 'boone-2'.

It's my view that we should not attempt forcibly splitting existing terms on WP version upgrade until we can be reasonably sure that the 4.1 schema change has taken place. So, 4.2 seems like a reasonable goal for that. Then we can start talking about further steps in the taxonomy roadmap.

Last edited 9 years ago by boonebgorges (previous) (diff)

#55 @wonderboymusic
12 years ago

Is there any particular reason this conversation is stalled? My patch works, and I'm sure there are other patches that work. I don't have Unit Tests set up, but if someone does and wants to help in this arena: that would rock.

#56 @sc0ttkclark
12 years ago

If no one has made a unit test or said they will within a week, I'll volunteer my time to set one up.

#57 in reply to: ↑ 48 @nacin
12 years ago

Replying to wonderboymusic:

can someone point me to where unit tests would be included / uploaded / patched, etc?

Unit tests can go into tests/term.php or tests/term/shared.php (for shared terms, or the demise thereof).

I'm really excited about getting this into core as I think it sets the stage for potential future changes to our taxonomy API and schema.

terms.diff looks like it handles quite a bit more than 5809.diff. That also means it would be *great* if there were individual tests for all of the different moving parts.

If this is fully tested and ready to go within the next few weeks, I want to land this in 3.5.

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#58 @tollmanz
12 years ago

  • Cc tollmanz@… added

I'm willing to work on these unit tests. I have everything setup and have committed unit tests before. The only catch is that I likely won't have time until late next week. Would also be happy to coordinate efforts with someone else as well.

#59 @wonderboymusic
12 years ago

  • Keywords needs-unit-tests removed

#60 in reply to: ↑ 49 @SergeyBiryukov
12 years ago

Replying to wonderboymusic:

both counts need to be updated when tt_id is updated in term_relationships - I updated my patch

Is there a reason to call _update_*_term_count() directly instead of wp_update_term_count_now()?

#61 @SergeyBiryukov
12 years ago

  • Keywords needs-unit-tests added

Per the IRC chat and http://core.trac.wordpress.org/report/37, it's better to keep the keyword until the tests are reviewed and committed.

#62 @ryan
12 years ago

Does it break global terms? ( global_terms_enabled(), install_global_terms(), sitecategories, etc. )

Last edited 12 years ago by ryan (previous) (diff)

#63 @ryan
12 years ago

Completely removing global terms is an option. But it should be discussed and not simply forgotten. :-)

#64 @nacin
12 years ago

The UNIQUE KEY slug in $wpdb->terms needs to be converted to a regular key as well.

#65 @scribu
12 years ago

Remove global terms. Do it: #21734

#66 @johnbillion
12 years ago

  • Summary changed from Categories affect tags of the same name to All terms affect terms of the same name

#67 @scribu
12 years ago

That new title makes no sense at all! Just read it out loud. :))

#68 @wonderboymusic
12 years ago

  • Summary changed from All terms affect terms of the same name to Updating a term in one taxonomy affects the term in every taxonomy

Let's keep changing it until we reach nirvana

#69 @scribu
12 years ago

To clarify, it might have made sense to those already familiar with the issue, but to outsiders, it's incomprehensible.

"All terms affect terms of the same name?! You mean you can have two terms with the same name?! What's a term? How did I end up here, anyway? I think I need some fresh air..."

#70 @husobj
12 years ago

  • Cc ben@… removed

#71 @husobj
12 years ago

  • Cc ben@… added

#72 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.5

Global terms were removed here: #21734. Nacin supported this ticket in dev chat 4 weeks ago. I'll write a Unit Test every day until December if we can make this happen. Moving this to 3.5 until someone says anything definitive about it / objects.

#73 @JustinSainton
11 years ago

+1. This is crazy. More than happy to help with unit tests, debugging, etc. @wonderboymusic, let me know if there's anything you need help on.

#74 @scribu
11 years ago

[UT1029], but still needs more. See #UT125

Last edited 11 years ago by scribu (previous) (diff)

#75 @scribu
11 years ago

Related: #21950

#76 @greenshady
11 years ago

  • Cc justin@… added

#77 @scribu
11 years ago

  • Keywords needs-unit-tests removed

#78 @scribu
11 years ago

Say you have this configuration:

| Taxonomy | Term slug  | Term ID |
-----------------------------------
| A        | orange     | 1       |
| B        | orange     | 1       |
| C        | banana     | 2       |
-----------------------------------

What happens when you try to rename 'orange' to 'banana'?

I'm starting to think nacin's intuition that this should be handled at the same time with #21950 (which will require removing the UNIQUE constraint on wp_terms.slug).

Last edited 11 years ago by scribu (previous) (diff)

#79 follow-up: @ryan
11 years ago

I think this needs to use wp_update_term_count() instead of direct calls to _update_generic_term_count() and _update_post_term_count() so that deferred term counting still works. Deferred counting is more important when creating new terms vs. updating so it might not be a big deal here. Deferred counting is to avoid count update storms during imports.

Last edited 11 years ago by ryan (previous) (diff)

#80 @scribu
11 years ago

False alarm: [UT1048]

It will actually create a term with the title 'Banana', but the slug 'orange-2'.

The bad news is that users probably don't want 'orange-2' as the slug for the 'Banana' term, so it doesn't seem like this would help that much.

The good news is that we don't need to touch the UNIQUE index and don't need to worry about #21950.

Last edited 11 years ago by scribu (previous) (diff)

#81 @scribu
11 years ago

Related: #22023

#82 @nacin
11 years ago

#22153 was marked as a duplicate.

#83 @mojowill
11 years ago

#22226 was marked as a duplicate.

#84 @mojowill
11 years ago

  • Cc will@… added

#85 in reply to: ↑ 79 @nacin
11 years ago

  • Keywords punt added

Replying to ryan:

I think this needs to use wp_update_term_count() instead of direct calls to _update_generic_term_count() and _update_post_term_count() so that deferred term counting still works. Deferred counting is more important when creating new terms vs. updating so it might not be a big deal here. Deferred counting is to avoid count update storms during imports.

This remains important. I sense a sad punt, then reviving this in 3.6.

#86 @nacin
11 years ago

  • Keywords 3.6-early added; punt removed
  • Milestone changed from 3.5 to Future Release
  • Priority changed from normal to high

This is 3.6 territory.

#87 @chriscarson
11 years ago

Hi --

I'm new here, but am an experienced userland WordPress programmer who is conversant with the issue being discussed.

This thread worries me, because it addresses a symptom of the problem (term names and slugs updated willy-nilly across taxonomies,) rather than the fundamental problem (how WordPress handles taxonomy, custom post types and rewrites.)

I've got some ideas on the latter, fundamental problem. Where should I post them? Thanks,

Chris

#88 @scribu
11 years ago

Hello chriscarson,

I think the best place for high-level architecture discussion is the wp-hackers mailing list.

#89 @WraithKenny
11 years ago

  • Cc Ken@… added

#90 @coreygilmore
11 years ago

  • Cc corey@… added

#91 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.6

#92 @aaroncampbell
11 years ago

  • Cc aaroncampbell added

#93 @nicokaiser
11 years ago

  • Cc nicokaiser added

#94 @teauser
11 years ago

  • Cc martrober@… added

#95 @nikolov.tmw
11 years ago

  • Cc ico.the.star.dust@… added

#96 @markoheijnen
11 years ago

Shouldn't this be punted to 3.7-early? Seems as something we want to deal early in a release.

#97 @SergeyBiryukov
11 years ago

  • Keywords 3.7-early added; 3.6-early removed
  • Milestone changed from 3.6 to Future Release

#98 @LucasMS
11 years ago

  • Cc LucasMS added

#99 follow-up: @LucasMS
11 years ago

I noticed that even the wordpress API functions are not dealing correctly with term IDs and category IDS. For exemple, the delete_category Hook will pass the TERM ID to the hooked function, not the term_taxonomy_ID, which should be the correct.

This is very serious, since plugins are storing the TERM IDs as if they were identifying numbers of categories.

#100 in reply to: ↑ 99 @SergeyBiryukov
11 years ago

Replying to LucasMS:

For exemple, the delete_category Hook will pass the TERM ID to the hooked function, not the term_taxonomy_ID, which should be the correct.

Term taxonomy ID is passed as a second argument:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/taxonomy.php#L1838

You'll need to specify the number of arguments in your add_action() call:

add_action( 'delete_category', '...', 10, 3 );

There was, however, an error in the inline docs, fixed in #24343.

#101 follow-up: @aaroncampbell
11 years ago

  • Keywords early added; 3.7-early removed
  • Milestone changed from Future Release to 3.7

#103 @DeanMarkTaylor
11 years ago

  • Cc deanmarktaylor@… added

#102 in reply to: ↑ 101 @nacin
11 years ago

  • Milestone changed from 3.7 to 3.8

Replying to aaroncampbell:

Milestone changed from Future Release to 3.7

Based on http://make.wordpress.org/core/2013/07/28/potential-roadmap-for-taxonomy-meta-and-post-relationships/, I think this is dependent on #22023 which itself is dependent on #17689. As in, 3.8 at the earliest.

#104 @VCcreate
11 years ago

We are absolutely astonished that this problem has persisted for OVER 6 YEARS!!!

For shame WP, for shame :(

#105 @VCcreate
11 years ago

  • Cc VCcreate added

#106 @SergeyBiryukov
10 years ago

#26067 was marked as a duplicate.

#107 @wonderboymusic
10 years ago

  • Milestone changed from 3.8 to Future Release

Gonna drag this in with my bare hands as soon as possible

#108 @leemon
10 years ago

So sad this got punted again

#109 @maorb
10 years ago

  • Cc maor@… added

#110 follow-up: @voilamedia
10 years ago

Want to make sure this issues is related before posting another ticket. I tested this on a fresh install with WordPress 3.8.1.

When you create a custom taxonomy, (post types most likely would do this too but I didn't want to test it) then add terms to the taxonomy as you would. Then add a term of that taxonomy as the name of custom menu that you set up with the new menu manager feature, than add menu items it modifies the term and the menu it modifies both the menu term and the custom taxonomy term

To replicate.

  1. create custom taxonomy called centers with a term called "Western US / Los Angeles" with slug of la
  2. Create a menu called "Western US / Los Angeles"
  3. Add items to the menu and save it.
  4. Go back and check the taxonomy the slug has been updated to "western-us-los-angeles"
  5. The same thing happens if you use a slug or an id as the menu name. It changes the term name of the custom taxonomy to term Id.
  6. Once this happens If you update either the menu or the term of the custom taxonomy term it updates both terms.

After some looking it seems like once you create a term and than create a menu term of the same name,Id, or slug instead of creating a new menu term it updates the existing term.

If this issue is as big as it looks I really don't understand why this has not been resolved yet. This has been issue for years and it can cause a lot of issues. I propose this should be fixed in 3.8.2.

#112 follow-up: @voilamedia
10 years ago

What can I do to make this happen sooner.

#113 in reply to: ↑ 112 @helen
10 years ago

Replying to voilamedia:

What can I do to make this happen sooner.

#17689 and then #22023 need to be resolved first. Believe me, I don't think it's much of an exaggeration if at all when I say that this drives all of us crazy, too. You may also be interested in this potential roadmap.

#114 in reply to: ↑ 110 @MikeSchinkel
10 years ago

Replying to @all in reference to voilamedia's comments:

When you create a custom taxonomy, (post types most likely would do this too but I didn't want to test it) then add terms to the taxonomy as you would. Then add a term of that taxonomy as the name of custom menu that you set up with the new menu manager feature, than add menu items it modifies the term and the menu it modifies both the menu term and the custom taxonomy term

Let me float an idea that I've thought about ever since I tried to use the menu system for in ways that required some significant custom coding. This idea would not at all address the issue of tag names and slugs changing when they were edited for a different context but would solve the menu issue mentioned above:

Let's deprecate the use of Posts and Taxonomies for menus and instead store serialized objects in the Options table.

I've posted another ticket for discussion of that proposal:

#115 @helen
10 years ago

#27010 was marked as a duplicate.

#116 @SergeyBiryukov
10 years ago

#27179 was marked as a duplicate.

#117 @SergeyBiryukov
10 years ago

#27596 was marked as a duplicate.

#118 @helen
10 years ago

#27966 was marked as a duplicate.

#119 @imath
10 years ago

Hi,

Sorry if my patch (5809.03.diff) is too simply, but i thought, if we make sure slugs are different before creating the term, then it can avoid the trouble.

@imath
10 years ago

This ticket was mentioned in IRC in #wordpress-dev by simonwheatley. View the logs.


10 years ago

#121 @rachelbaker
10 years ago

  • Keywords needs-unit-tests added

#122 @rachelbaker
10 years ago

Any thoughts or feedback on imath's patch?

#123 @jkohlbach
10 years ago

Seriously.. how is this still a issue? Such a fundamental bug should have been fixed in the next release, yet we're 6 years on here.

Huge +1 to get this included in the next release. It effects much more than just WordPress core now that CPTs and Custom Taxonomies are commonplace.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

#126 @jpswade
10 years ago

#29131 was marked as a duplicate.

@jesin
10 years ago

Refresh and fix patch terms.diff

#127 follow-up: @jesin
10 years ago

Patch 5809-jesin.diff refreshes wonderboymusic's terms.diff patch and also fixes a Could not insert term into the database error that occurred when using terms.diff.

There already seems to be a unit test test_update_shared_term() in tests/term.php which is annotated with this ticket no.

Is a new test required?

#128 in reply to: ↑ 127 @voilamedia
10 years ago

Replying to jesin:

Thanks for the patch. what can i do to move this along. Since this is my first such ticket how does testing work?

Patch 5809-jesin.diff refreshes wonderboymusic's terms.diff patch and also fixes a Could not insert term into the database error that occurred when using terms.diff.

There already seems to be a unit test test_update_shared_term() in tests/term.php which is annotated with this ticket no.

Is a new test required?

#129 follow-up: @maxwelton
10 years ago

If this is never going to be updated, can we at least prevent the situations which lead to this circle-of-terms, by enforcing unique slugs throughout WP? I hate unique slugs as a requirement (isn't that what IDs are for?), but if it fixes this, unique slugs are way, way preferable to this embarrassing bug. WP is an excellent platform and I really enjoy working with it, but this is one of those things you cannot explain to a client without them pausing for a long time and finally saying "really?"

Interesting: if you create the term "best" in tax 1 and then create "best" in tax 2, they are married forevermore.

But if you create "best" in tax 1 and also create "Best" in tax 1, the latter will be assigned a slug of best-2.

If you go to tax 2 and create "best", you get the shared-term-bug misery. But if you create "Best" in tax 2, it is not shared (slug: best-3). And if you go to tax 3 and create "Best" (slug: best-4) it will not be shared, either. But adding "best" to any taxonomy and you'll be sharing that term across all taxonomies, no matter what.

Le sigh.

#130 in reply to: ↑ 129 @aaroncampbell
10 years ago

Replying to maxwelton:

If this is never going to be updated

It's not that it's never going to be fixed, it's just that the correct fix takes time because it needs to be spread out across several versions. If you look at the potential roadmap you can see the plan all laid out. It's taking longer than we had hoped, but we're making progress. I think the next step is #22023, which was dependent on #17689, which was fixed in 4.0.

#131 @boonebgorges
9 years ago

In 29830:

Improve unit test coverage for wp_insert_term().

See #5809, #22023.

#132 @boonebgorges
9 years ago

In 29875:

Improve unit test coverage for wp_update_term().

See #5809, #22023.

#133 @chriscct7
9 years ago

#19680 was marked as a duplicate.

#134 @boonebgorges
9 years ago

  • Keywords early needs-unit-tests removed

Now that #22023 is resolved, we can finally start moving forward with this ticket. 5809.patch is my proposed patch for 4.1.

A quick recap of the background will be helpful in explaining the patch. #22023 was a blocker for this ticket because splitting existing shared terms into separate terms requires duplicate slugs to appear in the wp_terms table. A tag and a category that share the term 'boone' (cool site, btw) will currently have default archive permalinks example.com/category/boone and example.com/tag/boone. If we want to split the tag and category into separate terms, they both have to have the slug 'boone'. That's why we need to remove the UNIQUE index before splitting existing shared terms.

This is somewhat more complex than it seems, because updates to the database schema don't propagate immediately. On Multisite, the Network Upgrade script needs to be triggered manually, and on large networks like wordpress.com, the upgrade can take weeks. So we have to be delicate about how we go about splitting shared terms: either we wait until we can be reasonably certain that all sites will have performed the necessary schema change (say, 4.2), or we do some checks of the actual db_version before splitting, and act accordingly in either case.

In 5809.patch, I've opted for the latter path. Once the schema change is applied to a given site, there's no reason IMO to wait on cleaning up this mess. As such, my patch does the following:

  • On wp_update_term(), if the schema change *has* been run, detect whether the tt_id shares its term_id with another tt_id. If so, split it off into its own term. If the schema change has *not* been run, do nothing - allow the shared term to stay shared. This solves the current ticket, at least for sites where the database schema change has taken place.
  • On wp_insert_term(), stop creating shared terms. This fixes #21950. If the schema change *has* been run, we allow a duplicate slug to be created. In other words: If you have a tag 'boone', you'll be allowed to create a new category called 'boone', which will create a new item in 'wp_terms'. If the schema has *not* been updated, we cannot allow a duplicate slug to be created; instead, we force a new slug in wp_unique_term_slug(). In other words: If you have a tag 'boone', a category called 'Boone' will get slug 'boone-2'.

It's my view that we should not attempt forcibly splitting existing terms on upgrade until we can be reasonably sure that the 4.1 schema change has taken place. So, 4.2 seems like a reasonable goal for that. Then we can start talking about further steps in the taxonomy roadmap.

  • When

@boonebgorges
9 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


9 years ago

@boonebgorges
9 years ago

#136 @boonebgorges
9 years ago

  • Milestone changed from Future Release to 4.1
  • Owner changed from garyc40 to boonebgorges
  • Status changed from reopened to accepted

5809.2.patch is a refresh of 5809.patch that adds some logic for preventing duplicate term creation. See the lines in the patch that are labeled "Sanity check".

This refreshed patch follows from a discussion I had earlier today with nacin. We talked about the risk, given the lack of UNIQUE index on the wp_terms:slug, that duplicate terms would be incorrectly created in certain circumstances, such as in environments where a master database is replicated to one or more slaves. The INSERT call in wp_insert_term() will always point to the master, but the change may take time to be replicated to the read-only slaves; and during this time, a term_exists() check might point to an out-of-date slave where the new term does not exist, which may allow a duplicate term to be created. The solution he suggested was this: in these replicated environments, a single INSERT statement will trigger all subsequent queries in the request to be performed against the master database. So, we allow wp_insert_term() to create the term; then, knowing that subsequent SELECT statements will hit the master, we check to see whether there's an older item that shares the slug+parent+taxonomy with the term just created. If so, we have a duplicate, and we should use the older term/tt and delete the just-created one. (Phew.)

The idea behind this updated patch is that it should catch the majority of cases where it's possible for duplicate terms to be mistakenly created, which should give us a good deal more confidence that we can go forward with starting the process of solving this ticket during the 4.1 release.

@boonebgorges
9 years ago

#137 @boonebgorges
9 years ago

5809.3.patch refines 2.patch in the following ways:

  • Improve cache busting for shared term_taxonomy_ids when splitting a shared term
  • Remove the parameter format args (%d) in the $wpdb method calls, which nacin reminds me are not necessary when querying core tables </plugin_developer_mea_culpa>
  • When a duplicate term is detected after insert in wp_insert_term(), return immediately rather than continuing with the function. We shouldn't run the term creation hooks in this case.

@boonebgorges
9 years ago

#138 @boonebgorges
9 years ago

5809.4.patch implements a suggestion by markjaquith to further avoid race conditions that may lead to duplicated terms. In 3.patch, there might still be cases - replication aside - where term inserts overlap in such a way that they would see each other as duplicates and unwittingly delete themselves. In 4.patch, the post-INSERT check looks for a term_id and term_taxonomy_id are < those just created, guaranteeing that the first one is always canonical even in cases of overlap.

#139 @boonebgorges
9 years ago

In 30238:

In wp_insert_term(), clean up accidental duplicate terms after insert.

In [30056], the UNIQUE index was removed from the 'slug' column of wp_terms.
While we have numerous checks in place to avoid the creation of unwanted
duplicate term+term_taxonomy pairs, it's possible that in certain edge cases -
such as during the lag caused by database replication, or a race condition
involving near-simultaneous creation of more than one term - we'll end up
unwittingly inserting two identical rows.

The current changeset minimizes this risk by introducing a failsafe mechanism
into wp_insert_term(). After a term and term_taxonomy are INSERTed, we check
to see whether the term just created is a duplicate of an existing term; if so,
we delete the new one and keep the old one. This prevents problems caused by
replication lag, because SELECT queries that take place after an INSERT will
hit the master database; it mitigates race conditions by enforcing that the
term that was created first (ie, the one with the lowest term_id) is
always the "canonical" one.

Props nacin, markjaquith.
See #22023, #5809.

#140 @boonebgorges
9 years ago

In 30239:

Enforce ORDER BY and LIMIT clauses in term_exists() queries.

In case of edge cases where a duplicate term might exist in a replicated
database for a split second, these explicit query clauses ensure that
term_exists() will always recognize the oldest matched term as the
canonical one. See [30238] for background.

Props pento.
See #22023, #5809.

#141 @boonebgorges
9 years ago

In 30240:

Do not create shared taxonomy terms.

A "shared" term occurs when two entries in the wp_term_taxonomy table share a
single term_id, and thereby correspond to the same row in wp_terms. This
changeset stops the practice of creating shared terms: each new row in
wp_term_taxonomy will receive its own row in wp_terms. The new strategy
for term creation depends on whether the installation's database schema is up
to date for 4.1:

  • If so, terms are allowed to be created with the same slug as an existing term, as long as they are in different taxonomies and do not share a parent. Thus, a new tag with the slug 'wordpress' can exist alongside a category with the slug 'wordpress'.
  • If not, new terms will be forced to have unique slugs. Thus, on an installation containing a category with the slug 'wordpress', a new tag 'WordPress' will get the slug 'wordpress-2'.

Fixes #21950. See #5809.

#142 @boonebgorges
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 30241:

Split shared taxonomy terms during term update.

When updating an existing taxonomy term that shares its term_id with
another term, we generate a new row in wp_terms and associate the updated
term_taxonomy_id with the new term. This separates the terms, such that
updating the name of one term does not change the name of any others.

Note that this term splitting only occurs on installations whose database
schemas have been upgraded to version 30133 or higher. Note also that shared
terms are only split when run through wp_update_term(), as on edit-tags.php;
we will wait until a future release of WordPress to force the splitting of all
shared taxonomy terms.

Props boonebgorges, rmccue, greuben, garyc40, wonderboymusic, imath, jesin.
Fixes #5809.

This ticket was mentioned in Slack in #core by justinsainton. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by rmccue. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by mboynes. View the logs.


9 years ago

@boonebgorges
9 years ago

#148 @boonebgorges
9 years ago

  • Keywords 4.2-early added
  • Milestone changed from 4.1 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

After some discussion, we've decided to pull this functionality for 4.1, in order to prepare more documentation about the change. This will help developers to prepare for possible breakage. See #30335.

Term splitting will go into trunk once 4.1 is released, and is slated for 4.2 (for realz). 5809.5.patch

#149 @boonebgorges
9 years ago

In 30585:

Revert shared taxonomy term splitting for 4.1.

This is a revert of [30494], [30492], [30347], and [30334]. The latter
changeset was a revert of [30336], which was a revert of [30241].

Watch for Term Splitting, Version III in version 4.2, coming soon to a
WordPress trunk near you.

See #30335, #5809.

#150 @iseulde
9 years ago

  • Milestone changed from Future Release to 4.2

has-patch 4.2-early so moving to 4.2.

@boonebgorges
9 years ago

Refreshed for 4.2.0. Introduces wp_get_split_terms(), which can be used to retrieve stored data about previously split terms.

#151 follow-up: @boonebgorges
9 years ago

  • Keywords 4.2-early removed

5809.6.patch refreshes the patch for trunk. The term splitting logic is as before: on wp_update_term(), if the term being updated is shared with another term, assign a new term_id to the updated term.

Plugins that store term_ids may experience miscellaneous breakage when term_ids change; see #30335. There are two mechanisms in the patch to ease the transition. (1) Hooking to the 'split_shared_term' action will allow plugins to make the necessary adjustments at the time of splitting. This action was in previous versions of the patch. (2) A new addition to this patch is wp_get_split_terms( $term_id, $taxonomy = '' ). We will store information about previously split terms in an options array, and this new function provides controlled access to the array. In this way, any plugin that needs to go back and see which terms have been split (given the old term_id) can use this function to do necessary cleanup.

mboynes and I have prepared some documentation for this change. The draft can be found at https://gist.github.com/boonebgorges/e873fc9589998f5b07e1. It's written to be a brief and practical how-to, appropriate for a Codex article. I'm imagining that it should be accompanied by a make/core post that provides some more historical context.

We're looking for feedback on the following:

  • wp_get_split_terms() - Does this seem like the right way to expose saved data about split terms?
  • Is the documentation clear enough? We tried to give representative examples that any developer could follow; we did not try to be exhaustive in our explanations.
Last edited 9 years ago by boonebgorges (previous) (diff)

#152 in reply to: ↑ 151 @theMikeD
9 years ago

Replying to boonebgorges:

5809.6.patch refreshes the patch for trunk. The term splitting logic is as before: on wp_update_term(), if the term being updated is shared with another term, assign the updated term with a new ID.

By this do you mean "...if the term being updated is used in another taxonomy"? Or "...if the ID of the term being updated is used by another taxonomy..."? I don't understand how a term can be shared with another term.

Otherwise the docs are clear to me.

This ticket was mentioned in Slack in #core by boone. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


9 years ago

This ticket was mentioned in Slack in #docs by boone. View the logs.


9 years ago

@boonebgorges
9 years ago

Refresh of .6.diff. Splits wp_get_split_term() and wp_get_split_terms() for more predictable return values.

#156 @boonebgorges
9 years ago

In 31418:

Split shared taxonomy terms on term update.

When updating an existing taxonomy term that shares its term_id with
another term, we generate a new row in wp_terms and associate the updated
term_taxonomy_id with the new term. This separates the terms, such that
updating the name of one term does not change the name of any others.

In cases where a plugin or theme stores term IDs in the database, term splitting
can cause backward compatibility issues. The current changeset introduces
two utilities to aid developers with the transition. The 'split_shared_term'
action fires when the split takes place, and should be used to catch changes in
term_id. In cases where 'split_shared_term' cannot be used, the
wp_get_split_term() function gives developers access to data about terms
that have previously been split. Documentation for these functions, with
examples, can be found in the Plugin Developer Handbook. WordPress itself
stores term IDs in this way in two places; _wp_check_split_default_terms()
and _wp_check_split_terms_in_menus() are hooked to 'split_shared_term' to
perform the necessary cleanup.

See [30241] for a previous attempt at the split. It was reverted in [30585]
for 4.1.0.

Props boonebgorges, mboynes.
See #5809.

#157 @boonebgorges
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#159 @JustinSainton
9 years ago

It's done. It's really, truly, done.

you're the bomb.com, @boonebgorges.

This ticket was mentioned in Slack in #forums by kenshino. View the logs.


9 years ago

#161 @DrewAPicture
9 years ago

  • Priority changed from high to normal

This ticket was mentioned in Slack in #core by boone. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by boone. View the logs.


8 years ago

#164 @johnbillion
7 years ago

In 40446:

Taxonomy: Remove a failing test from the 4.0 branch.

This functionality was fixed in [30241] but the test was failing prior to that.

See #5809

Note: See TracTickets for help on using tickets.