WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 6 months ago

Last modified 6 months ago

#19372 closed task (blessed) (fixed)

Don't call wp_count_comments() when we don't need to

Reported by: johnbillion Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: minor Version: 3.2
Component: Comments Keywords: has-patch
Focuses: performance Cc:

Description

Comments get counted on every admin screen with wp_count_comments(). We can skip a database call by not counting them if the user doesn't have the edit_posts capability.


Side note: With a persistent object cache it's possible to have admin screens with zero database queries with this patch applied. As an exercise you could write a database class that lazy loads MySQL and you'll get an admin screen that doesn't load MySQL. Almost completely pointless, but interesting nonetheless.

Attachments (2)

19372.patch (753 bytes) - added by johnbillion 4 years ago.
19372.1.patch (1.3 KB) - added by bordoni 6 months ago.
Updated to 4.4 + More readable code.

Download all attachments as: .zip

Change History (37)

@johnbillion
4 years ago

#1 @johnbillion
4 years ago

  • Keywords has-patch added

Patch

#2 @scribu
4 years ago

I think 'moderate_comments' would be a more appropriate capability to check. Related: #12104

#3 @ocean90
4 years ago

Related: #11409, #14751

Last edited 4 years ago by ocean90 (previous) (diff)

#4 @nacin
2 years ago

  • Component changed from Performance to Comments
  • Focuses performance added

#5 @johnbillion
12 months ago

#32366 was marked as a duplicate.

#6 @justindocanto
12 months ago

The proposed solution here does not fix what I mentioned in #32366.

Yes, both mention that it shouldn't be called when it's not needed, but the scenarios are completely different.

If I'm an admin and can 'edit_posts' Im still going to have the problem mentioned in #32366 because I'm still querying 5,000,000 comments on every single page load.

I think this ticket needs a better way to disable the function from being called OR we leave my ticket open and create another solution for the problem I brought up since this solution does not address it.

#7 @wonderboymusic
7 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Awaiting Review to 4.4

@justindocanto - this ticket suffices. We are attacking comments hard this release, will look at your issue with this one.

#8 @wonderboymusic
7 months ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

#9 @DrewAPicture
7 months ago

@wonderboymusic: What's the word on a new patch here?

#10 @wonderboymusic
7 months ago

  • Type changed from enhancement to task (blessed)

this just needs an hour of my time at some point

@bordoni
6 months ago

Updated to 4.4 + More readable code.

#11 @bordoni
6 months ago

  • Keywords has-patch added; needs-patch removed

Don't know if it helps or not, just updated the Patch for 4.4;

#12 @justindocanto
6 months ago

Would like to point out that the proposed patch still does not address the issue mentioned in #32366 which was merged into this issue.

With this patch, a user who can edit posts still has the issue of WP querying potentially millions of comments on every single page load and there is no way to disable it other than editing core files. There should be a 2nd check of some kind. Some sort of check if the user has flagged for this to be disabled...

e.g.

if ( current_user_can( 'edit_posts' ) && IF_USER_HAS_NOT_DISABLED_THIS ) {


This ticket was mentioned in Slack in #core by bordoni. View the logs.


6 months ago

#14 @sboisvert
6 months ago

The issue #32366 solves (slow query on admin) isn't solved by the latest patch.
What would be nice is to be able to short circuit this with a filter. Either to ignore this completely or to enable others to add a layer of caching in wp_count_comments.
Would people be open to putting a filter in there or another way of bypassing/fixing the potentially slow query?

#15 @bordoni
6 months ago

I think this problem is better addressed as a something on the function since there are other places on the Administration that WP will count comments.

#16 follow-up: @boonebgorges
6 months ago

There is already a filter in the function that lets you disable the call to get_comment_count(). https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/comment.php?marks=1725-1727#L1694

#17 @justindocanto
6 months ago

@bordoni - I disagree. As mentioned in #32366, there are ways to disable almost all of those occurrences (e.g., the comments menu item in the left admin menu, the comments column in the 'all posts' screen', etc.) have ways to disable them in a way that also stops the associated queries from happening.

The code in discussion here, in the top admin menu, does not have a way to disable it and that is why I keep pushing for the issue in #32366 (some sort of way to disable it other than 'user can edit posts') to be considered, since it got merged.

#18 follow-up: @bordoni
6 months ago

After I've checked the code I saw that it's possible to fix the problem by adding a filter to wp_count_comments.
Keep in mind that you might need to add a few more conditionals to make it work.

<?php
function filter_comments_counter_admin( $original, $post_id ){
    if ( is_admin() ) {
        return array(
                'approved'            => 0,
                'awaiting_moderation' => 0,
                'spam'                => 0,
                'trash'               => 0,
                'post-trashed'        => 0,
                'total_comments'      => 0,
                'all'                 => 0,
        );
    }
}
add_filter( 'wp_count_comments' 'filter_comments_counter_admin' );

As @boonebgorges said.

Last edited 6 months ago by bordoni (previous) (diff)

#19 @sboisvert
6 months ago

@boonebgorges My apologies, I was reading the get_comment_count() code by mistake. Thanks for pointing it out. @bordoni is correct that it's easy to implement a solution using that filter.

#20 in reply to: ↑ 16 ; follow-up: @justindocanto
6 months ago

Replying to boonebgorges:

There is already a filter in the function that lets you disable the call to get_comment_count(). https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/comment.php?marks=1725-1727#L1694

This functionality uses wp_count_comments() not get_comment_count().

The query that functionality runs is $count = $wpdb->get_results( "SELECT comment_approved, COUNT( * ) AS num_comments FROM {$wpdb->comments} {$where} GROUP BY comment_approved", ARRAY_A ); which is a resource intensive query to run on every single page view when you have a large amount of comments.

#21 in reply to: ↑ 20 ; follow-up: @boonebgorges
6 months ago

Replying to justindocanto:

Replying to boonebgorges:

There is already a filter in the function that lets you disable the call to get_comment_count(). https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/comment.php?marks=1725-1727#L1694

This functionality uses wp_count_comments() not get_comment_count().

The query that functionality runs is $count = $wpdb->get_results( "SELECT comment_approved, COUNT( * ) AS num_comments FROM {$wpdb->comments} {$where} GROUP BY comment_approved", ARRAY_A ); which is a resource intensive query to run on every single page view when you have a large amount of comments.

Please look more carefully at the link I provided. get_comment_count() is where the SQL query lives. wp_count_comments() is a wrapper that provides caching, as well as a filter for short-circuiting the call to get_comment_count(). I concede that it's not a very fine-grained filter, but it will allow for workarounds like the one described by @bordoni in 18.

#22 in reply to: ↑ 18 @justindocanto
6 months ago

@bordoni - I understand that this would stop the query, but 1) Also stop the function in the entire WordPress area and 2) you would have a broken 'pending comment count' in the top admin menu.

That suggestion is more of a duck tape job and not a proper solution. There should be a way to remove the item from the menu that also stops the query and allows the function to run anywhere else it is used.

#23 follow-up: @bordoni
6 months ago

@justindocanto the point is, the solution I provided, as I said requires some polishing around the edges but it's doable, no need for an extra filter there.

#24 in reply to: ↑ 21 @justindocanto
6 months ago

@boonebgorges I disagree. They are similar, but not the same function. See line 1738 within wp_count_comments(). This is the query that runs that causes slow queries.

#26 in reply to: ↑ 23 @justindocanto
6 months ago

@bordoni I disagree. Even if you hacked together something to only disable it when it runs on wp-admin/menu.php you would still have a broken pending comment count because the code blindly uses $awaiting_mod.

Again, the idea of stopping the wp_comment_count query is a duck tape solution, and not a proper answer to the overall issue detailed on #32366.

There needs to be a way to disable this menu item, and it's associated query. The easiest answer is a simple flag.

#27 follow-up: @bordoni
6 months ago

@justindocanto It's not a duck tape solution because that's how it's currently dealt with, the label for comments is always there, but if you have 0 comments it will be hidden by CSS.

On the file /src/wp-admin/css/admin-menu.css there is a ref to:

#adminmenu li span.count-0 {
    display: none;
}

You just got to add a new css rule somewhere to prevent it from showing up on the admin menu, maybe:

#wpadminbar #wp-admin-bar-comments .count-0 {
    display: none;
}

#28 in reply to: ↑ 27 @justindocanto
6 months ago

@bordoni Again - I disagree.

To do your method, a user would have to figure out a way to...

1) Figure out a way to only trigger your wp_count_comments filter when being used in /wp-admin/menu.php and send the function false information

2) This then depends on a css rule, meant to hide the pending comment count bubble when theres 0 pending comment, irregardless if there ARE pending comments.

1 and 2 are both using inaccurate info to trick WP to do what you want. That's a hack to me, not a solution. Other menu items have ways to disable them. I don't understand why we wouldn't want a way to properly disable this as well.

#29 @boonebgorges
6 months ago

@justindocanto I'm not opposed to a more explicit flag. I think what this flag would do is disable the display of the comment count within the Comments menu item, *not* disable the menu item altogether. No other top-level menu items can be disabled via filter at the moment, so I don't see a reason to introduce this functionality here. (Coming up with a more sane method for registering and rendering menus would be nice, but is way beyond the scope of this ticket.) I'd be glad to see a patch that does this.

As for whether the wp_count_comments filter is a "hack" for this purpose: Yes, of course it is. I don't think anyone is defending it as a robust API. My only point in bringing it up in the context of this ticket was that someone said we needed a filter at the level of the wp_count_comments filter, and I wanted to point out that one already exists there. This can be used as a workaround until a more robust solution is in place.

19372.1.patch should go in either way.

#30 @justindocanto
6 months ago

@boonebgorges Alright, I can get behind that. I'll get started on figuring out a way to use that function only on the menu file. Thanks everybody.

Last edited 6 months ago by justindocanto (previous) (diff)

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


6 months ago

#32 follow-up: @boonebgorges
6 months ago

A new filter is not going to happen for 4.4. If people want it (and I think it's a decent idea), please open a new ticket.

#33 @boonebgorges
6 months ago

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

In 35691:

Eliminate a db query when building the admin menu for non-privileged users.

Users who cannot edit_posts do not see the Comments nav item anyway, so
don't bother running a query that gets a comment count to display in the menu.

Props bordoni, johnbillion.
Fixes #19372.

#34 in reply to: ↑ 32 ; follow-up: @justindocanto
6 months ago

Replying to boonebgorges:

A new filter is not going to happen for 4.4. If people want it (and I think it's a decent idea), please open a new ticket.

I did. Please reopen #32366.

#35 in reply to: ↑ 34 @boonebgorges
6 months ago

Replying to justindocanto:

Replying to boonebgorges:

A new filter is not going to happen for 4.4. If people want it (and I think it's a decent idea), please open a new ticket.

I did. Please reopen #32366.

Reopened.

Note: See TracTickets for help on using tickets.