Make WordPress Core

Opened 12 years ago

Closed 9 years ago

Last modified 9 years ago

#19903 closed enhancement (fixed)

wp_count_comments() and get_comments_count() both do SQL queries

Reported by: markjaquith's profile markjaquith Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.3.1
Component: Comments Keywords: has-patch
Focuses: performance Cc:

Description (last modified by markjaquith)

wp_count_comments() and get_comments_count() are similar functions, with a few differences:

  • Their results are returned in different formats:
    • wp_count_comments() returns an object of:
      • spam
      • approved
      • moderated
      • total_comments
      • trash
      • post-trashed
    • get_comment_count() returns an array of:
      • spam
      • approved
      • awaiting_moderation
      • total_comments
  • wp_count_comments() caches, but get_comment_count() always hits the database.
  • get_comment_count() is used nowhere in WordPress core.

I propose that we add 'trash' and 'post-trashed' reporting to get_comment_count(), and then have wp_count_comments() use get_comment_count() instead of doing its own SQL queries.

See #6884 and #19901

Attachments (2)

19903.diff (3.9 KB) - added by markjaquith 12 years ago.
19903.2.diff (3.1 KB) - added by wonderboymusic 9 years ago.

Download all attachments as: .zip

Change History (17)

#1 @FolioVision
12 years ago

Hi Mark,

Wouldn't it make better sense to simply get rid of get_comment_count and make wp_count_comments count in the optimised way (see ticket #19901)?

Keeping them both around is very confusing, especially as get_comment_count is not used in core. Duplicate functions have been a bit of a bugbear for us.

Alec

#2 @markjaquith
12 years ago

Wouldn't it make better sense to simply get rid of get_comment_count and make wp_count_comments count in the optimised way

Yeah, probably would. I think I was just thinking we needed both because I'd assumed that core needed the non-cached one, which isn't the case... it doesn't use it at all.

@markjaquith
12 years ago

#3 @markjaquith
12 years ago

Patch deprecates get_comment_count().

#4 @nacin
12 years ago

Could the deprecated version become a wrapper for wp_count_comments()? We'd simply need to rename the returned array keys.

#5 follow-up: @markjaquith
12 years ago

  • Description modified (diff)

Fixing the description to note that wp_count_comments() returns an object.

Could the deprecated version become a wrapper for wp_count_comments()? We'd simply need to rename the returned array keys.

Maybe. Unless people are using it to get "hot" data from the database. The cached counts might sometimes be wrong.

#6 in reply to: ↑ 5 @nacin
12 years ago

Replying to markjaquith:

Maybe. Unless people are using it to get "hot" data from the database. The cached counts might sometimes be wrong.

If getting hot data from the database is a valid use case, then we shouldn't deprecate it, even if the function isn't used by core. Otherwise, we should either ensure that the cache is invalidated, modify any cached data directly when wp_update_comment_count() is called, or decide that the non-persistence of the counts cache group is enough to ensure that the data is warm enough.

#7 @nacin
12 years ago

Or, as a fourth option, do what is proposed in the ticket description — keep both functions and have wp_count_comments() wrap and cache get_comments_count() (and have that function's SQL changed in #19901).

#8 @ryan
12 years ago

  • Milestone changed from 3.4 to Future Release

#9 @chriscct7
9 years ago

  • Focuses performance added
  • Keywords dev-feedback added

#10 @afercia
9 years ago

Came across this ticket while investigating for #32362. About get_comment_count() would recommend to fix it or deprecate it :) The thing is, it can return wrong results and it does that when you have "post-trashed" or "trashed" comments. Wondering also no one complained in the last 3 years so maybe +1 for deprecating it. Anyway, please let me do a quick example, dumping the 2 functions here's what I get in my local environment:

wp_count_comments() dump:

object(stdClass)#339 (6) {
  ["moderated"] => string(1) "6"
  ["approved"] => string(2) "34"
  ["post-trashed"] => string(1) "2"
  ["spam"] => string(2) "14"
  ["trash"] => string(1) "8"
  ["total_comments"] => int(54)
}

get_comment_count() dump:

array(4) {
  ["approved"] => string(2) "34"
  ["awaiting_moderation"] => string(1) "8"
  ["spam"] => string(2) "14"
  ["total_comments"] => int(64)
}

where get_comment_count() returns a wrong value for "awaiting_moderation". The "total_comments" is correct but includes also "post-trashed" and "trashed", that's inconsistent with what wp_count_comments() does.

get_comment_count() uses a switch statement which does loose comparison. Possible values for 'comment_approved' are: '0', '1', 'post-trashed', 'spam', 'trash' in this order.
In PHP, 0 == 'string' returns true. Thus, case 0 will catch all the comments that are '0', 'post-trashed' and 'trash' and the value returned for "awaiting_moderation" will be the last one which overwrites the previous ones. Will work only if you don't have post-trashed or trashed comments. Not arguing, I can understand this is probably for historical reasons since those two statuses were added in a second time and maybe something was missed on the road, type-checking included ;)

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


9 years ago

#12 @afercia
9 years ago

get_comment_count() type-checking was fixed in 33806.

#13 @wonderboymusic
9 years ago

  • Keywords has-patch added; dev-feedback removed
  • Milestone changed from Future Release to 4.4

#14 @wonderboymusic
9 years ago

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

In 33822:

Comments: wp_count_comments() can use get_comment_count() internally to makes its DB query, provided that get_comment_count() returns more properties.

Adds/updates unit tests. There were zero (0) unit tests for wp_count_comments().

Fixes #19903.

#15 @wonderboymusic
9 years ago

In 33823:

After [33822], really add those unit tests for wp_count_comments().

See #19903.

Note: See TracTickets for help on using tickets.