WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#6593 closed defect (bug) (fixed)

Tags with custom slugs get recreated on post edit screen

Reported by: jhodgdon Owned by: ryan
Milestone: 2.8 Priority: normal
Severity: major Version: 2.5
Component: Taxonomy Keywords: tag custom slug duplicity has-patch
Focuses: Cc:

Description

Here is how to reproduce this bug (the short summary might not be clear about what I am talking about). This applies to 2.5 released version. It was working in earlier 2.5 bleeding, early on, but then got broken by the time of the release, not sure exactly when.

a) Go to the Manage Tags screen, and create a new tag with name "abcdef" and slug "ghi" (without the quotes in all cases). Note that the slug does not match the "sanitized" slug WordPress would create by default.

b) Go to the Write Post screen (edit an existing or create a new post, enter a title and something in the body just to have something to work with).

c) In the Tags section of the Write Post screen, start typing "abc" in the Add Tag box. When "abcdef" appears, click on it, and then click "Add". You'll see it below.

d) Save the post.

e) Return to the Manage Tags screen. You will see that there are now two "abcdef" tags, and the new one has slug "abcdef". The new one is the one that is attached to the post.

The reason for this happening is that when you save the post, WP is taking the tag *names* (which it has carefully saved in a hidden field "tags_input" on the edit screen), and then using is_term to see if they already exist in the DB. is_term just sanitizes the tag name and searches for it as a slug. It doesn't match, so WP creates a brand-new tag.

The reason this is important is that (a) some people override slugs (e.g. non-Euro language users) and (b) if you pick an existing tag from a drop-down list, you should certainly get the tag you picked, not create a new one!!!

The solutions I can imagine:

1) Save the tag slugs rather than tag names in the hidden field, so they will match. This would probably also mean that tag slugs rather than tag names would appear on the post edit screen in the "tags to delete" area, since these delete links and display names are generated from that field.

2) Use something other than is_tag to do the matching.

Any thoughts on which solution to pursue? (1) is probably easier, but (2) might be nicer on the display.

Attachments (9)

match_tag_names.diff (2.5 KB) - added by jhodgdon 7 years ago.
Patch to allow matching of tag names when saving post
is_term.diff (2.4 KB) - added by ryan 7 years ago.
is_term.2.diff (2.9 KB) - added by ryan 7 years ago.
6593.diff (2.9 KB) - added by DD32 7 years ago.
6593.2.diff (3.2 KB) - added by DD32 7 years ago.
6593_apostrophes.diff (1.8 KB) - added by jhodgdon 6 years ago.
Patch to fix apostrophes issue and tag cloud sanitizing issue
6593_cloud.diff (1.3 KB) - added by jhodgdon 6 years ago.
Fix to the tag cloud - no filtering
6593_stripslash.diff (818 bytes) - added by jhodgdon 6 years ago.
Strips slashes in is_term always (also trims, to fix #9347)
6593_stripslash_against_11151.diff (432 bytes) - added by jhodgdon 6 years ago.
Ryan is too fast for me. This patch is against [11151]

Download all attachments as: .zip

Change History (71)

@jhodgdon7 years ago

Patch to allow matching of tag names when saving post

comment:1 @jhodgdon7 years ago

Here's a patch that lets is_tag match on tag names as well as tag slugs (but leaves previous behavior unchanged for back compatibility). It fixes this problem, and I don't think it should introduce any other problems...

comment:2 @jhodgdon7 years ago

  • Keywords has-patch added

comment:3 @ryan7 years ago

This might be behavior we want to do by default, without a flag. Either way, I think we will need to add an index on the name to make the query reasonably fast.

comment:4 @jhodgdon7 years ago

Any chance of getting this into 2.6? Can I do anything to help? There is a patch here...

comment:5 @ryan7 years ago

  • Milestone changed from 2.5.2 to 2.9

Milestone 2.5.2 deleted

@ryan7 years ago

comment:6 @ryan7 years ago

Patch adds an index to term.name and does a query on the name of the slug query does not match anything. I decided to do two queries instead of lumping it into so that the typical case of where the slug matches doesn't get slowed down. "slug" has a UNIQUE index meaning the SELECT does a very fast const join type. SELECTing on slug OR name results in a slower index_merge. Since we expect to be matching on the slug in most cases, lets keep that as fast a possible.

comment:7 @ryan7 years ago

  • Milestone changed from 2.9 to 2.7

See also #6313

comment:8 @ryan7 years ago

  • Owner changed from anonymous to ryan

comment:9 @jhodgdon7 years ago

I just tested in 2.7-bleeding [8372].

First, I verified that the bug as described originally still exists (it does).

Then I applied Ryan's patch (is_term.diff from above) (it required me to update my database).

As I had created two tags I no longer wanted during the first test, the first thing after updating was to go to the Manage Tags screen to delete the dummy tags and start over. But I couldn't successfully delete tags there -- I checked some tags and clicked the Delete button, and although the message on the screen said they were deleted, they weren't deleted.

I reverted the patch (it again required me to update my database). Now I can delete tags.

So, I think there is something wrong with the patch... I don't know exactly what, but I wouldn't advise adding this patch to WordPress yet.

@ryan7 years ago

comment:10 @ryan7 years ago

The ID case was not being handled. is_term.2.diff fixes deletes for me.

comment:11 @ryan7 years ago

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

(In [8433]) Check both slug and name when determining if is_term(). fixes #6593 for trunk

comment:12 @ryan7 years ago

(In [8555]) Use get_term_by() instead of is_term() to query slug. see #6593

comment:13 @ryan7 years ago

  • Milestone changed from 2.7 to 2.6.1

comment:14 @ryan7 years ago

(In [8559]) Check both slug and name when determining if is_term(). fixes #6593 for 2.6

comment:15 @jhodgdon7 years ago

I just tested this in the 2.6 branch [8559], and it looks fine. Thanks for making this part of 2.6.1! Much appreciated.

comment:16 @count_07 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I tested 2.6 branch.I use japanese tag and english slug.That is fine only one tagging for the article.

If some tagging for the article,first tag is fine.but other tags recreated slug.[6313] solve a this problem.Please include [6313] in 2.6.1.

comment:17 @ryan7 years ago

The fix here is a more correct version of the fix on #6313. I cannot reproduce the problem you describe. Can you give us some example tag names and describe the steps you take to make the problem happen?

comment:18 @count_07 years ago

  • Cc count_0 added

I mistake WikiFormatting.I want to include this patch.

Steps

  1. Make a new tag at manage -> Tags with tag name "フー", tag slug "foo", "バー", tag slug "bar".
  2. Write a post, add a tag "フー" and "バー" hit the save button.
  3. Back to tag manage -> Tags no recreated(no problem).
  4. Go post manage and reedit post that tagged "フー" and "バー". -> Don't touch tags.Edit and Save post.
  5. Back to tag manage -> "フー" tag not recreated."バー" tag recreated.

comment:19 @count_07 years ago

Other case:

  1. Make a new tag at manage -> Tag name "San Andreas", slug "sa" and Tag name "Vice City", slug "vc"
  2. Write a post, add tags "San Andreas" and "Vice City" -> Save post.
  3. Back to tag manage -> "Vice City" recreated.New "Vice City" slug is "vice-city".

First tag is not recreated.but second or more tags are recreated.
This problem happen wordpress 2.6 branch.

I apply Ticket #6313: wp-admin-includes-taxonomy.diff.

Test:

  1. Delete recreated Tag on tag manage(Tag name "Vice City", slug is "vice-city")
  2. Edit post tagged "San Andreas" -> Tagging "Vice City" again.
  3. Back to tag manage."Vice City" is not recreated.

comment:20 @ryan7 years ago

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

(In [8602]) Fix tag duplication when saving posts with multiple tags that have custom slugs. Props mtekk and count_0. fixes #6593 see #6313

comment:21 @ryan7 years ago

(In [8603]) Fix tag duplication when saving posts with multiple tags that have custom slugs. Props mtekk and count_0. fixes #6593 see #6313

comment:22 @ryan7 years ago

Oops, I overlooked that. Thanks for pointing this out and providing the steps to reproduce.

comment:23 @lilyfan7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In 2.6.1 beta1, the issue is fixed when creating a new post. But, not fixed at editing the existing posts.

Example:

  1. Creat a tag "プラグイン" with a custom slug "plugin". ("プラグイン" means "plugin" in Japanese)
  2. Write a new post with tag "プラグイン". The post has the tag with slug "plugin". This is desired behavior.
  3. Edit the post. The tag of it will change to a new one with slug "プラグイン". Tag is duplicated: The same name "プラグイン", one has slug of "plugin" and the other has slug of "プラグイン".

comment:24 @ryan7 years ago

[8602] should fix. The next beta will include [8602].

comment:25 @jhodgdon7 years ago

Looks good, from a multilingual perspective, in 2.6.2 [8640]

comment:26 @jhodgdon7 years ago

Sorry, that was the 2.6.1 branch, changeset [8640], not 2.6.2 -- my finger slipped.

comment:27 @jhodgdon7 years ago

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

comment:28 @randyhoyt7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is happening again in 2.6.2.

comment:29 @ryan7 years ago

We're going to need specific examples because this works fine for me.

comment:30 @randyhoyt7 years ago

It's having the problem with the tag "Man's Relationship To The Divine". (I know; what a pretentious tag. :~) Perhaps it's because it has an apostrophe? The tag "Aesopic Fables" is working correctly.

comment:31 @neverbot7 years ago

I'm using the Tag C/C++ (programming freak, you know) and c-cplusplus as tagslug. Posting a text with two tags (first 'Programming' and second 'C/C++', for instance) duplicates the tag C/C++ (the original with its slug and the second with 'cc').

Updated just some minutes ago to WP 2.6.3, so it's still happening :(

Don't know if priority should change to high (not so important, but it's a quite old bug).

comment:32 @neverbot7 years ago

Another comment: is kinda weird, but if i edit the post and i add/remove some tags, it works fine, no matter if the tags have 'personalized' slugs or not, no matter the order (adding the personalized as the first or the second of the new tags)...

I think this is happening only the first time you're saving the post... could be? Could somebody else corroborate?

comment:33 @jhodgdon7 years ago

  • Milestone changed from 2.6.1 to 2.7

Just a note that this bug is occurring again in 2.7-bleeding [9621].

To reproduce, follow the original steps in the bug report, but use the tag

Man's Relationship To The Divine

with slug

man_devine

Note that the original reproduce (with tag abcdef and slug ghi) is NOT a problem in [9621], but this tag including an apostrophe is.

I am changing the milestone to 2.7 (was 2.6.1) so that maybe it will be addressed.

@DD327 years ago

comment:34 @DD327 years ago

  • Keywords needs-testing added

attachment 6593.diff added.

  • Fixes the issue of having tags with conflicting slugs, Eg, 'C++' => slug 'c', 'C' => slug 'c', After the patch, 'C' would get 'c-2'
  • Fixed the custom slug issue, Eg, Tagname: 'Jokes', slug: 'great-jokes', Adding a tag 'Great Jokes' will no longer recieve the slug 'great-jokes'(Which subsequently means the new tag vanishes) and will instead, get 'great-jokes-2'
  • Adding a Tag of 'C++' => slug 'c', and then adding a Category of 'C++' results in a category and tag with the same slug 'c', According to the Taxonomy documentation, The slug needs to be unique accross all taxonomies, Therefor, This patch will also cause the above to take the form of (tag)'C++' => slug 'c', (cat)'C++' => slug 'c-2', If this isnt the intended behaviour and the docs are wrong, wp_unique_term_slug() needs to be updated to take into account the taxonomy

Patch obviously needs some testing.

@DD327 years ago

comment:35 @DD327 years ago

attachment 6593.2.diff added.

Knew there'd be a problem with it..

  • Patch attempts to fix is_term() for when a term slug is passed in, as well as working when a term Name is passed in, Since these 2 are passed through the same variable, it looks a bit hackish

comment:36 @ryan7 years ago

See #5034 for the purpose of wp_unique_term_slug().

comment:37 @ryan7 years ago

  • Milestone changed from 2.7 to 2.8

Since this is not a regression versus 2.6.3 and is a pain to fix properly, let's postpone to 2.8.

comment:38 @janbrasna7 years ago

  • Cc janbrasna added
  • Component changed from Administration to Taxonomy
  • Keywords tag custom slug duplicity needs-patch added; tags has-patch needs-testing removed

I have to confirm that the steps outlined in comment:18 can reproduce the bug (again) in 2.7-final.

comment:39 @Denis-de-Bernardy6 years ago

  • Keywords reporter-feedback added; tag custom slug duplicity needs-patch removed

still current in trunk?

comment:40 @jhodgdon6 years ago

  • Keywords ag custom slug duplicity needs-patch added; reporter-feedback removed

I just tested this in 2.8-bleeding [11116].

Summary: Most of the reproduce options above are fixed, but there is still one that doesn't work.

Details:

The original reproduce from the bug description has been fixed for a long time, and is still fixed in 2.8-bleeding.

The Japanese reproduce from comment:18 above appears to be fixed too.

The San Andreas / Vice City issue from comment:19 above appears to be fixed too.

The Japanese reproduce form comment:23 above appears to be fixed too.

The C/C++ reproduce from comment:31 above appears to be fixed too.

However, the reproduce from comment:30 is still a problem. Steps:

a) Create a tag on the Post Tags screen
Name: Man's Relationship To The Divine
Slug: man_divine

b) Edit a post. Add this tag using the tag area of the screen (start typing Man, select from drop-down list).

c) Save the post.

d) Return to the tag manage screen and you will see two copies of the Man's Relationship to the Divine tag there (the new one has slug mans-relationship-to-the-divine).

So it appears that tags with apostrophes are still broken.

You know, this whole issue with tags would go away if the tag add section worked like category add -- behind the scenes, using the term ID rather than the term name as the identifier. Or use the term slug. Either way, it would be much more robust, and would also allow the term names to be filtered when displayed in the tag add area (good for multilingual sites). This would require a total rewrite of the tag section, but I think it would be a good idea. You'd have to think hard about what to do for non-JS users...

One more comment on this idea of total rewrite. As it is, the "choose from most used tags" functionality does not work with multilingual plugins right now, because the tag cloud is displaying filtered names. That would be OK, but those displayed names are then what is saved to the "add tags" area. And that doesn't work -- in order for this matching to work when the post is saved, it needs to save the raw, unfiltered names to the tag add area. If what was really being used behind the scenes was the tag slug or the tag ID, then this problem would not be present.

comment:41 @jhodgdon6 years ago

  • Keywords tag added; ag removed

comment:42 @Denis-de-Bernardy6 years ago

  • Severity changed from normal to major

confirmed with your test case...

comment:43 @Denis-de-Bernardy6 years ago

probably a dup of #9397

comment:44 @jhodgdon6 years ago

I don't think it is an exact duplicate, because #9397 happens in Quick Edit and does not require apostrophes to get tag duplication (which seems to be the only use case that still causes problems on the main post edit screen).

However, they may have related causes.

comment:45 @hakre6 years ago

  if (empty('database abstraction')) 
   echo 'WordPress';

comment:46 @hakre6 years ago

the bug is somewhere in wp_set_object_terms() and functions called by that function.

comment:48 @jhodgdon6 years ago

  • Owner changed from ryan to jhodgdon
  • Status changed from reopened to new

I'm going to work on a patch for this and #9397 tomorrow.

comment:49 @jhodgdon6 years ago

  • Status changed from new to assigned

@jhodgdon6 years ago

Patch to fix apostrophes issue and tag cloud sanitizing issue

comment:50 @jhodgdon6 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from jhodgdon to ryan
  • Status changed from assigned to new

The patch I just added fixes the apostrophes issue. The problem turned out to be fairly straightforward -- when WP receives POST data on post save, it has all the backslashes and quotes escaped. So we have to strip them back out. The rest of the post save routine does this, but the tag stuff wasn't.

So it was trying to compare "Man\'s Relationship" to the saved tag "Man's Relationship", and of course it was failing and creating a new tag instead.

I also fixed a second problem in this patch, which was that the Tag Cloud option for selecting tags wasn't working -- it was sanitizing the tag cloud for display (i.e. applying display filters), and then assuming the raw unfiltered name was OK to insert into the tags list. The solution (which is ugly but correct) is to display a tag cloud with unfiltered names, so that when you click on one the correct unfiltered term name is added to the post.

I haven't looked at #9397 yet. So far I think this is probably only a fix for #6593.

comment:51 @ryan6 years ago

I think the stripslashes needs to be done within is_term() on $else_where_fields. That should fix similar problems in other places.

comment:52 follow-up: @jhodgdon6 years ago

I'm not sure about that. We'd have to check all callers of the function and see whether they already stripped slashes prior to calling this function (stripping them again could get rid of legitimate slashes).

Also, the strip slashes also needs to be done when a new term is created from $_POST data, just as it is done on the post title, etc. in the wp_insert_post() function prior to saving in the DB (I think that is the right function). So it is not just for is_term() to do term matching. Or are slashes stripped when terms are created?

comment:53 @hakre6 years ago

Just FYI: I hope to get WP-hackers list to run because I thought about having some classes to do the terms and taxonomy handlings. these classes should provide a clear interface. these classes can be used in the backend then.

comment:54 in reply to: ↑ 52 @ryan6 years ago

Replying to jhodgdon:

I'm not sure about that. We'd have to check all callers of the function and see whether they already stripped slashes prior to calling this function (stripping them again could get rid of legitimate slashes).

Almost all of our functions expect slashed data. wp_set_object_terms() expects slashed data when it calls wp_insert_term().

comment:55 @hakre6 years ago

But it also looks for slugs guessed on names when you only want to look for names.... :/

comment:56 follow-up: @ryan6 years ago

AFAICT, all uses of is_term() are passing slashed data. This didn't matter back when we checked against just that ID and slug, but since we started checking against name this is a problem. As do other functions, I think is_term() should stripslashes before it prepare()s. Any function not documented as accepting unslashed data should expect slashes.

comment:57 in reply to: ↑ 56 @hakre6 years ago

Replying to ryan:

[...] Any function not documented as accepting unslashed data should expect slashes.

Just for clarification, because normally I know it the other way round. You want to have every function to accept slashed data if their docblock does not state to accept unslashed/normal data?

comment:58 follow-up: @jhodgdon6 years ago

Not sure about your doc logic Ryan - it seems like the function doc should state it expects raw, slash-escaped data as if from $_POST if that is what it expects.

Point taken on is_term() however. I'll re-patch, or you can? Let me know.

comment:59 in reply to: ↑ 58 @ryan6 years ago

Replying to jhodgdon:

Not sure about your doc logic Ryan - it seems like the function doc should state it expects raw, slash-escaped data as if from $_POST if that is what it expects.

Then we'd have to document all but three or four functions. I am lazy. :-)

Point taken on is_term() however. I'll re-patch, or you can? Let me know.

Can you patch and verify it still addresses your testbase?

@jhodgdon6 years ago

Fix to the tag cloud - no filtering

@jhodgdon6 years ago

Strips slashes in is_term always (also trims, to fix #9347)

comment:60 @jhodgdon6 years ago

OK, I put it in two separate patches.

"cloud" patch - fix for the tag cloud, so it is displayed with no filtering, in order to have minimal code changes and still have the cloud method of choosing tags work with multilingual and other filters (needs to be raw tag name, not filtered tag name).

"stripslash" patch - strips slashes in is-term() no matter what. Also includes the trim patch from #9397 (sorry, that was a typo above, 9397 not 9347).

@jhodgdon6 years ago

Ryan is too fast for me. This patch is against [11151]

comment:61 @ryan6 years ago

(In [11153]) strip slashes from term before prepare(). Props jhodgdon. see #6593

comment:62 @ryan6 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.