WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29589 closed defect (bug) (fixed)

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

Reported by: georgestephanis Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Taxonomy Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

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.

Attachments (2)

29589.unit-tests.diff (1.9 KB) - added by georgestephanis 6 years ago.
29589.patch (2.2 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (8)

#1 @georgestephanis
6 years ago

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.

#3 @wonderboymusic
6 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Owner set to boonebgorges
  • Status changed from new to assigned

#4 @boonebgorges
6 years ago

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

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
6 years ago

#5 @boonebgorges
6 years ago

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

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.

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


6 years ago

Note: See TracTickets for help on using tickets.