wp_count_comments() and get_comments_count() both do SQL queries

[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

[FolioVision]

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

[markjaquith]

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]

Patch deprecates get_comment_count().

[nacin]

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

[markjaquith]

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.

[nacin]

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.

[nacin]

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

[afercia]

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

[afercia]

get_comment_count() type-checking was fixed in 33806.

[wonderboymusic]

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.

[wonderboymusic]

In 33823:

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

See #19903.