WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#17548 closed defect (bug) (fixed)

wp_trash_post() does not call update_post_count()

Reported by: joehoyle Owned by: ryan
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.2
Component: Taxonomy Keywords: has-patch dev-feedback needs-testing
Focuses: Cc:

Description

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

Attachments (12)

17548.diff (1.4 KB) - added by joehoyle 3 years ago.
17548-2.diff (2.3 KB) - added by joehoyle 3 years ago.
17548-2.2.diff (2.3 KB) - added by joehoyle 3 years ago.
17548.2.diff (2.4 KB) - added by markjaquith 3 years ago.
17548.3.diff (3.2 KB) - added by joehoyle 3 years ago.
17548.4.diff (3.2 KB) - added by joehoyle 3 years ago.
17548.5.diff (3.1 KB) - added by joehoyle 3 years ago.
17548.6.diff (3.1 KB) - added by joehoyle 3 years ago.
Fixed php docs with correct @since
17548.7.diff (4.5 KB) - added by joehoyle 3 years ago.
17548.7.2.diff (4.5 KB) - added by joehoyle 3 years ago.
recount.diff (2.4 KB) - added by momo360modena 3 years ago.
Recount all the time !
17548.8.diff (4.8 KB) - added by joehoyle 3 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 markjaquith3 years ago

  • Milestone changed from Awaiting Review to Future Release

Seems like trashing should update the count.

comment:2 SergeyBiryukov3 years ago

  • Keywords needs-patch added

joehoyle3 years ago

comment:3 joehoyle3 years ago

  • Cc joehoyle@… added
  • Keywords has-patch dev-feedback needs-testing added; needs-patch removed

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?

Last edited 3 years ago by joehoyle (previous) (diff)

comment:4 nacin3 years ago

  • Milestone changed from Future Release to 3.3

A separate function would make more sense, yes.

comment:5 joehoyle3 years ago

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.

joehoyle3 years ago

joehoyle3 years ago

markjaquith3 years ago

comment:6 markjaquith3 years ago

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.

comment:7 joehoyle3 years ago

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.

joehoyle3 years ago

comment:8 markjaquith3 years ago

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

comment:9 SergeyBiryukov3 years ago

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

joehoyle3 years ago

comment:10 joehoyle3 years ago

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

comment:11 ryan3 years ago

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

comment:12 joehoyle3 years ago

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 ?

joehoyle3 years ago

comment:13 joehoyle3 years ago

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.

joehoyle3 years ago

Fixed php docs with correct @since

comment:14 ryan3 years ago

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

In [18783]:

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

comment:15 nacin3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:16 nacin3 years ago

In [18809]:

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

comment:17 joehoyle3 years ago

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

comment:18 ryan3 years ago

Maybe == publish || == inherit?

Last edited 3 years ago by ryan (previous) (diff)

comment:19 ryan3 years ago

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

comment:20 joehoyle3 years ago

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)

comment:21 ryan3 years ago

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

comment:22 nacin3 years ago

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?

joehoyle3 years ago

joehoyle3 years ago

comment:23 joehoyle3 years ago

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

comment:24 ryan3 years ago

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

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

comment:25 nacin3 years ago

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

comment:26 ryan3 years ago

The attachment count query is always returning 0 for me.

comment:27 joehoyle3 years ago

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?

comment:28 ryan3 years ago

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.

comment:29 ryan3 years ago

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.

Last edited 3 years ago by ryan (previous) (diff)

momo360modena3 years ago

Recount all the time !

comment:30 momo360modena3 years ago

Why not recount at every transition of the post?

comment:31 momo360modena3 years ago

  • Cc amaury@… added

comment:32 ZaneMatthew3 years ago

  • Cc zanematthew@… added

comment:33 joehoyle3 years ago

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.

comment:34 ryan3 years ago

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.

comment:35 joehoyle3 years ago

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.

joehoyle3 years ago

comment:36 ryan3 years ago

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	 

comment:37 joehoyle3 years ago

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

comment:38 ryan3 years ago

In [18932]:

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

comment:39 ryan3 years ago

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

comment:40 nacin2 years ago

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.

Note: See TracTickets for help on using tickets.