term_exists() short-circuits and results in duplicate inserts.

[georgestephanis]

If you are using a term that gets stripped down to an empty string -- like // or >> by sanitize_title() then it will be able to be inserted into the database properly, but any future checks to term_exists() will lie to you and tell you that it doesn't exist.

This is inconsistent behavior, and can cause database errors where keys are duplicated as it tries to insert the same term into the database twice.

Here's some unit tests that demonstrate the current issue. I'm not sure the ideal place to fix it, I suspect it's the conditional in term_exists, but we may want to just modify terms to always reject something that can't produce a slug. Uncertain.

[georgestephanis]

To hopefully save someone some blame tracking, this is due to

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/taxonomy.php?annotate=blame#L1680

which goes back to [8433] and #6593 --

if ( '' === $slug = sanitize_title($term) )
    return 0;

So it always says the term doesn't exist (returning 0) if it sanitizes down to an empty string.

[SergeyBiryukov]

Previously: #17689

[boonebgorges]

Confirmed. Thanks for the report and for the unit tests, georgestephanis.

Your second test is sorta redundant - since wp_set_object_terms() ultimately falls back onto term_exists(), we don't need it.

I'm not sure the ideal place to fix it, I suspect it's the conditional in term_exists, but we may want to just modify terms to always reject something that can't produce a slug.

My thought is that we should ditch the special check for empty slugs altogether. It looks like the only purpose it serves is to save a database query in case a slug has sanitized down to an empty string. But the query is definitely not worth saving if it causes a short circuit like this.

29589.1 is my proposed fix. Note that this will lead to a slight change in behavior: when you pass a slug that sanitizes to an empty string, the function returns null rather than 0. Checking this kind of slug is already an edge case; the possibility that someone is doing a strictly typed check of this return value is an even bigger edge case. Plus, returning null is in keeping with the behavior of the rest of cases when terms are not found. So I think the change is a good one. I've updated the relevant existing unit test (test_term_exists_term_trimmed_to_empty_string()) to reflect the change. I've also updated the docblock, which was incorrect to begin with.

[boonebgorges]

In 29865:

Don't bail out of term_exists() when term sanitizes to an empty string.

This change brings return values for failures of this sort in line with other
failed lookups in term_exists(): a null value is now returned in all cases
where the queried term is not found.

Adds unit test for the sanitization issue. Modifies existing unit test to
reflect the change in return value for empty term strings.

Props boonebgorges, georgestephanis.
Fixes #29589.