Opened 2 years ago
Closed 38 hours ago
#22023 closed enhancement (fixed)
Remove UNIQUE for slug in wp_terms
Reported by: | nacin | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 4.1 | Priority: | high |
Severity: | normal | Version: | 2.3 |
Component: | Taxonomy | Keywords: | has-patch 4.1-early |
Focuses: | Cc: |
Description
To set us up for future changes to the taxonomy API, we should remove the UNIQUE key for 'slug' for wp_terms. Said future changes include:
- Splitting shared terms on update (#5809)
- Stop creating shared terms (#21950)
- Forcibly split remaining shared terms
- Merge wp_terms and wp_term_taxonomy (I can dream, right?)
The term_exists() check should prevent duplicate terms from ever being inserted (before, of course, we fix #5809 and #21950). But, this needs unit tests, particularly because there is a case where term_exists() breaks. See #17689, which blocks this ticket.
Attachments (3)
Change History (52)
comment:2 hsatterwhite — 2 years ago
- Cc whsatterwhite@… added
comment:7 goblindegook — 2 years ago
- Cc goblindegook added
comment:10 intoxstudio — 2 years ago
- Cc jv@… added
comment:11 wonderboymusic — 2 years ago
- Keywords needs-patch reporter-feedback added
Is this part of 3.5 or 3.6?
comment:12 scribu — 2 years ago
- Keywords reporter-feedback removed
- Milestone changed from 3.5 to Future Release
- Type changed from defect (bug) to enhancement
No unit tests; #17689 not fixed yet. Punting.
comment:13 nacin — 2 years ago
Yeah, I thought this was punted a while ago.
comment:14 aaroncampbell — 22 months ago
- Cc aaroncampbell added
comment:15 aaroncampbell — 22 months ago
- Milestone changed from Future Release to 3.6
comment:16 adamsilverstein — 22 months ago
- Cc ADAMSILVERSTEIN@… added
comment:17 sc0ttkclark — 22 months ago
- Cc lol@… added
comment:18 tomdxw — 22 months ago
- Cc tom@… added
comment:19 tomdxw — 22 months ago
- Cc tom@… removed
comment:20 Viper007Bond — 20 months ago
- Cc Viper007Bond added
comment:21 greenshady — 20 months ago
- Cc justin@… added
comment:22 ryan — 18 months ago
- Milestone changed from 3.6 to Future Release
comment:23 nacin — 15 months ago
- Milestone changed from Future Release to 3.7
Fixing #5809 requires this to be fixed to get the ball rolling for http://make.wordpress.org/core/2013/07/28/potential-roadmap-for-taxonomy-meta-and-post-relationships/. #17689 must be fixed first.
comment:24 nacin — 13 months ago
- Milestone changed from 3.7 to 3.8
We can do #5809 and this in the same release.
hotchkissconsulting — 12 months ago
comment:25 hotchkissconsulting — 11 months ago
- Keywords has-patch added; needs-patch removed
There's a patch now. A teeny, tiny one.
comment:26 wonderboymusic — 11 months ago
- Keywords 3.9-early added
- Milestone changed from 3.8 to Future Release
comment:27 maorb — 10 months ago
- Cc maor@… added
comment:28 travisnorthcutt — 6 months ago
- Keywords 4.0-early added; 3.9-early removed
Just a hair late for 3.9-early, now ;-)
comment:29 ircbot — 5 months ago
This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.
comment:30 helen — 5 months ago
- Milestone changed from Future Release to 4.0
#17689 is in, let's do this pronto.
comment:31 follow-up: ↓ 33 ircbot — 5 months ago
This ticket was mentioned in IRC in #wordpress-dev by simonwheatley. View the logs.
comment:32 simonwheatley — 5 months ago
I'm not clear on the testing requirement here; there are tests newly added by #17689 (test_duplicate_name) which indirectly exercise term_exists, but I'm happy to write more direct testing if that's required.
comment:33 in reply to: ↑ 31 simonwheatley — 5 months ago
Replying to ircbot:
This ticket was mentioned in IRC in #wordpress-dev by simonwheatley. View the logs.
@nacin – "I don't think the current API in any way allows for multiple duplicate term slugs to sneak in the database, except possibly by race condition. which I am concerned about."
I've been thinking about Nacin's concerns above, and AFAIK it's not possible to have a UNIQUE constraint across the wp_term and wp_term_taxonomy tables. If this (race conditions allowing duplicate terms) is a big enough concern to block this ticket, and we need to block the possibility of duplicate terms with a constraint at the database level, then one alternative is to lose the wp_terms table (as mentioned in the potential taxonomy roadmap) so we can have a UNIQUE constraint in the restructured wp_term_taxonomy on taxonomy and slug (and name, perhaps). This is obviously (somewhat understating things) a big change… but we're considering some other structure changes, so may be a good time to get it in? I imagine we'd also want to have time and resource to test both core and existing plugins?
comment:34 follow-up: ↓ 37 simonwheatley — 5 months ago
Additional tests for term_exists:
- Don't use rand_str in test_term_exists_known or test_term_exists_unknown, instead specify a term name (allows for debugging tests more easily if the term name is in DB already)
- Check optional $taxonomy parameter
- Check optional `$parent parameter
- Incidentally check inserting a term with a parent
- Check specifying $term as integer other than 0 (in this case 99999)
- No need to assert the term deletion (we're testing term deletion functionality elsewhere)
comment:35 ircbot — 4 months ago
This ticket was mentioned in IRC in #wordpress-dev by simonwheatley. View the logs.
comment:36 ircbot — 4 months ago
This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.
comment:37 in reply to: ↑ 34 ericmann — 4 months ago
Replying to simonwheatley:
Those additional tests look good, and I'm definitely a fan of removing uses of rand_str (actually seen that cause collisions in rare cases).
Looking forward, let's put together a list of further areas that need testing. No one needs to actually write the tests yet, just pull together a list of what we need to test so we can divvy up work and get started. Thoughts?
comment:38 johnbillion — 4 months ago
- Keywords 4.1-early added; 4.0-early removed
- Milestone changed from 4.0 to Future Release
- Priority changed from normal to high
Let's do this in 4.1 early so we can get lots of nice tests in place and testing done.
comment:39 aaroncampbell — 6 weeks ago
- Milestone changed from Future Release to 4.1
boonebgorges — 4 weeks ago
comment:40 boonebgorges — 4 weeks ago
term_exists-tests.02.patch is an exhaustive set of tests for term_exists(), as well as some tests that show the duplicate-term failure conditions for wp_insert_term().
comment:41 nacin — 4 weeks ago
These tests look great.
comment:42 boonebgorges — 4 weeks ago
In 29798:
comment:43 boonebgorges — 4 weeks ago
The tests committed in r29798 are all passing. As long as all term creation passes through wp_insert_term() and uses term_exists(), I'm pretty confident that we're close to being ready to remove the UNIQUE key.
comment:44 ircbot — 4 weeks ago
This ticket was mentioned in IRC in #wordpress-dev by johnbillion. View the logs.
comment:45 boonebgorges — 4 weeks ago
See #29589, an edge case where term_exists() fails.
comment:46 boonebgorges — 4 weeks ago
In 29830:
comment:47 boonebgorges — 3 weeks ago
In 29875:
comment:48 boonebgorges — 38 hours ago
- Owner set to boonebgorges
- Status changed from new to accepted
YOLO
comment:49 boonebgorges — 38 hours ago
- Resolution set to fixed
- Status changed from accepted to closed
In 30056:
For the version 3.5 we would need:
Just checking so we can delivery something on 3.5