WP_Comment_Query::query() querying for too much data

[mkilian]

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

[mkilian]

Diff against trunk ([27579])

[boonebgorges]

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.

[wonderboymusic]

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.

[wonderboymusic]

While you're futzing with everything else

[wonderboymusic]

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.

[ocean90]

[34583] is causing unit test failures: ​https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/82275286

[boonebgorges]

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

[boonebgorges]

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.

[boonebgorges]

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.