wp_insert_term can return wrong ID when multiple existing terms are found with the same name but differnet parent

[mikejolley]

First reported here as an API issue ​https://github.com/woocommerce/woocommerce/issues/13386, it seems the problem is within the wp_insert_term logic.

To reproduce:

Create 2 terms with the same name, but different parents e.g.

• Top Level 1
  • Child
• Top Level 2
  • Child

You'll have 2 terms with name Child and slugs child and child-top-level-2.

Now try to add the same named Child term again to the second top level parent (in my case, Top Level 2).

var_dump( wp_insert_term( 'Child', 'category', array( 'parent' => 1158 ) ) );

The error message wil contain the term ID underneath the first top level parent (1159) instead of the correct one term under the chosen parent (1158):

object(WP_Error)#1270 (2) {
  ["errors"]=>
  array(1) {
    ["term_exists"]=>
    array(1) {
      [0]=>
      string(62) "A term with the name provided already exists with this parent."
    }
  }
  ["error_data"]=>
  array(1) {
    ["term_exists"]=>
    int(1159)
  }
}

I've traced this back to the $name_matches code inside the wp_insert_term function. This code does not use the parent right now.

Patch to follow.

[mikejolley]

Fix for issue 39984

[mikejolley]

Added a potential patch, but it needs some more eyes and testing. It does 2 things;

1. Ensures only terms from the specified parent are found to check names.
2. It forces the name check if no slug was provided by the user. This is because the slug (if multiple terms with same name exists) will already have a suffix.

[boonebgorges]

Thanks for the detailed report and the patch, @mikejolley. I've attached a second patch that includes a test demonstrating the problem and the fix.

Regarding your point 2 - I would like to talk through what's happening here, since it seems like a change in behavior. I believe that your patch for 1 has turned up a bug, along the following lines:

• It's possible for there to be more than one member of $name_matches, either because of accented characters (see the following block) or because of hierarchy branches like the one described in this ticket.
• When no slug is provided to wp_insert_term() and there's a name match (in the relevant part of the taxonomy), we generally want to return an error - this is one of the legacy error conditions of wp_insert_term()
• It's usually the case that the first member of the $name_matches array will have the non-prefixed slug. That is: under normal circumstances (entering category names from the Dashboard), WP is responsible for generating slugs; so the first 'Child' will get the slug child, the second will get child-foo, etc. Since the default sort order of get_terms() is by ID, child will practically always be first.
• This means that the $name_match->slug === $slug condition on line 2066 is usually met. But it's sorta coincidental that this works: if you first created 'Child' with the manual slug child-foo and then created a second 'Child' with child, you'd surface the bug, independently of the hierarchy issue described in this ticket.

Does that sound right to you? If so, there are really two bugs to fix here.

[mikejolley]

Yeah that sounds about right. It's hard to explain this one. I think it's important we allow multiple terms with same names under different parents. I'm not sure how we'd fix this in our API if it were disallowed :)

[boonebgorges]

Thanks, @mikejolley.

I think it's important we allow multiple terms with same names under different parents.

Yes, WP has always allowed this. This particular weird bug was introduced as part of a refactor in support of multibyte slugs. See https://core.trac.wordpress.org/ticket/31328#comment:9 and follow-up conversation, especially [31792]. (Prior to WP 4.2, this specific kind of bug didn't occur because we used term_exists() instead of get_term_by( 'name' ); term_exists() would fall back on a slug check.)

[boonebgorges]

In 40144:

Taxonomy: Improve precision of duplicate name checks when inserting terms.

wp_insert_term() does not allow for terms with the same name to exist
at the same hierarchy level, unless the second term has a unique slug.
When this logic was refactored in [31792] and [34809], a bug was
introduced whereby it was possible to bypass the no-same-named-sibling
check in cases where the first term had a non-auto-generated slug
(ie, where the name was 'Foo' but the slug something other than 'foo',
such that the second term would get the non-matching slug 'foo').

This changeset fixes this issue by ensuring that the duplicate name
check runs both in cases where there's an actual slug clash *and* in
cases where no explicit slug has been provided to wp_insert_term().
The result is a more reliable error condition:
wp_insert_term( 'Foo' ... ) will always fail if there's a sibling
'Foo', regardless of the sibling's slug.

Props mikejolley.
See #39984.

[boonebgorges]

In 40145:

Taxonomy: Take 'parent' into account when checking for terms with duplicate names.

Terms with duplicate names are not allowed at the same level of a
taxonomy hierarchy. The name lookup introduced in [34809] did not
properly account for the 'parent' parameter, with the result that
the duplicate-name restriction was tighter than intended (terms
with duplicate names could not be created at different levels of
a single hierarchy).

Props mikejolley.
Fixes #39984.