WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 weeks ago

#46431 closed defect (bug) (fixed)

wp_unique_term_slug() doesn't guarantee unique slugs for child terms

Reported by: saskak Owned by: boonebgorges
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.3
Component: Taxonomy Keywords: has-patch has-unit-tests
Focuses: Cc:
PR Number:

Description

Steps to reproduce:

  1. Create a fresh site with WP 5.1 and Twenty Nineteen
  2. Create a category "Animal" with slug "animal"
  3. Create a category "Dog" with slug "dog"
  4. Create a child category for "Animal" named "Cat" with slug "dog-animal"
  5. Create a top-level category "Giraffe" with slug "giraffe"
  6. Edit the "Giraffe" category so that in a single update, the parent changes from "None" to "Animal" and the slug becomes "dog".

What I expected?
The slug of category "Giraffe" should have become "dog-animal-2".

What did I get?
Category "Giraffe" now has identical slug with "Cat", both having the exact same slug "dog-animal".

---

I tracked this down to wp_unique_term_slug(). It seems that if a parent suffix is added to a term, no further checks are made to ensure its uniqueness. The whole section where a number is appended is skipped.

Why those exact steps are needed to reproduce this has to do with differences in wp_insert_term() and wp_update_term() I won't go into detail on that as it's a completely separate issue and seemingly a huge mess...

Attachments (1)

46431.diff (2.8 KB) - added by boonebgorges 2 months ago.

Download all attachments as: .zip

Change History (7)

#1 @dlh
7 months ago

  • Keywords needs-patch needs-unit-tests added
  • Version changed from 5.1 to 4.3

Hi @saskak, and welcome to WordPress Trac!

It looks to me like this behavior was introduced in [32837] and that the cause is as you describe.

#2 @yashar_hv
4 months ago

I encountered this bug when I was trying to auto assign custom taxonomy term slugs by using pre_{$taxonomy}_{$field} filter hook.
I found out in the last part of wp_unique_term_slug() in taxonomy.php, when it tries to append sequel numbers in case $slug is not unique yet, it checks for $parent_suffix ('-parent-slug') and if it exists, the function ignores appending a number and returns $slug.$parent_suffix :

if ( apply_filters( 'wp_unique_term_slug_is_bad_slug', $needs_suffix, $slug, $term ) ) {
	if ( $parent_suffix ) {
		$slug .= $parent_suffix;
	} else {
		if ( ! empty( $term->term_id ) ) {
			$query = $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s AND term_id != %d", $slug, $term->term_id );
		} else {
			$query = $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s", $slug );
		}

		if ( $wpdb->get_var( $query ) ) {
			$num = 2;
			do {
				$alt_slug = $slug . "-$num";
				$num++;
				$slug_check = $wpdb->get_var( $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s", $alt_slug ) );
			} while ( $slug_check );
			$slug = $alt_slug;
		}
	}
}

Possible Solution

I modified this code as below and it seems that this solves the problem and now the number appends to both $slug.$parent_suffix and $slug in case there is a existing term with that slug:

if ( apply_filters( 'wp_unique_term_slug_is_bad_slug', $needs_suffix, $slug, $term ) ) {
	
	/*
	* MODIFIED: $parent_suffix conditional if block is closed right after appending $parent_suffix to $slug.
	*           Else block content for this condition moved to the outside of conditional block.
	*/ 

	if ( $parent_suffix ) {
		$slug .= $parent_suffix;
	}

	if ( ! empty( $term->term_id ) ) {
		$query = $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s AND term_id != %d", $slug, $term->term_id );
	} else {
		$query = $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s", $slug );
	}

	if ( $wpdb->get_var( $query ) ) {
		$num = 2;
		do {
			$alt_slug = $slug . "-$num";
			$num++;
			$slug_check = $wpdb->get_var( $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s", $alt_slug ) );
		} while ( $slug_check );
		$slug = $alt_slug;
	}
}
Last edited 4 months ago by yashar_hv (previous) (diff)

@boonebgorges
2 months ago

#3 @boonebgorges
2 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
  • Milestone changed from Awaiting Review to 5.3

Thanks all for the detailed report.

I've written a test that confirms what @saskak describes above (minus the additional complication of wp_update_term()), and the test confirms that the suggested fix from @yashar_hv works. Could I get another set of eyes on 46431.diff ? @dlh since you dropped by recently? :)

#4 @dlh
2 months ago

46431.diff looks good to me, with a couple potential copy-edits:

  • Switch the test annotation from @group to @ticket.
  • Drop be_ from the test name: test_duplicate_parent_suffixed_slug_should_get_numeric_suffix.

#5 @boonebgorges
8 weeks ago

@dlh Thanks as always for your careful eye! Can you tell that I rewrote the name of that test several times?

#6 @boonebgorges
8 weeks ago

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

In 45893:

Taxonomy: Fix unique-slug check for terms with parents.

wp_unique_term_slug() appends numeric suffixes when the requested slug is
already in use by a sibling term. Changes introduced in [32837] inadvertently
caused this suffixing to be skipped in cases where the requested slug is
suffixed with the parent slug, so that it became possible to have two terms
childslug-parentslug underneath to the same parentslug. We fix this
regression by ensuring that the numeric-suffix routine runs in all cases.

Props yashar_hv, saskak, dlh.
Fixes #46431.

Note: See TracTickets for help on using tickets.