Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#30780 closed defect (bug) (fixed)

Inconsistent behavior/results of wp_insert_term and wp_update_term

Reported by: ipm-frommen's profile ipm-frommen Owned by: boonebgorges's profile boonebgorges
Milestone: 4.1.1 Priority: normal
Severity: major Version: 4.1
Component: Taxonomy Keywords: has-patch fixed-major commit
Focuses: Cc:

Description

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.

Attachments (4)

30780.patch (1.9 KB) - added by ipm-frommen 10 years ago.
30780.2.patch (6.4 KB) - added by boonebgorges 10 years ago.
30780.tests.patch (2.1 KB) - added by boonebgorges 10 years ago.
30780.3.patch (5.9 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (34)

@ipm-frommen
10 years ago

#1 @ipm-frommen
10 years ago

  • Summary changed from Inconsistent behavior/results of wp_inesrt_term and wp_update_term to Inconsistent behavior/results of wp_insert_term and wp_update_term

#2 @ipm-frommen
10 years ago

  • Keywords has-patch added

#3 @boonebgorges
10 years ago

  • Milestone changed from Awaiting Review to 4.2
  • Type changed from defect (bug) to enhancement

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.

#4 @ipm-frommen
10 years ago

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

#5 @ipm-frommen
10 years ago

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

Last edited 10 years ago by ipm-frommen (previous) (diff)

#6 @boonebgorges
10 years ago

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?

#7 @ipm-frommen
10 years ago

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.

Last edited 10 years ago by ipm-frommen (previous) (diff)

#8 @boonebgorges
10 years ago

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

#9 @boonebgorges
10 years ago

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?

#10 @boonebgorges
10 years ago

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

#11 @ipm-frommen
10 years ago

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

#12 @boonebgorges
10 years ago

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.

#13 @dubh
10 years ago

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.
Last edited 10 years ago by dubh (previous) (diff)

#14 follow-up: @boonebgorges
10 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 4.2 to 4.1.1

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.

#15 @ipm-frommen
10 years ago

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

Last edited 10 years ago by ipm-frommen (previous) (diff)

#16 in reply to: ↑ 14 @dubh
10 years ago

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.

#17 @boonebgorges
10 years ago

  • Keywords 2nd-opinion removed
  • Type changed from enhancement to defect (bug)

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.

#18 @boonebgorges
10 years ago

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

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.

#19 @boonebgorges
10 years ago

  • Keywords fixed-major added

#20 @boonebgorges
10 years ago

  • Keywords commit added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.1.1.

#21 @boonebgorges
10 years ago

#30882 was marked as a duplicate.

#22 follow-up: @valendesigns
10 years ago

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.

#23 in reply to: ↑ 22 ; follow-up: @boonebgorges
10 years ago

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.

#24 in reply to: ↑ 23 @valendesigns
10 years ago

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.

#25 @boonebgorges
10 years ago

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.

#26 @helen
10 years ago

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

#27 @dd32
10 years ago

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

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.

This ticket was mentioned in Slack in #core by helen. View the logs.


10 years ago

#29 @nacin
10 years ago

  • Severity changed from normal to major

#30 @boonebgorges
10 years ago

#31617 was marked as a duplicate.

Note: See TracTickets for help on using tickets.