Quick tag/category edit appears to be using filtered data

[jhodgdon]

In 2.7 bleeding [9597], when I go to either the Categories or Tags edit/list screen (wp-admin/edit-tags.php or wp-admin/categories.php), and try to edit a category/tag using the Quick Edit link, the tag/category name field is pre-filled with a display-filtered version of the tag/category name, rather than the full unfiltered tag/category name that is stored in the database.

When I click "edit" to edit the tag/category in full screen mode, I get the full unfiltered name, which is good.

I am not sure if this is true for Link Categories too (i.e. Quick Edit links from wp-admin/edit-link-categories.php), but I imagine it would be, so if someone fixes Tags and Categories Quick Edit, they should check on Link Categories as well.

[jhodgdon]

This is also true of Post/Page titles when you use Quick Edit on them from the Edit/List screen -- it is showing the display-filtered page/post title. Very dangerous -- I'm sure if I just wanted to update to Published, it would also lose my database-saved post title.

[azaozz]

(In [9606]) QE fixes: don't display-filter titles, show QE for newly added cats and tags, fixes #8146

[jhodgdon]

This needs to be reopened.

In [9621], it is fixed for post and page titles.

However, tags, categories, and link categories are still doing Quick Edit with the display-filtered names.

[azaozz]

Replying to jhodgdon:

In [9621], it is fixed for post and page titles.

However, tags, categories, and link categories are still doing Quick Edit with the display-filtered names.

Don't see them being filtered. There's no "curly" quotes or html entities, etc. Could you include some more info.

[jhodgdon]

My multilingual plugin filters tags, categories, etc. so that only one language is displayed at a time, though both appear in the tag name. For example, a particular tag might show up as "stuff" in English, and "cosas" in Spanish, with the tag name containing some language codes, as well as the words "stuff" and "cosas".

In [9621], my observation is that if I click "edit", I get the whole tag name, but if I click "quick edit", I only see "stuff" or "cosas", not the whole tag name with both languages and the language codes. So something is passing the tag name through filters before putting it into the quick edit block.

I haven't had time to debug this and find where it is in the code, but it's probably from the function get_term() in wp-includes/taxonomy.php, which has an argument $filter. If $filter is set to 'display', then all the standard plugin filters are called to alter the term name before returning it. If it is set to 'raw', then no filters are applied (which is what you need for editing, so you are editing the raw data from the database). Based on what I am observing, it is set to 'display' when the call to get_term() is made when setting up Quick Edit. That is incorrect.

Does that help?

[markjaquith]

This broke many rounded buttons for Safari. We need to keep that padding!

[jhodgdon]

Mark - was that intended for a different bug report? Doesn't seem related to this one...

[azaozz]

(In [9648]) Filter category and tag for editing when using QE, fixes #8146

[azaozz]

Replying to jhodgdon:

I haven't had time to debug this and find where it is in the code, but it's probably from the function get_term() in wp-includes/taxonomy.php, which has an argument $filter. If $filter is set to 'display', then all the standard plugin filters are called to alter the term name before returning it. If it is set to 'raw', then no filters are applied (which is what you need for editing, so you are editing the raw data from the database). Based on what I am observing, it is set to 'display' when the call to get_term() is made when setting up Quick Edit. That is incorrect.

They were already using the 'raw' filter as that's the default, but should be using the 'edit' filter like on the Edit pages.

[jhodgdon]

I am going to reopen again.

I just updated to [9649] and it is NOT fixed. Quick edit for tags and categories is still showing me a filtered category/tag name. Full edit is not a problem. They are not showing the same things.

I'll see if I can do a patch this time... I have about an hour to investigate...

[jhodgdon]

Well, I have at least identified the problem.

We're in function _cat_row in wp-admin/template.php .

The argument $category to this function is an object, which has already been filtered for display elsewhere, and is passed in to _cat_row.

The first thing that _cat_row does is to call get_category( $category ) [in wp-includes/category.php], which prompty calls get_term( $category ) [in wp-includes/taxonomy.php].

So about 10 lines down in get_term(), it says "Oh, I just received an object as input. I'll cache that." Note that it is caching a display-filtered object, not a raw-database-data object.

After the return from get_category/get_term, farther down in the _get_row() function, it calls get_category_to_edit( $id ), which calls get_category( $id ), which calls get_term( $id ), which finds the cached data it previously cached (same ID), and uses that instead of doing a database query to get the real data. So it just returns display-filtered data instead of raw database data, even though the filter type is set to 'raw' or 'edit'.

The issue is that get_term is assuming that the object it is receiving as input and caching is equivalent to raw database output. But in this case, someone has passed in an object that has been filtered, and it should NOT be cached as raw database output.

I think the solution might be just to remove that line from get_term that automatically caches any object passed in. But it might break something else? I do not know why it is there, but it seems wrong...

[jhodgdon]

OK, now I think I am going crazy. I removed all the add_to_cache statements. It still doesn't work right. I then put a bunch of debug statements in get_term. It is running this query:

SELECT t.*, tt.* FROM wp_terms AS t INNER JOIN wp_term_taxonomy AS tt ON t.term_id = tt.term_id WHERE tt.taxonomy = 'category' AND t.term_id = '6' LIMIT 1

When I run it in PHPMyAdmin, I can see that the name column is

[lang_en]Viewslang_en[lang_es]vistaslang_es

(which is unfiltered). But when I print out the result of the query from within WP, I get just "Views".

Decidedly odd. Also, when similar queries are run earlier on and later on to get all the terms,

SELECT t.*, tt.* FROM wp_terms AS t INNER JOIN wp_term_taxonomy AS tt ON t.term_id = tt.term_id WHERE tt.taxonomy IN ('category') ORDER BY t.name ASC

It finds the un-filtered name in the element whose term ID is 6.

What gives? This is just plain strange... am I missing something obvious? Not sure how to fix it if the database query is doing something strange.

[jhodgdon]

Oh yeah, forgot to metion that the exact same query

SELECT t.*, tt.* FROM wp_terms AS t INNER JOIN wp_term_taxonomy AS tt ON t.term_id = tt.term_id WHERE tt.taxonomy = 'category' AND t.term_id = '6' LIMIT 1

gives the raw, unfiltered result when it is run on the full Edit page (wp-admin/categories.php?action=edit&cat_ID=6)

Why would the same query in the same database give two different results, without having edited anything? I don't understand this. My debug statements printed out every single query run through $wp_db->query(), and there wasn't anything but those and similar SELECT queries that involved the wp_terms table...

[azaozz]

The problem could be that your plugin is using a low-level filter/action to show either the English or Spanish string. Don't think there's code in WordPress that would filter

[lang_en]Views[/lang_en][lang_es]vistas[/lang_es]

to only "Views".

Just tried a tag with the full string and it wasn't changed in any way, was able to both Edit and Quick Edit it without problems.

Perhaps we need to adjust the display filters for terms or even add another one. Tried using "term_name" and it seems it's working well, except it's not applied everywhere, so tags in the tag cloud and categories on the front end are not passed through it, but both tags and categories are using it on the Manage Posts page.

[jhodgdon]

Yes, of course my plugin is fitering -- that is the exact function of this plugin, to make it so that only one language is displayed at a time, while two or more are stored in the database for posts, tags, categories, etc. It is only because my plugin is filtering that I know there is a problem. You do not see the problem because you are not running a plugin that filters for display.

But data needed for editing should not be applying display filters. That has to work, and it does work for regular edit currently, just not quick edit. In my debugging, I also saw that, even putting debug prints right in the $wp_db->query() function, that query was pulling out different data when called from the quick edit setup than when called from the full edit setup. Can you explain that? I can't. I put a print right inside the $wp_db->query() function that printed out the query, as well as the results, and I got a different answer from the same query when run during quick edit setup and when run during full edit setup. Mystifying.

Because of the unstructured way that WP does filtering, my plugin has a large number of filters... I am gonig to try to narrow this down to a minimal subset and attach a simple display filtering plugin you can use for testing shortly.

[jhodgdon]

OK, I figured it out.

My plugin uses a filter on 'cat_rows' to make the display of the categories list on wp-admin/categories.php look nice. If you remove that filter, then the categories list doesn't look nice. This has been needed over the last many many versions of WordPress.

The issue now is that the hidden fields that are used for Quick Edit are built at the same time as the rest of the display. So they are also passed through the 'cat_rows' filter. Hence, they are filtered.

My debug prints from the database queries were also being passed through the same filter, by the way, which is why it looked like they were doing something very weird.

So, what needs to be done is to fix the categories, tags, etc. displays so they are properly display filtered in the first place and remove that brute force cat_rows filter call. In my plugin, I have filters 'the_category', 'the_category_rss', 'single_cat_title', 'list_cats', 'term_name', and 'category_name' (among others), but only 'cat_rows' seems to be affecting that particular page.

I will see about a patch...

[jhodgdon]

The easy fix seems to work, more or less: use get_category( $category, OBJECT, 'display') at the top of _cat_row(), and then remove the call to apply_filters('cat_rows') at the bottom of _cat_rows().

Not sure if you want to implement that or not...

[jhodgdon]

Hold off on that... actually it doesn't work, because the return from get_category is passed into the quick edit. I'll figure out a patch that does work.

[azaozz]

Don't think you will be able to use 'cat_rows' to filter that as it will always include the data for the quick edit. Would need to use a filter that plugs into the display filtering of get_category or get_term, something like 'term_name' but applied everywhere.

[jhodgdon]

Right. I am working on a fix though. It's not as simple as I though, because if you do dipslay fitlering on get_category, the next call to get_category ends up returning a reference to the same object, so $qe_data has display-filtered data in it. so the fix is uglier than it needs to be, sigh.

[jhodgdon]

Fix for the quick edit display filtering issues

[jhodgdon]

I've created a patch that works for me...

In my opinion, it's better to remove the cat_rows filters, as I've done in the patch, because if everything is properly filtered in the first place, they are unnecessary. But they were necessary in previous versions of WP, so a plugin that wants to be backwards compatible still needs to have them defined (or else do some complex version checking).

As I mentioned above, a simpler patch that just display-filters $category directly does not work, because when you try to get the $qe_data, it ends up returning a reference to the same PHP object. That is why the patch instead just uses sanitize_term_field to sanitize the name and description fields, before displaying them, rather than sanitizing the entire object. And why the sanitized fields have to be stored in separate variables, rather than just replacing the ->name and ->description fields in the $category object. I did try it without separate variables, and $qe_data ended up sanitized for display.

Anyway, there it is...

[jhodgdon]

One thought... since $qe_data does seem to be a reference to $category, I don't think it is actually edit-sanitized... Not sure about that, or how to test?

If that is correct, it might make sense to do a sanitize_term_field( $filter = 'edit') call before storing that data in the hidden fields. I am not running into any problems, but I might not have any problematic data in there.

[ryan]

See #8191

[ryan]

Patch adds a filter field to the term object and get_term() refuses to store objects that have been filtered.

[ryan]

(In [9737]) Don't cache filtered term objects. see #8146

[jhodgdon]

This patch needs some modification.

Although you are being careful not to cache incoming data that has been filtered, I am pretty sure wp_cache_add is storing a *reference* to the term object. Then later down in get_term it is applying filters to the same term object. So the next call will retrieve filtered data from the cache.

[jhodgdon]

Also, this patch will not fix the problem of quick editing. The only filtering done for the display (unless you apply the patch I submitted earlier) is the overall 'cat_rows' filter, which filters the display information as well as the quick edit data. They need to be separated (which is what my patch does).

[jhodgdon]

Just a note: My copy of "PHP in a Nutshell" says "From PHP 5 onward, objects are always handled as references.". So my comment about wp_cache_add storing a reference may be specific to PHP 5 (which is what my test server is running, where I concluded that the cache was behaving this way). I guess in PHP 4, that might not have been the case.

Anyway, assuming we want get_term and its caching to work in PHP 5, you would need to clone the object after retrieving it from the cache. The Nutshell book suggests something like this, which I'd put in get_term right after retrieval/adding to cache and before filtering:

$_term = clone $_term;

I'm not sure whether the clone keyword exists in PHP 4. ???

[jhodgdon]

And is this the only place in WP that caching is used and then the object returned is modified, assuming it's a copy of the data?

[ryan]

wp_clone()

[ryan]

I could reproduce #8191 in PHP 5 but not 4. Attached patch introduces wp_clone() and uses it when setting and getting objects to/from the cache. With that 8191 appears fixed.

[ryan]

(In [9740]) Use clone to break object refs when setting and getting cache. see #8146 #8191

[azaozz]

(In [9751]) Fix QE data filtering for tags and categories, props jhodgdon fixes #8146

[azaozz]

Changed the patch a bit to make it the same for tags, cats and link cats, seems to work well.

[jhodgdon]

Changeset [9751] looks mostly OK to me -- will test a bit later. Of course, now that [9740] and [9737] are in there, it might not have been necessary.

And why did you decide not to remove the overall cat_rows, tag_rows, and link_rows filtering at the end? That is just going to cause headaches in plugin compatibility.

The reason is that 'cat_rows' and the others were necessary in older WP versions, because there wasn't proper display filtering being applied to the categories lists. But now if you apply cat_rows, you will filter the quick edit data, which is problematic.

So how about reversing [9751] and instead applying a patch that removes 'cat_rows', 'link_rows', and 'tag_rows' overall filtering?

[jhodgdon]

Leaving those 'cat_rows' etc. filters in there means it is not fixed for anyone who was using those filters previously, because those filters were only being used in prior versions of WP for filtering displayed data, and now they are being applied to editable data (in the quick edit fields). So I am reopening this again.

I am also attaching a patch that reverts [9751] and does all the filtering in a more rational way (now that the underlying cache problem has been fixed), and also removes the 'cat_rows' filtering.

[jhodgdon]

Patch that rationalizes filtering in link cats, cats, and tags, and removes overall cat_rows and related filters

[jhodgdon]

Just a note that I didn't revert all the changes in [9751] in that patch, just the ones that didn't make sense any more (now that caching in get_term is fixed) inside wp-admin/includes/template.php. The changes to the other files should remain.

[ryan]

(In [9757]) Cat row filtering cleanup from jhodgdon. see #8146

[ryan]

We'll see if anyone bemoans the loss of the filters.

[jhodgdon]

It's highly likely that I was the only one using them. :)

[azaozz]

Replying to jhodgdon:

Leaving those 'cat_rows' etc. filters in there means it is not fixed for anyone who was using those filters previously, because those filters were only being used in prior versions of WP for filtering displayed data, and now they are being applied to editable data (in the quick edit fields). So I am reopening this again.

True, but that doesn't make any difference for most user cases and can easily be separated by a small regexp. I think these filters were meant for manipulating the entire table rows, like adding content, converting from table row to list, even replacing the entire HTML, etc. Not sure if any plugins are using them though.

I am also attaching a patch that reverts [9751] and does all the filtering in a more rational way (now that the underlying cache problem has been fixed), and also removes the 'cat_rows' filtering.

I like the previous solution too, however that reintroduces an inconsistency that was there. If we are applying display filters to all fields in _cat_row and link_cat_row, we probably should do the same for _tag_row too. So instead of just

$name = apply_filters( 'term_name', $tag->name );

it should be

$tag = sanitize_term($tag, 'post_tag');