WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 9 months ago

Last modified 9 months ago

#43271 closed enhancement (fixed)

Issue with term duplicate check in wp_insert_term

Reported by: strategio Owned by: boonebgorges
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.1
Component: Taxonomy Keywords: needs-patch commit
Focuses: Cc:

Description

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:

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).

Attachments (4)

43271.diff (1.7 KB) - added by boonebgorges 14 months ago.
43271.2.diff (2.1 KB) - added by boonebgorges 13 months ago.
43271.3.diff (580 bytes) - added by audrasjb 9 months ago.
Update @since to 5.0.3
43271.4.diff (580 bytes) - added by audrasjb 9 months ago.
Update @since to 5.1.0

Download all attachments as: .zip

Change History (19)

@boonebgorges
14 months ago

#1 @boonebgorges
14 months ago

  • Keywords has-patch reporter-feedback added

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() :)

#2 @boonebgorges
14 months ago

  • Version changed from 4.9.4 to 4.1

#3 @strategio
14 months ago

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).

#4 @boonebgorges
13 months ago

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?

#5 @strategio
13 months ago

@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.

#6 @boonebgorges
13 months ago

  • Milestone changed from Awaiting Review to 5.0

Ah yes, you are right. See 43271.2.diff

#7 @strategio
13 months ago

It sounds good @boonebgorges :)

#8 @boonebgorges
13 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

#9 @johnbillion
11 months ago

  • Keywords needs-patch added; has-patch reporter-feedback removed
  • Milestone changed from 5.0 to 5.0.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

@since needs updating

#10 @pento
9 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#11 @pento
9 months ago

  • Milestone changed from 5.0.2 to 5.0.3

@audrasjb
9 months ago

Update @since to 5.0.3

#12 @audrasjb
9 months ago

  • Keywords commit added

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.

#13 @boonebgorges
9 months ago

  • Milestone changed from 5.0.3 to 5.1

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

#14 @boonebgorges
9 months ago

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

In 44372:

Update since annotation for wp_insert_term_duplicate_check filter.

Originally added in [43570].

Props audrasjb.
Fixes #43271.

@audrasjb
9 months ago

Update @since to 5.1.0

#15 @audrasjb
9 months ago

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

EDIT: Ah ok, too late. Thanks :)

Last edited 9 months ago by audrasjb (previous) (diff)
Note: See TracTickets for help on using tickets.