WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#29255 closed defect (bug) (worksforme)

wp_insert_term() calls term_exists() with 'slug' where 'slug' gets compared to term 'name'

Reported by: Rob Walker Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.9.2
Component: Taxonomy Keywords: close
Focuses: Cc:

Description

This may not specifically be a bug, but the resulting behavior is not what is expected.

It appears that a new term slug gets checked against existing term names when wp_insert_term() calls term_exists($slug) on line 2430 of taxonomy.php.

term_exists(), line 1645 of taxonomy.php, processes this slug as though it were the term name when checking against the db.

The documentation reads:

"If 'slug' argument exists then the slug will be checked to see if it is not a valid term. If that check succeeds (it is not a valid term), then it is added and the term id is given."

Is that supposed to mean it checks slug against slug?

Scenario:

I have a taxonomy where the term names need to support duplicate strings therefore I am using the slugs as unique identifiers. To prevent collisions when inserting new terms I compare slugs and not names.

Specifics (an example of how this issue came to light):

I am working a taxonomy of student_names where the term name = the student's first name. The student's family name is handled in a separate taxonomy and is not appropriate to use as a unique identifier (the family name needs to be associated with the parents and possibly other students).

When a new student is added, I generate the slug and ensure that it will be unique in the db.

Normally the first student with a given name, say Craig, will be given the default slug "craig". I call wp_insert_term("Craig", "student_taxonomy", array($slug=>"craig")). It is the first term "Craig", wp_insert_term() finds no collision. Execution completes successfully.

Adding an second student named Craig will result in my custom methods detecting the collision and generating a new slug "craig1". I call wp_insert_term("Craig", "student_taxomony", array($slug=>"craig1")). wp_insert_term() calls term_exists() with the slug "craig1", line 2430. term_exists checks "craig1" against existing term names (Craig), finds no conflict, execution completes successfully.

A third Craig insertion generates the slug "craig2" which is compared against the existing term names (Craig, Craig), finds no conflict, execution completes successfully.

Now, if the first Craig with the slug "craig" is deleted, and a fourth Craig is added, my code will generate the slug "craig" because there is no longer a conflict with that slug. Eventually wp_insert_term compares the slug "craig" to the term names "Craig, Craig" and finds a conflict.

Attachments (2)

29255.test.diff (980 bytes) - added by boonebgorges 4 years ago.
29255.test.2.diff (1.1 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (12)

#1 @boonebgorges
5 years ago

  • Keywords good-first-bug needs-unit-tests added

Thanks for the report. This does sound like a bug, though I'm not sure how best to solve it. We can't stop checking against the 'name' column in term_exists() for backward compatibility reasons. I wonder if we might introduce another parameter that allows you to specify which columns you're checking against, and then in wp_insert_term(), if a 'slug' has been passed, we can tell term_exists() only to check the slug column.

Whatever the solution, we'll need to have a unit test that demonstrates the problem.

#2 @MikeNGarrett
5 years ago

This seems to have been fixed in trunk by forcing wp_unique_term_slug to run. This is an issue for 3.9.2 and 4.0, but should be fixed in 4.1.

#3 @boonebgorges
4 years ago

Thanks for having a look, MikeNGarrett - but I think you're wrong about this. The term_exists() check is still being run, and if it matches based on 'name' alone, then a WP_Error will be thrown. In 4.0, see https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/taxonomy.php#L2481, especially line 2500; in 4.1, see https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/taxonomy.php#L2826. In 4.1, we never reach wp_unique_term_slug() in the case described by Rob Walker.

29255.test.diff is an attempt to write a unit test that describes the bug in as narrow a fashion as possible. It fails on 4.1 and 4.0.

The purpose of this section of wp_insert_term() is to ensure that in a hierarchical taxonomy, terms at the same level of the hierarchy do not share the same 'name'. So the term_exists() check should probably be swapped out with get_term_by( 'name' ), though it'll probably need more massaging than just this to make it work properly.

#4 @boonebgorges
4 years ago

  • Keywords needs-patch added; good-first-bug needs-unit-tests removed
  • Milestone changed from Awaiting Review to Future Release

#5 @MikeNGarrett
4 years ago

Hey Boone,
You're right. I ran back through my tests and realized I was looking too narrowly at the test case. Is the ideal to allow for Name(slug) of Craig(craig), Craig(craig1), Craig(craig2), etc to exist or to allow only one Craig(craig) to exist per level in a hierarchical taxonomy?

I mean, should users be allowed to create a term with the name "Craig" if one already exists regardless of slug? or should slug always be unique, but name can be anything? Either way implies a bit of difficulty in this situation.

#6 @MikeNGarrett
4 years ago

Just making a note. This may have been fixed with ticket #30780. We should retest this scenario with the latest from trunk.

#7 follow-up: @MikeNGarrett
4 years ago

  • Keywords close added; needs-patch removed
  • Resolution set to worksforme
  • Status changed from new to closed

Tested on 4.2. Term splitting solved this ticket.

#8 in reply to: ↑ 7 @boonebgorges
4 years ago

Replying to MikeNGarrett:

Tested on 4.2. Term splitting solved this ticket.

Confirmed. See 29255.test.2.diff for a cleanly applying patch. Thanks for following up.

#9 @MikeNGarrett
4 years ago

Thanks, Boone. I appreciate the insight into the patch.

#10 @DrewAPicture
4 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.