WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#19903 new enhancement

wp_count_comments() and get_comments_count() both do SQL queries

Reported by: markjaquith Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3.1
Component: Comments Keywords:
Focuses: 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 (1)

19903.diff (3.9 KB) - added by markjaquith 2 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 FolioVision2 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

comment:2 markjaquith2 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.

markjaquith2 years ago

comment:3 markjaquith2 years ago

Patch deprecates get_comment_count().

comment:4 nacin2 years ago

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

comment:5 follow-up: markjaquith2 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.

comment:6 in reply to: ↑ 5 nacin2 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.

comment:7 nacin2 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).

comment:8 ryan2 years ago

  • Milestone changed from 3.4 to Future Release
Note: See TracTickets for help on using tickets.