Recent Comments Widget Should Use Comments API

[filosofo]

Currently it makes a direct database query.

Patch calls get_comments() instead.

[westi]

Relates to #10612

[westi]

Patch looks good.

I wonder if the old caching mech was trying to be clever for multiple widget instances so they all used the same cache hence always getting 15 comments

don't know if we want to preserve that behaviour.

It means that if you use the widget three times but with different comment counts its only one cache entry to store and invalidate.

[filosofo]

I think you're right about the intent. However, having multiple instances of the recent comments widget seems like too obscure a situation to justify the unnecessary queries.

I think that also explains why the max limit was set to 15. Without the caching cleverness, that limit is unnecessary, so I removed it from the second patch.

[azaozz]

Why remove the explicit caching? That will force blogs with persistent cache to run the rest of the code on almost every page load.

[filosofo]

Replying to azaozz:

Why remove the explicit caching? That will force blogs with persistent cache to run the rest of the code on almost every page load.

Ok, taking that into account, third patch does what the recent posts widget cache does: it uses the query API but caches the printed output.

Improvements:

• Still using API instead of direct db calls
• Likely to have to query less in most cases, as default 5 queried comments is less than previous 15 comments no matter what.
• Persistent cache of output instead of serialized comment objects means we're caching a smaller amount of data but interrupting sooner, thereby reducing processing and memory use.

[westi]

Replying to filosofo:

Replying to azaozz:

Why remove the explicit caching? That will force blogs with persistent cache to run the rest of the code on almost every page load.

Ok, taking that into account, third patch does what the recent posts widget cache does: it uses the query API but caches the printed output.

Improvements:

• Still using API instead of direct db calls
• Likely to have to query less in most cases, as default 5 queried comments is less than previous 15 comments no matter what.
• Persistent cache of output instead of serialized comment objects means we're caching a smaller amount of data but interrupting sooner, thereby reducing processing and memory use.

Looks good.

Do we really need to use output buffering and echos here?

Can we not just build up the html as a string and cache that?

[filosofo]

Replying to westi:

Do we really need to use output buffering and echos here?

No. I was desperately trying to get get_comments() in instead of querying the database directly, so to respond to the above criticism I just copied the recent posts widget almost exactly. (Recent posts, the calendar, and various chunks of the admin use the output buffering trick).

How about a patch that builds the HTML for recent comments as well as recent posts, so we're somewhat consistent?

[hakre]

Regarding Cache and multiple widgets please see this ticket which should contain a fix for the problem: #11580

[Denis-de-Bernardy]

widget_comments_use_api.10615.persistent_cache.diff is erroneous. the last call to wp_cache_add() is bogus (it's missing an argument), and it should be using wp_cache_set() instead.

[filosofo]

Whoops. However, aside from the missing argument it's the same as the recent posts widget. I'll see how the dust settles on that and imitate it for recent comments.

[rmccue]

No longer applies cleanly.

[nacin]

Just a thought -- custom comment types will show up in the widget, and plugins have no control over that currently unless they filter on the SQL itself. Using the API should allow for a plugin to filter that nicer.

[blepoxp]

Refreshed. Doesn't use output buffering. Uses get_comments()

[blepoxp]

This is basically the same patch as filosofo submitted but its built off of the current revision and it doesn't use output buffering. Comments / suggestions?

[blepoxp]

This diff actually puts the $cache var into cache... removes limit label in GUI as well

[zeo]

relates to #12904. Probably need refresh.

[westi]

Kicking the tires on this patch now.

[westi]

(In [14483]) Make the recent comments widget use the get_comments() api and cache the result. Fixes #10615 props filosofo and blepoxp.

[jshreve]

only show approved comments

[nacin]

(In [14679]) Don't show unapproved comments in comments widget. props jshreve, fixes #10615.