WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#9397 closed defect (bug) (fixed)

Quick edit creates new tag from existing tag-name

Reported by: forposts Owned by: ryan
Milestone: 2.8 Priority: high
Severity: critical Version: 2.7.1
Component: Taxonomy Keywords: has-patch tested tag quick edit duplicity
Focuses: Cc:

Description

Product version:
2.8 svn from trunk, revision 10843.

Steps to reproduce:
1) Make some post.
2) Create new tag with "tag1" slug / "tag1" name, assign in to the post from 1).
3) Create second tag with "tag2" slug and "tag2 name" name. Assign it to the same post.
4) Go to /wp-admin/edit.php then choose Quick Edit for the post in question.
You'll see two terms "tag1, tag2 name" in Tags window.
Don't touch anything and push Update Post button.
5) Go to /wp-admin/edit-tags.php?taxonomy=post_tag
you'll find new unwanted tag with "tag2-name" slug.

I consider this a bug because of wrong behaviour - system must assign "tag2" slug to the post instead of making new slug from "tag2 name".

It looks like tag order is critical, the system works fine if you change tag places and enter "tag2 name, tag1" in Quick Edit window. In this case undesirable tag is not created.

Attachments (2)

9397.diff (397 bytes) - added by jhodgdon 5 years ago.
Fix for this problem
9397v2.diff (388 bytes) - added by jhodgdon 5 years ago.
Better patch - trims before creating slugs (see comments below)

Download all attachments as: .zip

Change History (36)

comment:1 forposts5 years ago

  • Component changed from Quick Edit to Taxonomy
  • Owner set to ryan
  • Version changed from 2.8 to 2.7.1

Same bug detected for WP 2.7.1

It seems that new "unauthorized" tag popping up when two conditions are met - it must be not the first tag in a row in Quick Edit AND tag slug and tag name are different.

Example.
Original tag list for the post, slug/name: "tag1"/"tag1 name", "tag2"/"tag2 name","tag2"/"tag3 name". If you click on Update Post in Quick Edit you get two new tags created, "tag2 name"/"tag2 name" and "tag3 name"/"tag3 name". That's bad behaviour for sure.

comment:2 hakre5 years ago

Quick Edit seems to be not working correctly. Is there a way to provide a set of unit-test to check if Quick Edit was ever working safe, strict and properly?

comment:3 forposts5 years ago

  • Component changed from Taxonomy to Quick Edit
  • Owner ryan deleted

comment:4 forposts5 years ago

  • Owner set to ryan

comment:5 Denis-de-Bernardy5 years ago

probably a dup of #6593

comment:6 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added; quick edit tags removed

comment:7 jhodgdon5 years ago

I don't think it is a duplicate, because this bug happens in Quick Edit and #6593 is in the full Edit screen. Also, #6593 is only happening in 2.8-bleeding for tags with apostrophes in them, whereas this bug does not involve apostrophes.

However, they may have related causes.

comment:8 Denis-de-Bernardy5 years ago

it's the same js being used in the background. but you might be right (which is why I didn't close this :-) ).

comment:9 follow-up: jhodgdon5 years ago

I don't think it's the JS that is causing tag duplication in #6593. At least last time I looked at it (around when I first created #6593, so several versions ago) the problem was in the PHP code that is taking the form POST submission, and from that figuring out which tags the user meant to associate with the post (i.e. corresponding the POST submission to term IDs).

That is why I think if the POST had the term IDs in it rather than trying to do text matching, we would avoid all these problems. That's what the categories section does, and it doesn't suffer from this problem.

I haven't ever looked into what Quick Edit is doing, though, so I have no idea if the problem is in JS or PHP for this bug. Likely it is in the PHP -- when you submit the quick edit, the info is submitted via AJAX (I think) and it is definitely PHP code that saves the post.

Then again, maybe you meant to say "same PHP being used in the background" rather than JS. :) Not sure where you are, but it's definitely the end of a long day here...

comment:10 jhodgdon5 years ago

Just as one more note, one of the reproduces from #6593 that is currently working in 2.8-bleeding was the tag "San Andreas", which has a space in it. So although I didn't test "tag2 name" in the full edit screen, I don't think spaces are an issue there, as they apparently are in Quick Edit.

comment:11 in reply to: ↑ 9 Denis-de-Bernardy5 years ago

Replying to jhodgdon:

Then again, maybe you meant to say "same PHP being used in the background" rather than JS. :) Not sure where you are, but it's definitely the end of a long day here...

A bit of both, actually. The stuff got revamped recently -- I had to update a few plugins of mine. And yeah, it *is* getting late over here in France. :D

comment:12 hakre5 years ago

I've just could reproduce it with the current 2.8 Trunk. From the view from above, Tag Parsing of Quickedit is not properly working here. I'll take a look inside.

comment:13 hakre5 years ago

  • Keywords needs-patch removed
  • Resolution set to wontfix
  • Status changed from new to closed

Quickedit uses (via 4 or 5 other functions) wp_set_object_terms() with an $append=false. Read yourself:

* @param array|int|string $term The slug or id of the term, will replace all existing 
* related terms in this taxonomy.

Actually passed is an array that contains the names, not slugs, not ids.

First all ids for the post are pulled. ids are numbers, not the names nor slugs.

Then the passed names (not slugs) as terms are passed one by one to is_term() which tries to find out by slug or name. that function mainly is for forming two sql queries who should look up for a particular tag by name or slug.

that function then calls sanitize_title() which replaces spaces. therefore the ones named tag has been converted to slug. but that is only slug by accident because the correct slug of 'tag2 name' is 'tag2' not 'tag2-name'.

because a tag with the slug 'tag2-name' can not be found, it will be inserted with the appropriate name ('tag2 name'). this is why a new tag is added. that tag will be discovered (new id) and the one with the old id won't be discovered any longer and therefore removed ($append = false).

how to patch?

I have no Idea. This is all so mixed up, something sanitzed here, something re-interpreted there. I doubt touching any of these functions is really promising.

i therefore set this to wontfix. feel free to provide a solution/patch and reopen then.

comment:14 follow-up: jhodgdon5 years ago

  • Keywords needs-patch tag quick edit added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

So you set it to "won't fix" just because you cannot think of a solution? How about setting to "needs patch"?

This is actually a pretty serious bug, because many users do custom slug tags (for instance, non-European languages and multi-lingual blogs). Setting it to "won't fix" means that no one will give it a second look to see if they can find a solution. Bad idea, in my opinion.

comment:15 in reply to: ↑ 14 hakre5 years ago

  • Component changed from Quick Edit to Taxonomy
  • Keywords 2nd-opinion probably-wontfix added
  • Priority changed from normal to high
  • Severity changed from normal to critical

Sorry to have you bugged with that. I explained why I set that to wontfix and it has taken me about one and a half hour to only understand why that actually is happening. I even know a way of how _that_ bug can be fixed but I assume then major other parts of the Backend won't work any longer. That is why I set that to wontfix, because to fix this, there is a lot more work to do.

As written, feel free to provide actually a concept how to fix or - even better - a patch.

I raised the Priority and Severity for you.

comment:16 hakre5 years ago

  • Summary changed from Quick edit creates new tag from existing tag name to Quick edit creates new tag from existing tag-name sanitizing to a new tag-slug taking over the existing tag-name as name for the new tag-name.

comment:17 hakre5 years ago

  • Summary changed from Quick edit creates new tag from existing tag-name sanitizing to a new tag-slug taking over the existing tag-name as name for the new tag-name. to Quick edit creates new tag from existing tag-name sanitizing to a new tag-slug taking over the existing tag-name as name for the new tag.

comment:18 hakre5 years ago

To get a view what all is happening, why and it's history, grab yourself a cup of coffee and read #6593. That helps to get an insight.

the problem with the current code is, that it is only a fragment around the database. there is no real concept (compare #6593), it is a fixing here and a fixing there and if it seems to work, the patch is accepted.

can someone provide a written concept of how taxnonomies should work?

comment:19 jhodgdon5 years ago

OK. This may be a slight simplification, but here it is, including some history.

Tags and categories are both "taxonomies", and a taxonomy is a set of terms.

In the database, there's a table that associates term IDs, taxonomy IDs, and post IDs. That is what tells WordPress which category and tag IDs (all numbers) are associated with each post.

There is another table that associates term IDs with term names and term slugs. This is what tells WordPress how to display a given term and figure out its URL.

When you edit a post (via full or quick edit), WP has to figure out which terms (categories or tags) the user meant to associate with the post, and save the IDs in the taxonomy/term/post association table.

Categories are easy (and in my opinion done the right way). When you edit a post (on the main edit screen), category choices are displayed with their displayed term names, but the edit screen behind the scenes is working with term IDs. So when you save the post, the desired term IDs are sent in the $_POST data, so there's no question as to which categories go with the post.

Tags are not so easy. When tags were first introduced in WP, the way they worked is that when you edited a post, there was a plain text field where you could type in your desired tags, separated by commas. When you saved the post, WP received the typed-in term names in $_POST. Then WP would sanitize the term names to convert them to valid URL slugs (there was no way to customize slugs for tags then except with a plugin). Then WP would look up the generated slugs in the term table to find the term IDs, add missing tags to the term table to get new IDs, and then associate all those term IDs with the post.

In more current versions of WP, you can customize your term slugs, and this lookup scheme broke down. It's been somewhat patched (see the long history of #6593), but obviously it isn't totally working. And just as a note, although on the main post edit screen you don't see a plain text field with tags separated by commas, behind the scenes that is exactly what is there (the field is simply hidden if you have JavaScript enabled).

What I think needs to be done to fix this bug is to overhaul the way tags are selected for a post on both the Quick Edit screen and the full Edit screen, so that they work more like categories. That is, whatever the user interface is, when you click on, type, or otherwise select a tag, behind the scenes of the UI it should only be working with tag IDs. To make things compatible with how things have been in WP, you'd still want a way to type in new tags, probably, since they're supposed to be kind of free-form (that's the whole tag philosophy, isn't it?).

Does that help?

comment:20 jhodgdon5 years ago

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

I'm going to do my best at a patch for this and #6593 tomorrow.

comment:21 jhodgdon5 years ago

  • Status changed from new to assigned

comment:22 Denis-de-Bernardy5 years ago

in case you're unfamiliar with it, you'll want to use the SCRIPT_DEBUG define in a wp-config.php file, to make use of the post.dev.js file rather than the mostly unreadable post.js file.

comment:23 jhodgdon5 years ago

Thanks! I am sure that will be helpful, and I wasn't aware of it.

comment:24 hakre5 years ago

I think there is another route that is doable: Do not lookup Tags by their Slugs but by their actual names. That will prevent the Bug of this ticket as well. The Bug only existst because it is done only for looking for the (guessed) slug.

The Bug would be prevented if the lookup would be done by only looking for the name not a (guessed) slug.

comment:25 jhodgdon5 years ago

The main Post Edit screen does look up by name. But even that still has a problem if the name has an apostrophe in it, so there is still some problem.

jhodgdon5 years ago

Fix for this problem

comment:26 jhodgdon5 years ago

  • Keywords has-patch duplicity added; needs-patch 2nd-opinion probably-wontfix removed
  • Owner changed from jhodgdon to ryan
  • Status changed from assigned to new
  • Summary changed from Quick edit creates new tag from existing tag-name sanitizing to a new tag-slug taking over the existing tag-name as name for the new tag. to Quick edit creates new tag from existing tag-name

All that worry. It turns out the reason this bug was happening was that somewhere in WP it assumed there were no spaces in the tags field, but the tags field had spaces.

So when the quick edit went to be saved, WP was exploding on commas, and the 2nd and subsequent terms started with spaces.

When WP then tried to find the tags in the DB to see if they already existed, the name with a leading space didn't match the name in the DB. Ergo, must be a new term.

The patch I just added simply trims the term name before trying to find it in the DB. I don't think this is a bad idea ever. Terms shouldn't start or end with spaces, and I think that when WP creates new terms, the spaces get stripped anyway. Not sure about that? Anyway, this fixes the problem. No more duplicate tags on quick edit.

Also, just as a note, the Quick Edit stuff that saves the tags is going through the same code as full Post Edit. So the fix to #6593 that I posted a few minutes back also fixes apostrophe issues here that probably existed.

comment:27 hakre5 years ago

  • Keywords tested added

Passed that. Because of not finding with Name, it creates new based on slug. Okay, I can follow.

Tested your patch, it works''' No new tags created.

comment:28 ryan5 years ago

Trim $term before creating the slug so we don't end up with trailing or leading dashes?

comment:29 hakre5 years ago

the term (not slug!) must not be created because it already exists and already was attached to the post object. the input must be trimmed to get the correct name sothat it can be looked up in the terms table as being already linked with the object.

comment:30 ryan5 years ago

Agreed. But if we trim term we should do it before creating the slug.

comment:31 jhodgdon5 years ago

Ah. I didn't test that -- assumed the sanitize function was smart enough to trim, but perhaps it isn't?

I'll fix that patch.

jhodgdon5 years ago

Better patch - trims before creating slugs (see comments below)

comment:32 jhodgdon5 years ago

How about this patch? Works for me (fixes the issue reported here), and trims before the slug gets created.

comment:33 ryan5 years ago

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

(In [11151]) Trim term before checking if it already exists. Props jhodgdon. fixes #9397

comment:34 hakre5 years ago

Second patch works as well for me. Test: OK, no additional category is created after submitting the testdata via the quickedit form.

Note: See TracTickets for help on using tickets.