#30780 closed defect (bug) (fixed)
Inconsistent behavior/results of wp_insert_term and wp_update_term
Reported by: | ipm-frommen | Owned by: | 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)
Change History (34)
#1
@
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
#3
@
10 years ago
- Milestone changed from Awaiting Review to 4.2
- Type changed from defect (bug) to enhancement
#4
@
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
@
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
#6
@
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
@
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.
#8
@
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
@
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
@
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
@
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
@
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
@
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:
- Add a term to Authors with the name "John Smith" and the slug "john-smith".
- Add a term to Speakers with the name "John Smith" and the slug "john-smith".
Results:
- Two terms now have the same slug. As of v4.1, this is the expected behavior.
- However, the name and description of the term "John Smith" in the Speakers taxonomy cannot be edited.
- Only the slug can be edited. When the slug is renamed, the name and description can be edited again.
#14
follow-up:
↓ 16
@
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
@
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
#16
in reply to:
↑ 14
@
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
@
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
@
10 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from new to closed
In 30985:
#20
@
10 years ago
- Keywords commit added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for 4.1.1.
#22
follow-up:
↓ 23
@
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:
↓ 24
@
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 atwp_update_term_parent
as an interim solution.
The milestone on the ticket is 4.1.1.
#24
in reply to:
↑ 23
@
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
@
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
@
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.
ipm-frommen - Thanks for the report.
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:wp_unique_term_slug()
. This mirrorswp_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.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.