Remove UNIQUE for slug in wp_terms

[nacin]

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.

[webord]

For the version 3.5 we would need:

• Remove the UNIQUE from slug on wp_terms
• Improvments on term_exists()

Just checking so we can delivery something on 3.5

[wonderboymusic]

Is this part of 3.5 or 3.6?

[scribu]

No unit tests; #17689 not fixed yet. Punting.

[nacin]

Yeah, I thought this was punted a while ago.

[nacin]

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.

[nacin]

We can do #5809 and this in the same release.

[hotchkissconsulting]

There's a patch now. A teeny, tiny one.

[travisnorthcutt]

Just a hair late for 3.9-early, now ;-)

[helen]

#17689 is in, let's do this pronto.

[simonwheatley]

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.

[simonwheatley]

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?

[simonwheatley]

Beef up tests for term_exists

[simonwheatley]

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)

[ericmann]

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?

[johnbillion]

Let's do this in 4.1 early so we can get lots of nice tests in place and testing done.

[boonebgorges]

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().

[nacin]

These tests look great.

[boonebgorges]

In 29798:

Improve unit test coverage for duplicate term creation.

These include an exhaustive set of tests for term_exists(), as well as tests
for wp_insert_term() that demonstrate failure when attempting to create a
duplicate term.

Props simonwheatley for an initial patch.

See #22023.

[boonebgorges]

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.

[boonebgorges]

See #29589, an edge case where term_exists() fails.

[boonebgorges]

In 29830:

Improve unit test coverage for wp_insert_term().

See #5809, #22023.

[boonebgorges]

In 29875:

Improve unit test coverage for wp_update_term().

See #5809, #22023.

[boonebgorges]

YOLO

[boonebgorges]

In 30056:

Remove UNIQUE key from 'slug' column of terms table.

Each slug is a unique and beautiful snowflake, but let's enforce that
uniqueness elsewhere.

Props hotchkissconsulting.
Fixes #22023.

[boonebgorges]

I neglected to write the proper upgrade routine for existing installs to remove the UNIQUE index. Reopening.

[boonebgorges]

In 30121:

Bump db_version and add upgrade routine for schema change in [30056].

Fixes #22023.

[ocean90]

Not sure why, but I got 22023.db-error.png​.

[boonebgorges]

In 30134:

Fix database schema update introduced in [30121].

The index must be manually dropped before dbDelta() can add the new index
without throwing a notice.

Fixes #22023.

[boonebgorges]

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.

[boonebgorges]

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.