wp_trash_post() does not call update_post_count()

[joehoyle]

Calling wp_trash_post() on either a custom post type of a post does not update the term counts until it has been it is "permanently deleted", however _update_post_term_count() (which is the default "update_count_callback" for tags and categories) calculates the term coint based of published posts. This means if you trash hundreds of posts - without emptying the trash, the term counts become way out.

To me, it would make sense for wp_trash_post() to call update_term_count() if the "update_count_callback" is "_update_post_term_count()" (as we know it will be calculating term count sbased off published status)

I can create a patch if this is accepted

[markjaquith]

Seems like trashing should update the count.

[joehoyle]

I have added a patch to call update_post_term_count() on transition_post_status() (but only taxonomies with an update_count_callback defined. This will also catch items moved to private etc (which it will not currently).

Not sure if it would be best to split this into a function, and add it via add_filter( 'transition_post_status', 'wp_update_term_count_on_transition_status' ) or something?

[nacin]

A separate function would make more sense, yes.

[joehoyle]

Added patch with it in a separate function. I should clarify when this is needed:

When you update / trash a post with a custom taxonomy. As wp_set_object_terms is never called for the custom taxonomy, the term counts are not updated. Tags / Categories are always keps in sync, as wp_insert_post calls wp_set_post_categories and wp_set_post_tags. It only calls wp_set_post_terms if you pass an array of terms to be added to the post (which wp_trash_post / removing all terms will not do).

Though the attached patch only covers this scenario, it will mean whenever a post's status is updated, terms counts will be kept in sync - this isn't true as things stand at the moment.

[markjaquith]

Our default update_count_callback is kind of lame. It doesn't exclude trashed posts. so in addition to this patch, CPTs need to specify a better callback. Maybe we can update our default to at least exclude trashed posts.

[joehoyle]

I have added a new patch to improve the default update_count_callback, by doing an INNER JOIN on the posts table to exclude trashed posts.

The generated query becomes:

SELECT COUNT(*) FROM wp_term_relationships INNER JOIN wp_posts ON wp_term_relationships.object_id = wp_posts.ID WHERE wp_term_relationships.term_taxonomy_id = 3 AND wp_posts.post_status != 'trash'

Again, to reproduce this issue - register a custom taxonomy and trash a post with a term assigned from that taxonomy. The count of the term taxonomy will not update.

[markjaquith]

I'm +1 on having the default callback exclude trashed posts. Let's have some others weigh in.

[SergeyBiryukov]

@since 2.3.0 for _update_term_count_on_transition_post_status() looks like a typo. Shouldn't it be @since 3.3.0?

[joehoyle]

Added new patch with @since typo fixes (Mark did this, but presuming it was a typo)

[ryan]

Why status != trash instead of == public? Could wp_update_term_count_now() simply call _update_post_term_count() in the else block?

[joehoyle]

Yup, that sounds ok, I put != trash as it was just a direct reply "dont include trash". Why was the default there in the first place? (as opposed to always calling _update_post_term_count ?

[joehoyle]

Added patch for wp_update_term_count_now () default to calling _update_post_term_count(), which will take care of only including published posts, calling actions etc.

[joehoyle]

Fixed php docs with correct @since

[ryan]

In [18783]:

Count only published posts when updating term counts. Fire term count updates on transition_post_status. Props joehoyle. fixes #17548

[nacin]

Looks like this broke Matt's community tags plugin, which puts tags on attachments. Attachments have a post_status of inherit.

Reverting immediately, since this is (very) destructive. We can still look into a solution, whether it's attachment-aware or simply does != trash. Might need to tweak _update_post_term_count() or revert that out.

[nacin]

In [18809]:

Revert [18783], as it breaks term relationship counts for attachment taxonomies. see #17548.

[joehoyle]

Should the behavior be: Posts count towards count if there status is inherit and their parent is published?

We could put this into the default term count SQL query if so.

If not, change to != trash and if a dev wants somethign else they specify a custom update_term_count_callback

[ryan]

Maybe == publish || == inherit?

[ryan]

Note that this means _update_post_term_count() has always been broken for attachments.

[joehoyle]

Maybe == publish || == inherit?

If an attachment's parent is draft, should it up the count though? I was presuming it should not...

Note that this means _update_post_term_count() has always been broken for attachments.

I don't think so - as attachments used the old default (which just counts posts in a tt_id regardless of status), then attachments would have counted towards the post_count (of course, thats providing post_tag is added to the attachment post type). (unless you are referring to that as broken)

[ryan]

I don't know if the more complex query is worth the relatively rare situation where the parent is not publish, and the even rarer case of a taxonomy added to attachments.

And, true, this would not have been broken for attachments before unless they registered _update_post_term_count().

[nacin]

We can detect when attachments have a taxonomy attached to them. And even which taxonomies. I don't mind 'inherit', but maybe that'd be a way to avoid the more complex query most of the time?

[joehoyle]

I have updated the patch to account for attachments, which does a sub query if the term's taxonomy is registered for attachments.

This does not fix Matt's plugin, as he isn't adding the attachment post_type to post_tag (or any other taxonomy). As the tax isn't added to the post type (which I believe it should), when tags are added to the attachment, it's doesn't fire the more complex query.

Also, Matt's plugin registers a new taxonomy "people", however it doesn't look like this taxonomy is used anywhere. Suggested tags are stored as post meta, and when they are approved, they get added as whatever taxonomy you select (post_tag, category or post_format).

Once I added register_taxonomy_for_object_type( 'post_tag', 'attachment' ); to matt's plugin, term counts accounted for attachments (which used the complex query).

[ryan]

That looks workable. I'll make some time to test with Community Tags soon.

​http://wordpress.org/extend/plugins/matts-community-tags/

[nacin]

I'll see what other kind of code Matt gas running here as well.

[ryan]

The attachment count query is always returning 0 for me.

[joehoyle]

The attachment count query is always returning 0 for me.

Is this for post_tag? If so, has post_tag been registered for attachment? It's a pretty basic query I added in the modified patch, do you see any potential problems with it?

[ryan]

The problem is that the object types are array( 'attachment:image', 'attachment:video', 'attachment:audio' ), This throws off the in_array() check and the post_type IN part of the query. I guess we need to trim :* off the end of the types.

[ryan]

Do we even need to query against post_type? If a taxonomy is registered against multiple post types, then the count is going to be stomped back and forth with the last post_type updated being reflected in the count. The count is not stored per post_type.

[momo360modena]

Recount all the time !

[momo360modena]

Why not recount at every transition of the post?

[joehoyle]

The problem is that the object types are array( 'attachment:image', 'attachment:video', 'attachment:audio' ), This throws off the in_array() check and the post_type IN part of the query. I guess we need to trim :* off the end of the types.

The object types are not even registered on a taxonomy that is assigned terms with matt's plugin. array( 'attachment:image', 'attachment:video', 'attachment:audio' ) is registered for the People taxonomy, but it appears Matt's plugin does not even assign terms to that taxonomy (yet at least).

If it were adding attachment:image etc to post_tag / category then the patch would not work, however from what I see attachment:image is not even a valid post type. register_taxonomy_for_object_type( 'post_tag', 'attachment:image' ); returns false, because it does not recognise it as a registered post type, so why are we trying to fix this issue? It seems to me Matt's plugin is currently very rough round the edges and we are trying to patch odd cases (such as "attachment:image") that are not even registered for the taxonomy we are testing against.

Do we even need to query against post_type?

Probably not, but I thought it would be better to be more specific and only run against registered post types for that taxonomy. Again, why are we trying to fix "attachment:image", unless I have missed something, "attachment:image" is not a default registered post type / anything special? There is no spec for ":image, :audio" etc on post types, It's just something matt wrote.

[ryan]

We're stuck with supporting attachment:*. It may be made more first class in the future, but for now we just have to avoid breaking plugins.

[joehoyle]

Ok, added a patch to support foo:* post types (which will catch attachment:* etc). Let me know if this is too ambiguous.

To test this, I had to assign an attachment to a 'person' taxonomy term from matt's plugin.

[ryan]

EXPLAIN SELECT COUNT(*) FROM wp_trunk_term_relationships, wp_trunk_posts p1 WHERE p1.ID = wp_trunk_term_relationships.object_id AND ( post_status = 'publish' OR ( post_status = 'inherit' AND post_parent > 0 AND ( SELECT post_status FROM wp_trunk_posts WHERE ID = p1.post_parent ) = 'publish' ) ) AND post_type IN ('attachment') AND term_taxonomy_id = 49

id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
1	PRIMARY	wp_trunk_term_relationships	ref	PRIMARY,term_taxonomy_id	term_taxonomy_id	8	const	1	 
1	PRIMARY	p1	eq_ref	PRIMARY,type_status_date,post_parent	PRIMARY	8	wordpress.wp_trunk_term_relationships.object_id	1	Using where
2	DEPENDENT SUBQUERY	wp_trunk_posts	eq_ref	PRIMARY	PRIMARY	8	wordpress.p1.post_parent	1	 

[joehoyle]

Alternative query with JOIN:

SELECT COUNT(*) FROM wp_term_relationships, wp_posts p1 INNER JOIN wp_posts p2 ON p1.post_parent = p2.ID WHERE p1.ID = wp_term_relationships.object_id AND ( p1.post_status = 'publish' OR ( p1.post_status = 'inherit' AND p1.post_parent > 0 AND p2.post_status = 'publish' ) ) AND p1.post_type IN ('attachment') AND term_taxonomy_id = 11;

EXPLAIN:

1	SIMPLE	wp_term_relationships	ref	PRIMARY,term_taxonomy_id	term_taxonomy_id	8	const	1	
1	SIMPLE	p1	eq_ref	PRIMARY,type_status_date,post_parent	PRIMARY	8	wordpress-trunk.wp_term_relationships.object_id	1	Using where
1	SIMPLE	p2	eq_ref	PRIMARY	PRIMARY	8	wordpress-trunk.p1.post_parent	1	Using where

[ryan]

In [18932]:

Count only published posts when updating term counts. Fire term count updates on transition_post_status. Props joehoyle. see #17548

[nacin]

In [19035]:

Separate non-post term counting from _update_post_term_count(). Introduce _update_generic_term_count(). The generic handler will be the default whenever the taxonomy is attached to an object type other than a post type (link, user). Otherwise the _update_post_term_count() handler will be the default. fixes #18986. see #17548.