Inconsistent behavior/results of wp_insert_term and wp_update_term

[ipm-frommen]

In WordPress 4.1, I noticed the following behavior (in my opinion misbehavior):

• I can create a new category Ping (slug ping);
• I can create a new tag Ping (slug ping).

This means, I have two terms (one DB entry each) that have the same slug, each in one of the two mentioned taxonomies.

With the above setup, I continue like so:

• I can change the slug of the category Ping to pong;
• I can not change the slug back to ping (The slug "ping" is already in use by another term).

This is true for both QuickEdit and Edit.

I hope this is not by-design!?

In function wp_insert_post, there is a check for the existence of the new slug - in the very taxonomy. In function wp_update_term, however, there is a check for the slug only (no taxonomy included in the query/check).

Please see attached path. It just uses the check from wp_insert_term in wp_update_term.

[boonebgorges]

ipm-frommen - Thanks for the report.

This means, I have two terms (one DB entry each) that have the same slug, each in one of the two mentioned taxonomies.

The ability to create terms with duplicate slugs in separate taxonomies is new to WP 4.1. See #21950 [30240]. Due to an oversight, the ability to have terms with duplicate slugs was not extended to term *updating*. So you're correct that this inconsistency is not by design.

Your patch is a good start, but it's not narrow enough - the checks that are required when updating a term are somewhat different than what is required when creating a term. We have to account specifically for the case where WP changes term slugs in response to a term parent being changed (for legacy reasons - you might argue that this is a bug). In 30780.2.patch​, I've broken out the 'slug' checks from the 'name' checks, because the requirements are different in each case, and term_exists() muddies the waters. With the patch, the constraints look like this:

• Duplicate 'slug' enforcement is performed by wp_unique_term_slug(). This mirrors wp_insert_term(). The one exception is, as noted above, that updating a term to a new parent that already has a child with the same slug will result in an updated term with a slug that's been appended with the parent slug.
• Duplicate 'name' is allowed in all cases, except for term that exist at the same hierarchy level of the same taxonomy.

I've also included new unit tests and modified the existing ones as necessary to describe the desired behavior.

ipm-frommen, would you mind having a look? Apply the patch locally and see if the behavior with term updates is what you'd expect.

[ipm-frommen]

Hi Boone,

thanks for your quick response.

I applied your patch locally, and it seems to be doing exactly what it should.

The fact that your term_exists error messages have a full stop at the end, while others don't, is just a cosmetic flaw. (To be honest, I would like to see full stops in some messages for along time, so this is a good start. ;))
Is there a reason for the blank line (new line 3378)? But again, this has no functional meaning.

Thanks again,
Thorsten

[ipm-frommen]

Sorry, me again.

I just realized that moving a top-level term foo to a parent always generates a new slug incorporating the parent's slug, even if there is no other term with the current slug.

This is due to this code fragment in wp_unique_term_slug function:

if ( is_taxonomy_hierarchical($term->taxonomy) && !empty($term->parent) ) {
	$the_parent = $term->parent;
	while ( ! empty($the_parent) ) {
		$parent_term = get_term($the_parent, $term->taxonomy);
		if ( is_wp_error($parent_term) || empty($parent_term) )
			break;
		$slug .= '-' . $parent_term->slug;
		if ( ! term_exists( $slug ) )
			return $slug;

		if ( empty($parent_term->parent) )
			break;
		$the_parent = $parent_term->parent;
	}
}

I suggest to change this into

if ( is_taxonomy_hierarchical($term->taxonomy) && !empty($term->parent) ) {
	$the_parent = $term->parent;

	$args = array(
		'slug'    => $slug,
		'exclude' => $term->term_id,
		'get'     => 'all',
		'parent'  => $the_parent,
	);
	$siblings_with_the_same_slug = get_terms( $term->taxonomy,$args );
	if ( is_wp_error( $siblings_with_the_same_slug ) || empty( $siblings_with_the_same_slug ) ) {
		return $slug;
	}

	while ( ! empty($the_parent) ) {
		$parent_term = get_term($the_parent, $term->taxonomy);
		if ( is_wp_error($parent_term) || empty($parent_term) )
			break;
		$slug .= '-' . $parent_term->slug;
		if ( ! term_exists( $slug ) )
			return $slug;

		if ( empty($parent_term->parent) )
			break;
		$the_parent = $parent_term->parent;
	}
}

The new part is this check right after having confirmed we have a hierarchical taxonomy and a parent:

	$args = array(
		'slug'    => $slug,
		'exclude' => $term->term_id,
		'get'     => 'all',
		'parent'  => $the_parent,
	);
	$siblings_with_the_same_slug = get_terms( $term->taxonomy,$args );
	if ( is_wp_error( $siblings_with_the_same_slug ) || empty( $siblings_with_the_same_slug ) ) {
		return $slug;
	}

What do you say? Do I miss something here?

If this is good, I'd happily provide a patch for this.

Cheers,
Thorsten

[boonebgorges]

Thanks for testing!

I just realized that moving a top-level term foo to a parent always generates a new slug incorporating the parent's slug, even if there is no other term with the current slug.

Is this with the patch applied, or even without the patch applied? Was this the behavior in 4.0 as well?

[ipm-frommen]

On WordPress 4.0.1, this does not happen.

With 4.1, it does. And it doesn't matter if I apply your (or mine) patch, or none at all. This is true for a fresh WP 4.1 install.

[boonebgorges]

Thanks, ipm-frommen. I've confirmed this behavior, though it's only reproducible in a very specific set of circumstances. I'm going to do a bit more digging and open a separate ticket for the problem, as it's strictly speaking a regression. Your suggested fix makes sense in a way, but I think we'll need to go with something more conservative (ie local to wp_update_term()) in the short term. When I've got that ticket number, I'll post it here.

In the meantime we can continue to use this ticket to discuss support for duplicate slugs when using wp_update_term().

[boonebgorges]

ipm-frommen - I'm still trying to get to the bottom of this, but was hoping for some clarification from you first. You said that "moving a top-level term foo to a parent always generates a new slug incorporating the parent's slug, even if there is no other term with the current slug." I can't reproduce this: I can only get the new slug to generate when a term exists in a different taxonomy with the same slug. See 30780.tests.patch​. Can you look over the tests to see if I'm understanding your report correctly?

[boonebgorges]

(Just to be clear: in 30780.tests.patch​, the difference between the two tests is that the first one is changing the parent of a term with no slug conflicts in *any* taxonomies, while the second one is changing the parent of a term that has a slug conflict with another taxonomy. The second test is failing, but the first is passing - which seems counter to what you're suggesting.)

[ipm-frommen]

Hi Boone,

you're right with your assumption/observation. I happened to use a term that I already had inserted to another taxonomy. When moving this term to some parent, its slug will be generated using the parent's slug. If there is no other term with this slug - even in other taxonomies - then there's no problem (i.e., renaming of the slug).

That being said, I was wondering what you meant by go with something more conservative (ie local to wp_update_term()). My addition to the wp_unique_term_slug function makes sense to me in any case. If there is no term with the same slug having the same parent, then there should be no need for generating a new slug (incorporating the parent term's slug). Or do I miss something here?

The naming wp_unique_term_slug, however, does not fit anymore as it is sufficient to generate a slug that's unique in the given context (i.e., either unique for a non-hierarchical taxonomy, or unique for a hierarchical taxonomy wrt. the term's parent). So far, I didn't come up with a more fitting name, though.

Cheers,
Thorsten

[boonebgorges]

If there is no other term with this slug - even in other taxonomies - then there's no problem (i.e., renaming of the slug).

Excellent - thanks for confirming.

My addition to the wp_unique_term_slug function makes sense to me in any case.

It makes sense for the two places where wp_unique_term_slug() is currently used in core, but this function has existed in WP for many years, and is probably being used in various ways in plugins. This needs some analysis before we can go making substantive changes to its behavior. At a minimum, it needs more comprehensive unit test coverage.

The naming wp_unique_term_slug, however, does not fit anymore as it is sufficient to generate a slug that's unique in the given context

I think this is a good observation, and it shows that changing the behavior of wp_unique_term_slug() is probably not a great idea, since it'll change the semantics of the function to some extent. It might be that the ideal solution will involve a new function with a new name, as I think that the stuff in that function related to parents no longer really makes any sense.

I spent a few hours staring at this today and need to sleep on it to make any further conclusions :) I'll be back within the next few days with a patch. We'll definitely do something in 4.2.

[dubh]

Related issue?

Conditions:

• A post type named Articles with an Authors taxonomy
• A post type named Events with a Speakers taxonomy

Steps:

1. Add a term to Authors with the name "John Smith" and the slug "john-smith".
2. Add a term to Speakers with the name "John Smith" and the slug "john-smith".

Results:

1. Two terms now have the same slug. As of v4.1, this is the expected behavior.
2. However, the name and description of the term "John Smith" in the Speakers taxonomy cannot be edited.
3. Only the slug can be edited. When the slug is renamed, the name and description can be edited again.

[boonebgorges]

dubh - Thanks for the report. This is indeed a related issue. I'm moving this ticket into the 4.1.1 milestone, because the behavior you describe sounds like a regression in 4.1.

A summary of what's happening: As of 4.1, multiple terms can be created with the same slug, as long as they're not in the same taxonomy. This improvement was not extended properly to term *updating*. As it stands, attempting to edit a term so that it has the same slug as an older term (ie a term with a lower term_id) - whether or not that term is in a different taxonomy, and whether the term being updated already has the shared slug or whether it's being updated to have that slug - the update will fail.

Having spent some time looking at the duplicate checks in wp_insert_term() and wp_update_term(), I think that it may be a good idea to do a systematic overhaul for 4.2 so that (a) the duplicate restrictions are identical between the two functions, and (b) those restrictions are not overly conservative. But for 4.1.1, we can eliminate the immediate problems simply by swapping this duplicate check https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/taxonomy.php#L3355 with one that is specific to the taxonomy.

See 30780.3.patch​ for my suggested fix for 4.1.1, with unit tests demonstrating the problem. dubh, ipm-frommen - feedback would be welcome on whether this sounds sane, and whether this simple fix resolves your issues.

[ipm-frommen]

Hi Boone,

I just tried your new patch, and it seems to work. Thanks for that.

What I still don't really like is the following:

Scenario A

• there is a top-level term with the slug foo
• you add a new term Foo, which you also would like to have the slug foo, to an already existing term Bar
• the new term gets the slug foo-bar instead

Scenario B

• there is a child term with the slug foo (i.e., this term has a parent)
• you add a new top-level term Foo, which you also would like to have the slug foo
• the new term gets the slug foo-2 instead

And here's the thing: in both cases, you can edit the new term and manually make it have the slug foo.

The reason for the automatically (and unnecessarily) given suffixed slug lies within the wp_unique_term_slug function. Okay, we already (silently) agreed in that it might be best not to change this function. But then we should take care of it not being used when it's not necessary. This means we either create a new function that takes care of checking (and maybe adapting) the new slug, or we have to check some cases and according to these don't use wp_unique_term_slug - just because it's not necessary.

I hope this makes sense.

Cheers,
Thorsten

[dubh]

Replying to boonebgorges:

See 30780.3.patch​ for my suggested fix for 4.1.1, with unit tests demonstrating the problem. dubh, ipm-frommen - feedback would be welcome on whether this sounds sane, and whether this simple fix resolves your issues.

Your patch appears to work for my purposes. I can fully edit terms, in different taxonomies belonging to different post types, that have the same slugs without having to rename those slugs.

[boonebgorges]

Thanks for the feedback, all.

What I still don't really like is the following

I agree that this behavior is not ideal. But this is the way duplicate slug checks have always worked. We should revisit them in light of the changes in 4.1, but it's beyond the scope of what should be done to address the regression in this ticket.

[boonebgorges]

In 30985:

In wp_update_term(), limit duplicate slug checks to the same taxonomy as the updated term.

In 4.1 [30240], wp_insert_term() was modified to allow the creation of terms
with duplicate slugs, as long as the terms are in different taxonomies.
wp_update_term() didn't get the corresponding modification, with the result
that term updates fail when the term being updated shares a slug with an older
term, regardless of that older term's taxonomy.

Props ipm-frommen.
Fixes #30780 for trunk.

[boonebgorges]

Reopening for 4.1.1.

[boonebgorges]

#30882 was marked as a duplicate.

[valendesigns]

Is this going to make it into 4.1.1? It's a really annoying issue. Especially when you're dealing with custom taxonomies and can't use any of the action hooks in wp_update_term because the code errors before they're executed. The only option would be to filter at wp_update_term_parent as an interim solution.

[boonebgorges]

Replying to valendesigns:

Is this going to make it into 4.1.1? It's a really annoying issue. Especially when you're dealing with custom taxonomies and can't use any of the action hooks in wp_update_term because the code errors before they're executed. The only option would be to filter at wp_update_term_parent as an interim solution.

The milestone on the ticket is 4.1.1.

[valendesigns]

Replying to boonebgorges:

The milestone on the ticket is 4.1.1.

Yes, but it's only been merged into trunk. So I suppose I was just curious to know if this would actually make it in, and more importantly why it hasn't already? It's been sitting collecting dust for 5 weeks, so you can understand why I'm asking. Just trying to get some movement. Thanks for the response.

[boonebgorges]

This should be merged into the 4.1 branch some point soon, and will be part of the 4.1.1 release. I don't have any more information about when that will be.

[helen]

We often merge over right before release (beta or final). We're very good about staying on top of milestones in Trac, at least.

[dd32]

In 31378:

In wp_update_term(), limit duplicate slug checks to the same taxonomy as the updated term.

In 4.1 [30240], wp_insert_term() was modified to allow the creation of terms
with duplicate slugs, as long as the terms are in different taxonomies.
wp_update_term() didn't get the corresponding modification, with the result
that term updates fail when the term being updated shares a slug with an older
term, regardless of that older term's taxonomy.

Props ipm-frommen.
Merges [30985] to the 4.1 branch.
Fixes #30780.

[boonebgorges]

#31617 was marked as a duplicate.