Tags with custom slugs get recreated on post edit screen

[jhodgdon]

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.

[jhodgdon]

Patch to allow matching of tag names when saving post

[jhodgdon]

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

[ryan]

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.

[jhodgdon]

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

[ryan]

Milestone 2.5.2 deleted

[ryan]

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.

[ryan]

See also #6313

[jhodgdon]

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.

[ryan]

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

[ryan]

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

[ryan]

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

[ryan]

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

[jhodgdon]

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.

[count_0]

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.

[ryan]

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?

[count_0]

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.

[count_0]

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.

[ryan]

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

[ryan]

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

[ryan]

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

[lilyfan]

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 "プラグイン".

[ryan]

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

[jhodgdon]

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

[jhodgdon]

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

[randyhoyt]

This is happening again in 2.6.2.

[ryan]

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

[randyhoyt]

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.

[neverbot]

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

[neverbot]

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?

[jhodgdon]

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.

[DD32]

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.

[DD32]

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

[ryan]

See #5034 for the purpose of wp_unique_term_slug().

[ryan]

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

[janbrasna]

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

[Denis-de-Bernardy]

still current in trunk?

[jhodgdon]

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.

[Denis-de-Bernardy]

confirmed with your test case...

[Denis-de-Bernardy]

probably a dup of #9397

[jhodgdon]

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.

[hakre]

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

[hakre]

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

[hakre]

see #9397

[jhodgdon]

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

[jhodgdon]

Patch to fix apostrophes issue and tag cloud sanitizing issue

[jhodgdon]

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.

[ryan]

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

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

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?

[hakre]

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.

[ryan]

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

[hakre]

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

[ryan]

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.

[hakre]

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?

[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.

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

[ryan]

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?

[jhodgdon]

Fix to the tag cloud - no filtering

[jhodgdon]

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

[jhodgdon]

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

[jhodgdon]

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

[ryan]

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

[ryan]

[11158]