WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 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 5 years ago.
17548-2.diff (2.3 KB) - added by joehoyle 5 years ago.
17548-2.2.diff (2.3 KB) - added by joehoyle 5 years ago.
17548.2.diff (2.4 KB) - added by markjaquith 5 years ago.
17548.3.diff (3.2 KB) - added by joehoyle 5 years ago.
17548.4.diff (3.2 KB) - added by joehoyle 5 years ago.
17548.5.diff (3.1 KB) - added by joehoyle 5 years ago.
17548.6.diff (3.1 KB) - added by joehoyle 5 years ago.
Fixed php docs with correct @since
17548.7.diff (4.5 KB) - added by joehoyle 5 years ago.
17548.7.2.diff (4.5 KB) - added by joehoyle 5 years ago.
recount.diff (2.4 KB) - added by momo360modena 5 years ago.
Recount all the time !
17548.8.diff (4.8 KB) - added by joehoyle 5 years ago.

Download all attachments as: .zip

Change History (52)

#1 @markjaquith
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Seems like trashing should update the count.

#2 @SergeyBiryukov
5 years ago

  • Keywords needs-patch added

@joehoyle
5 years ago

#3 @joehoyle
5 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 5 years ago by joehoyle (previous) (diff)

#4 @nacin
5 years ago

  • Milestone changed from Future Release to 3.3

A separate function would make more sense, yes.

#5 @joehoyle
5 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.

@joehoyle
5 years ago

@joehoyle
5 years ago

@markjaquith
5 years ago

#6 @markjaquith
5 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.

#7 @joehoyle
5 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.

@joehoyle
5 years ago

#8 @markjaquith
5 years ago

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

#9 @SergeyBiryukov
5 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?

@joehoyle
5 years ago

#10 @joehoyle
5 years ago

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

#11 @ryan
5 years ago

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

#12 @joehoyle
5 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 ?

@joehoyle
5 years ago

#13 @joehoyle
5 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.

@joehoyle
5 years ago

Fixed php docs with correct @since

#14 @ryan
5 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

#15 @nacin
5 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.

#16 @nacin
5 years ago

In [18809]:

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

#17 @joehoyle
5 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

#18 @ryan
5 years ago

Maybe == publish || == inherit?

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

#19 @ryan
5 years ago

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

#20 @joehoyle
5 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)

#21 @ryan
5 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().

#22 @nacin
5 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?

@joehoyle
5 years ago

@joehoyle
5 years ago

#23 @joehoyle
5 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).

#24 @ryan
5 years ago

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

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

#25 @nacin
5 years ago

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

#26 @ryan
5 years ago

The attachment count query is always returning 0 for me.

#27 @joehoyle
5 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?

#28 @ryan
5 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.

#29 @ryan
5 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 5 years ago by ryan (previous) (diff)

@momo360modena
5 years ago

Recount all the time !

#30 @momo360modena
5 years ago

Why not recount at every transition of the post?

#31 @momo360modena
5 years ago

  • Cc amaury@… added

#32 @ZaneMatthew
5 years ago

  • Cc zanematthew@… added

#33 @joehoyle
5 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.

#34 @ryan
5 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.

#35 @joehoyle
5 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.

@joehoyle
5 years ago

#36 @ryan
5 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	 

#37 @joehoyle
5 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

#38 @ryan
5 years ago

In [18932]:

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

#39 @ryan
5 years ago

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

#40 @nacin
5 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.