WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 months ago

#16567 new defect (bug)

Can't create categories with different names but similar slugs on Edit Post screen

Reported by: jeffreymcmanus Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0.5
Component: Posts, Post Types Keywords: has-patch 3.6-early needs-unit-tests
Focuses: administration Cc:

Description

We run a blog that deals with programming languages. Today we were creating a post concerning the C programming language and attempted to create a new category for it, called "C" (just one character). This angered WordPress; it appeared to create a superfluous "Uncategorized" category, but it wound up not actually creating any categories for the post.

I've attached a screenie so you can see what this looks like after I attempt to create the category.

If it's for some reason forbidden to create categories comprised of a single character, maybe a validation error message of some kind would be in order?

Attachments (3)

wordpress_c_category_doh.png (29.2 KB) - added by jeffreymcmanus 3 years ago.
Screen shot of error
16567.diff (742 bytes) - added by garyc40 3 years ago.
16567.2.diff (1.6 KB) - added by SergeyBiryukov 19 months ago.

Download all attachments as: .zip

Change History (24)

jeffreymcmanus3 years ago

Screen shot of error

comment:1 linuxologos3 years ago

I can reproduce that, if I try to create a category with a name resembling another category but only through the Add New Category on the Add New Post screen. Creating the category through Posts > Categories succeeds. So the problem is not the single-character category name in general.

Try to create the category through Posts > Categories to see if it works.

comment:2 linuxologos3 years ago

The bug is simple to reproduce. Just try to create a category that already exists or something like C, C#, C+, C! etc. (they have to be parent categories). "Uncategorized" is constantly appearing, but nothing is really created (try to click one of those "Uncategorized" and all of them are chosen) and refreshing the page makes them disappear.

This applies to WP 3.1-RC4 too.

comment:3 jeffreymcmanus3 years ago

Confirmed that it is possible to create a single-letter category through Posts > Categories (which I never knew existed until today!).

comment:4 garyc403 years ago

Can't reproduce on trunk (currently on r17453).

comment:5 garyc403 years ago

Wait, I can reproduce now. It happens when there's already a "C#" category, and you try to add "C".

garyc403 years ago

comment:6 garyc403 years ago

  • Keywords has-patch needs-testing added

The conditional check on line 263, admin-ajax.php seems to be unnecessary, and introduces edge case like this.

Removing the conditional check seems to fix this problem and produce no side effects (the same kind of check is performed within wp_insert_term, and it handles the case better by creating the term anyways with a numerical suffix).

Attached patch does this 16567.diff.

comment:7 garyc403 years ago

In case someone wonders why "Uncategorized" is output, it's because the return value of term_exists() can also be an array if a taxonomy is specified (wp-includes/taxonomy.php:1438). wp_terms_checklist() later casts this value as an int, making it 1 because it's an array. As a result, what you get is whatever category has the ID 1 (in this case, Uncategorized).

By the way, the inline documentation of term_exists() states that:

@return mixed Get the term id or Term Object, if exists.

Judging from the code I see, the return value could only be either an associative array, or an integer, but never a term object. Or did I miss something?

comment:8 MikeHansenMe19 months ago

  • Keywords close added

I cant seem to product this in trunk (3.5-alpha-21751), even with the example C# then C from the edit posts page.

SergeyBiryukov19 months ago

comment:9 SergeyBiryukov19 months ago

  • Component changed from General to Administration
  • Keywords needs-testing close removed
  • Milestone changed from Awaiting Review to 3.5

The code is still there, and I can reproduce the issue in both trunk and 3.4.2.

  1. Go to Edit Post screen.
  2. Create "C#" category.
  3. Try to create "C" category. It won't be created.

The only difference from 3.0.5 is that "Uncategorized" item no longer gets added to the list in step 3. That was fixed in [19792] (for #17939).

A check for empty name was added in [6303]. is_term() was added in [12798] as a check for an existing term, and replaced with term_exists() in [15220].

Looking at the code, it doesn't make much sense to check if a category with the same slug exists here. This is better handled further down the stack in wp_insert_term(), as noted by garyc40 in comment:6.

A check for empty name:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/taxonomy.php#L2031
A check for an existing term:
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/taxonomy.php#L2063
trim() is a part of in sanitize_text_field():
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/formatting.php#L3164

16567.2.diff removes unnecessary checks from ajax-actions.php.
is_array() is removed as well, since wp_insert_term() only returns an array or a WP_Error object.

Version 1, edited 19 months ago by SergeyBiryukov (previous) (next) (diff)

comment:10 SergeyBiryukov19 months ago

  • Summary changed from Can't create single-character categories to Can't create categories with different names but similar slugs on Edit Post screen

This isn't limited to single-character categories.

comment:11 follow-up: technosailor19 months ago

The problem is query_var collision. sanitize_title() is going to strip special chars like # in C# for the slug. So C# does in fact become 'c' as does C++ or just C.

I don't agree that wp_insert_term should be where the checks are happening. It adds a potentially unneeded query. We should not be doing wp_insert_term if term_exists. I believe the existing functionality is as it should be. I respect the problem but it's really edge and a core behavior modification just doesn't feel warranted. That's my opinion though.

comment:12 in reply to: ↑ 11 ; follow-up: SergeyBiryukov19 months ago

Replying to technosailor:

I don't agree that wp_insert_term should be where the checks are happening. It adds a potentially unneeded query. We should not be doing wp_insert_term if term_exists.

Quite the contrary, 16567.2.diff prevents term_exists() from being called twice if the term does not exist, since wp_insert_term() calls it anyway.

comment:13 in reply to: ↑ 12 ; follow-up: technosailor19 months ago

Replying to SergeyBiryukov:

Replying to technosailor:

I don't agree that wp_insert_term should be where the checks are happening. It adds a potentially unneeded query. We should not be doing wp_insert_term if term_exists.

Quite the contrary, 16567.2.diff prevents term_exists() from being called twice if the term does not exist, since wp_insert_term() calls it anyway.

Then wp_insert_term() shouldn't be calling it. We shouldn't be doing an insert if there is a collision as that adds a DB call.

comment:14 in reply to: ↑ 13 ericlewis18 months ago

Replying to technosailor:

Replying to SergeyBiryukov:

Replying to technosailor:

I don't agree that wp_insert_term should be where the checks are happening. It adds a potentially unneeded query. We should not be doing wp_insert_term if term_exists.

Quite the contrary, 16567.2.diff prevents term_exists() from being called twice if the term does not exist, since wp_insert_term() calls it anyway.

Then wp_insert_term() shouldn't be calling it. We shouldn't be doing an insert if there is a collision as that adds a DB call.

Changing the functionality of wp_insert_term seems out of the scope of this ticket.

16567.2.diff works for me.

Last edited 18 months ago by ericlewis (previous) (diff)

comment:15 nacin18 months ago

  • Keywords needs-unit-tests punt added

comment:16 SergeyBiryukov18 months ago

  • Keywords 3.6-early added; punt removed
  • Milestone changed from 3.5 to Future Release

comment:17 SergeyBiryukov16 months ago

  • Milestone changed from Future Release to 3.6

comment:18 SergeyBiryukov15 months ago

Seems like #17689 would fix this ticket as well.

However, the sanitization in _wp_ajax_add_hierarchical_term() and wp_ajax_add_link_category() still seems redundant (see comment:9), so 16567.2.diff might still make sense.

comment:19 ryan10 months ago

  • Milestone changed from 3.6 to Future Release

comment:20 SergeyBiryukov9 months ago

#24841 was marked as a duplicate.

comment:21 nacin2 months ago

  • Component changed from Administration to Posts, Post Types
  • Focuses administration added
Note: See TracTickets for help on using tickets.