humanmade / authorship

A modern approach to author attribution in WordPress.
GNU General Public License v3.0
66 stars 7 forks source link

Add WP_Cache to get_authors function #92

Closed addaeabeng closed 1 year ago

tomjn commented 2 years ago

This appears good, but I'd like someone who's worked with this plugin that isn't on our project to review it too. Once that's done, a 0.3.0 release might be necessary but we can always reference a specific commit at our end

mattheu commented 2 years ago

get_authors() does perform a user query each time, but as it's a query for a list of user IDs it's very fast. Caching complete WP_User objects risks stale data without comprehensive cache flushing when users are updated.

Yes, this the actual reason we flagged this for investigation. Hypothetically a page may have 10 posts shown, and get_authors is called 10 times. If they all have the exact same authors set for each, then this leads to the same identical query being run 10 times. This was flagged because we did an audit and were seeing these duplicate queries in Query Monitor. In our case we're actually doing some additional sorting of in user queries, which makes it less performant than a simple query by IDs.

Really I had hoped to avoid too aggressive caching here, and probably fine to just be be cached for the duration of the request. But understand the concerns. It's not so easy to clear the cache. Not sure how to work around this really.

johnbillion commented 2 years ago

The result could go into a static variable inside get_authors(), maybe an array keyed by post ID. Then it would just be cached for the duration of the request.

mattheu commented 2 years ago

Made an adjustment to this to cache the user query. I am keen to avoid keying this by ID as this only solves the issue of calling multiple times for the same post. And not the issue of having different posts with the same authors. (Admittedly this is more an issue for my dev environment where all posts are written by "admin" :) )

shadyvb commented 2 years ago

Still not keen on the change, this is aggressively caching the list of users, and not even in object cache, so consumers wouldn't be able to override the behavior, which isn't expected. User objects can change between different function calls, and this does not count for it.

I'd rather look at a way that is less aggressive, or to use filters to possibly enable consumers to do the caching on their end.

addaeabeng commented 2 years ago

After further discussion with @johnbillion will consider adding a pre filter to allow external code to manage caching

mattheu commented 1 year ago

I think we can close this. No longer a project need for this changes and quite a bit of pushback on this ticket.