Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#27952 closed defect (bug) (fixed)

get_blog_details()->post_count Does not subtract deleted posts

Reported by: gsmcnamara's profile GSMcNamara Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.0 Priority: normal
Severity: minor Version: 3.8.1
Component: Networks and Sites Keywords: good-first-bug has-patch commit
Focuses: multisite Cc:

Description

The number returned for the post_count attribute is correct when posts are created, but does not decrease when posts are deleted.

Attachments (5)

27952.patch (650 bytes) - added by 5um17 11 years ago.
27952.1.diff (1.2 KB) - added by midxcat 11 years ago.
27952.2.diff (2.6 KB) - added by strangerstudios 11 years ago.
Fixing update_post_count for multisite.
27952.diff (2.9 KB) - added by wonderboymusic 10 years ago.
27952.3.diff (476 bytes) - added by pento 10 years ago.

Download all attachments as: .zip

Change History (20)

#1 @johnbillion
11 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from normal to minor

Confirmed. The update_posts_count() function is only hooked into the publish_post action. It should probably be hooked into delete_post and transition_post_status too in order to catch all relevant changes.

@5um17
11 years ago

#2 @5um17
11 years ago

Is there any better way than above patch ?

#3 @travisnorthcutt
11 years ago

  • Keywords has-patch added; needs-patch removed

#4 @SergeyBiryukov
11 years ago

  • Milestone changed from Future Release to 4.0

@midxcat
11 years ago

#5 @midxcat
11 years ago

Since publish_post and delete_post hooks are called in transition_post_status, I think it is enough to hook only on transition_post_status. And comments for the function should be updated.

#6 @strangerstudios
11 years ago

I just ran a test on 3.9.1 and when you force delete a post (and thus skip the trash status) the transition_post_status hook is NOT fired. I don't believe there is a way to delete without trashing first in the vanilla admin, but you can with a function call like this:

wp_delete_post( 2855, true); //setting $force_delete to true

So we will still want to hook into post_delete to catch these cases I'd think.

Also, the count will only be changing if the status is changing to or from "publish" (or being force deleted). The transition_post_status hook passes $new_status, $old_status, and $post as params. If we did a test if($new_status == "publish" || $old_status == "publish"), we could avoid resetting the count (and what could be a decently long query on very large sites) if the status was changing from draft to trash or something like this.

If this makes sense, I can write a new patch that keeps the post_delete hook in there and possibly updated the function to optionally take $new_status and $old_status params to test them before updating the count.

UPDATE: Hey, MidxCat, I'm at WordCamp Philly with Nacin and talking about this one. :)

I'm told transition_post_status will also fire if the status is "chaning" from "publish" to "publish". So we'll handle that case to NOT update the count if the status remains "publish". Working on a patch right now.

Last edited 11 years ago by strangerstudios (previous) (diff)

#7 @strangerstudios
11 years ago

Okay. Attached a new diff. Changes:

I left the update_posts_count() function in ms-functions.php alone, except to change the wording of the comment. It is slightly different from the first patch but I think reflects what this latest patch does.

I added two new functions to ms-blogs.php mimicking the _update_blog_date_on_post_publish() and _update_blog_date_on_post_delete() functions. I wouldn't want to add new functions here if we didn't have to, but we need to do something like this so we can check if the status is really changing. Added these two functions also has the advantages of having clearer code and preserving the update_posts_count() function to simply update the count with no checks if people are using the function that way.

  1. _update_posts_count_on_delete($post_id) now fires on post_delete. If the post being deleted is not in "publish" status, we don't update the count since deleting a draft/etc wouldn't affect the count.
  1. _update_posts_count_on_transition_post_status($new_status, $old_status) now fires on transition_post_status. If the post statuses are the same or neither of them is "publish", we don't update the count since this change shouldn't affect the count.

I tested on my multisite dev and it successfully updated the count when posts were created, deleted, updated, etc, and didn't update the count in the new cases where we don't. This will avoid potentially slow queries on sites with lots of published posts.

If adding so much code for this feels icky, I'd suggest another patch that calls update_posts_count() on post_delete and save_post (might as well use this since we wouldn't be checking the statuses anyway) with an update to the comment RE when update_posts_count() is called.

@strangerstudios
11 years ago

Fixing update_post_count for multisite.

#8 follow-up: @wonderboymusic
10 years ago

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

In 28835:

get_blog_details()->post_count should update on more actions than just publish_post.

Adds unit test.

Props 5um17, midxcat, strangerstudios.
Fixes #27952.

#9 in reply to: ↑ 8 @strangerstudios
10 years ago

Looks good to me.

Replying to wonderboymusic:

In 28835:

get_blog_details()->post_count should update on more actions than just publish_post.

Adds unit test.

Props 5um17, midxcat, strangerstudios.
Fixes #27952.

#10 @nacin
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

get_post_field() is going to be a bit slower than get_post( $post_id )->post_status because get_post_field() runs things through sanitize_post_field().

@pento
10 years ago

#11 @pento
10 years ago

attachment:27952.3.diff switches to using get_post() instead of get_post_field().

#12 @nacin
10 years ago

  • Keywords commit added

27952.3.diff looks good.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

#14 @SergeyBiryukov
10 years ago

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

In 29667:

Check $post->post_status directly in _update_posts_count_on_delete() for better performance.

props pento.
fixes #27952.

#15 @helen
10 years ago

#15424 was marked as a duplicate.

Note: See TracTickets for help on using tickets.