Make WordPress Core

Opened 10 years ago

Closed 9 years ago

#27571 closed defect (bug) (fixed)

WP_Comment_Query::query() querying for too much data

Reported by: mkilian's profile mkilian Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.8.1
Component: Comments Keywords: needs-patch
Focuses: Cc:


Since [17667], the query() method of WP_Comment_Query not only fetches the comment data, but also the post data, if there are any constraints related to posts (like 'post_status' => 'publish' as used in the recent_comments widget, for example).

This doesn't scale well if you've a lot of large posts with a lot of comments, and it may cause the database to use an unnecessary large amount of memory or creating large internal temporar tables.

The attached patch should fix that (but it should be widely tested, in case any theme or plugin started to rely on the currently wrong behaviour).

Attachments (2)

comment.diff (352 bytes) - added by mkilian 10 years ago.
Diff against trunk ([27579])
27571.diff (1.5 KB) - added by wonderboymusic 9 years ago.

Download all attachments as: .zip

Change History (13)

10 years ago

Diff against trunk ([27579])

#1 @SergeyBiryukov
10 years ago

  • Keywords has-patch 4.0-early added
  • Milestone changed from Awaiting Review to Future Release

#2 @boonebgorges
9 years ago

  • Keywords needs-patch added; has-patch 4.0-early removed

Thanks for posting, mkilian. As you note, there are likely plugins etc that are depending on this data being present, and for this reason it's probably not going to work to make the simple change that you've suggested here.

A more robust fix would be something like this. Introduce a WP_Comment class, with a magic method __get() that will populate the post-related variables only if they're requested (and will respect the cache, etc). In WP_Comment_Query, only pull up comment-related data - as you suggest in your patch - and then convert it to WP_Comment objects before returning. This way, we preserve backward compatibility while improving performance in the way you've suggested.

9 years ago

#3 @wonderboymusic
9 years ago

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

27571.diff implements __get() and __isset() on WP_Comment to allow WP_Comment_Query to just return fields from $wpdb->comments (if 'fields' is set to '*') and then magically get $post properties if the comment has comment_post_ID > 0 and doesn't already have the property on itself.

#4 @wonderboymusic
9 years ago

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

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

9 years ago

#6 @wonderboymusic
9 years ago

  • Owner changed from wonderboymusic to boonebgorges

While you're futzing with everything else

#7 @wonderboymusic
9 years ago

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

In 34583:

Comments: add __get() and __set() to WP_Comment to provide BC for plugins that are grabbing post properties from the comment object due to an (unintended?) side effect of JOINing with the posts table in WP_Comment_Query, see [17667]. 'fields' used to default to *, so the JOIN would grab every field from both tables. #YOLO

Since [34310], these properties no longer exist. We can lazy load them for plugins with the magic methods.

Fixes #27571.

#8 @ocean90
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 @boonebgorges
9 years ago

  • Status changed from reopened to assigned

[34583] is also going to cause problems for people who are actually using this data inside of a comment loop. Referencing $comment->post_title etc for all 50 comments on a page is going to result in 50 database queries when no persistent cache is in use. At the very least, we should provide a flag that lets people decide that they want post data loaded.

Also, I think the fallback logic is too liberal here - $comment->foo shouldn't force the post to be loaded. We should have a whitelist of post properties, and lazy load only when those are requested.

I'll look at it if you don't get a chance before I do.

#10 @boonebgorges
9 years ago

In 34595:

Use correct property name when setting child comments.

This fixes a typo from [34546], uncovered by unrelated changes in [34583].

See #8071, #27571.

#11 @boonebgorges
9 years ago

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

In 34599:

Improve post field lazyloading for comments.

[34583] modified comment queries so that all post fields are no longer loaded
by default. Instead, they are loaded only when requested on individual comment
objects. This changeset improves that flow:

  • WP_Comment magic methods __isset() and __get() should only load the post when a post field is being requested.
  • The new update_comment_post_cache argument for WP_Comment_Query allows developers to specify that, when comments are queried, all of the posts matching those comments should be loaded into cache with a single DB hit. This parameter defaults to false, since typical comment queries are linked to a single post.

Fixes #27571.

Note: See TracTickets for help on using tickets.