Issue with term duplicate check in wp_insert_term

[strategio]

In WPML, we assume that a term can be duplicated in the same taxonomy in a secondary language. This means that it will have the same 'slug' as the one in the primary language.

Here's a practical example of 2 term links from the same taxonomy with the same slug:

• ​http://example.org/city/paris/
• ​http://example.org/fr/city/paris/

In wp_insert_term we have several checks to ensure that a term is unique in its taxonomy. All these checks can be overpassed with the standard WP API except the last one which makes a direct call to the database:

/*
 * Sanity check: if we just created a term with the same parent + taxonomy + slug but a higher term_id than
 * an existing term, then we have unwittingly created a duplicate term. Delete the dupe, and use the term_id
 * and term_taxonomy_id of the older term instead. Then return out of the function so that the "create" hooks
 * are not fired.
 */
$duplicate_term = $wpdb->get_row( $wpdb->prepare( "SELECT t.term_id, tt.term_taxonomy_id FROM $wpdb->terms t INNER JOIN $wpdb->term_taxonomy tt ON ( tt.term_id = t.term_id ) WHERE t.slug = %s AND tt.parent = %d AND tt.taxonomy = %s AND t.term_id < %d AND tt.term_taxonomy_id != %d", $slug, $parent, $taxonomy, $term_id, $tt_id ) );
if ( $duplicate_term ) {
	$wpdb->delete( $wpdb->terms, array( 'term_id' => $term_id ) );
	$wpdb->delete( $wpdb->term_taxonomy, array( 'term_taxonomy_id' => $tt_id ) );

	$term_id = (int) $duplicate_term->term_id;
	$tt_id   = (int) $duplicate_term->term_taxonomy_id;

	clean_term_cache( $term_id, $taxonomy );
	return array( 'term_id' => $term_id, 'term_taxonomy_id' => $tt_id );
}

The above data validation is only performed in wp_insert_term and not in wp_update_term which makes the data validation inconsistent between the two function. This also allowed us to demonstrate that two terms with the same slug can live together in the same taxonomy.

As a suggestion, this last check could be either:

• removed: since we already have several checks before and it's not performed when a term is updated.
• filterable: by using get_term_by to fetch the potential existing term, or having a specific filter hook (e.g. wp_bypass_term_duplication_check).

[boonebgorges]

Hi @strategio - Thanks very much for your clear description of the issue.

The check you've cited was introduced in [30238] as part of a schema change in wp_terms. You can read the gory details in the commit message and the linked ticket. Long story short, it will be difficult to simply remove the check. For the same reason, it may not be advisable to switch over to get_term_by(), because that function interacts with the cache in a way that might introduce the kinds of data-integrity problems that the direct database-query is meant to avoid.

A specific filter is here should be doable. See 43271.diff​. Does this look reasonable to you? Note that the use of this filter will put WPML on the hook for guarding against the kinds of replication-related data-integrity issues that [30238] was meant to guard against.

The above data validation is only performed in wp_insert_term and not in wp_update_term which makes the data validation inconsistent between the two function. This also allowed us to demonstrate that two terms with the same slug can live together in the same taxonomy.

By this, I assume you mean that you are using a filter on the return value of get_term_by() or wp_unique_term_slug() here https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/taxonomy.php#L2734, and by "*can* live together" you mean "our plugin triggers duplicate slugs within a taxonomy and WP continues to work" - correct? Or, alternatively, do you mean that it's possible, using wp_update_term() *alone* (with no filters), to update a term so that it shares a slug with a sibling from the same taxonomy? The latter would be a problem. The former seems OK, and if I understand correctly, then 43271.diff​ will let you do the same thing in the case of wp_insert_term() :)

[strategio]

Thanks for the answer @boonebgorges!

A filter would be perfectly fine in my opinion and I already gave it a try (successfully). I would just ask to replace the parameter $term with $slug in the filter. Indeed, we search duplicates with the same parent + taxonomy + slug (but the term name can still be duplicated).

On WPML side, we already have some code to check if a taxonomy slug is unique in its language, so we will reuse it in the new filter (like below for example):

/**
 * ??? filter parameters to be confirmed.
 *
 * @param stdClass $duplicate_term Duplicate term row from terms table, if found.
 * @param string   $slug           Slug being inserted.
 * @param string   $taxonomy       Taxonomy name.
 *
 * @return stdClass|null
 */
public function wp_insert_term_duplicate_term_check( $duplicate_term, $slug, $taxonomy ) {
        if ( $duplicate_term ) {

                $lang = $this->lang_term_slug_save( $taxonomy );

                if ( ! WPML_Terms_Translations::term_slug_exists( $slug, $taxonomy, $lang ) ) {
                        $duplicate_term = null;
                }
        }

        return $duplicate_term;
}

To reply to your other questions:

By this, I assume you mean that you are using a filter on the return value of get_term_by() or wp_unique_term_slug()

=> We currently filter pre_term_slug but we'll change it to wp_unique_term_slug (I think our code existed before wp_unique_term_slug was available).

by "*can* live together" you mean "our plugin triggers duplicate slugs within a taxonomy and WP continues to work" - correct?

=> Exactly.

Or, alternatively, do you mean that it's possible, using wp_update_term() *alone* (with no filters), to update a term so that it shares a slug with a sibling from the same taxonomy?

=> No, this not possible "alone". It works for us because we have a filter on pre_term_slug (which will be changed to wp_unique_term_slug).

[boonebgorges]

Thanks for reviewing and confirming, @strategio.

I would just ask to replace the parameter $term with $slug in the filter. Indeed, we search duplicates with the same parent + taxonomy + slug (but the term name can still be duplicated).

$duplicate_term should contain all the information you need: slug, taxonomy, name, etc. I've chosen to pass $term, $taxonomy, and $args to the filter as well, because these are the original values that were passed to wp_insert_term(), and they may differ from those used in the $duplicate_term check. Can you confirm this reasoning?

[strategio]

@boonebgorges, I understand how you defined the parameters now, and indeed it makes sense.

However, $duplicate_term has just 2 properties: term_id and term_taxonomy_id.

See the query => $duplicate_term = $wpdb->get_row( $wpdb->prepare( "SELECT t.term_id, tt.term_taxonomy_id FROM ...

So you will have to adjust it to:

$duplicate_term = $wpdb->get_row( $wpdb->prepare( "SELECT t.term_id, tt.term_taxonomy_id, t.slug FROM $wpdb->terms t INNER JOIN $wpdb->term_taxonomy tt ON ( tt.term_id = t.term_id ) WHERE t.slug = %s AND tt.parent = %d AND tt.taxonomy = %s AND t.term_id < %d AND tt.term_taxonomy_id != %d", $slug, $parent, $taxonomy, $term_id, $tt_id ) );

I am not sure it's necessary to add more properties for now, but I need the slug one to avoid an extra query.

[boonebgorges]

Ah yes, you are right. See 43271.2.diff​

[strategio]

It sounds good @boonebgorges :)

[boonebgorges]

In 43570:

Introduce wp_insert_term_duplicate_term_check filter.

This filter allows plugins to intervene in the duplicate-term check
that takes place at the time of term creation. See [30238], #22023.

Props strategio.
Fixes #43271.

[johnbillion]

@since needs updating

[audrasjb]

Update @since to 5.0.3

[audrasjb]

This ticket is triaged in milestone 5.0.3. 43271.3.diff is a refreshed patch to include relevant @since tag. Now it will needs commit and backport to the related branch.

[boonebgorges]

Thanks, @audrasjb. As we're now returning to a normal major/minor release schedule, this ticket is more appropriate for 5.1.0.

[boonebgorges]

In 44372:

Update since annotation for wp_insert_term_duplicate_check filter.

Originally added in [43570].

Props audrasjb.
Fixes #43271.

[audrasjb]

Update @since to 5.1.0

[audrasjb]

Makes sense @boonebgorges I just updated @since to 5.1.0.

EDIT: Ah ok, too late. Thanks :)