Opened 4 years ago

Closed 3 years ago

#9500 closed defect (bug) (wontfix)

Object caching is incompatible with mutative the_posts filters.

Reported by: Antiarc Owned by: ryan
Priority: normal Milestone:
Component: Cache Version: 2.7.1
Severity: normal Keywords: has-patch needs-testing 2nd-opinion early
Cc:

Description

query.php runs something like:

  1. Get the posts
  2. Run the_posts filters
  3. Save $posts in object cache

The problem is that when you go to edit a post, get_post() checks the object cache for a post before pulling it from the DB. Thus, presume the following:

  1. We pull a post, with the content "foo".
  2. We run a the_posts hook, maybe with a regex that transforms "foo" into "foobar".
  3. $posts are saved in the object cache
  4. We go to edit a post. Post is pulled from the object cache, as "foobar" - not what the user entered into their post.
  5. We save the post without any editing. Post gets saved as "foobar".
  6. The post is pulled from the DB and the_posts is run. Our regex transforms it to "foobarbar", etc etc.

I know that transformative actions are generally done on the_content, but in this particular case, I absolutely have to be doing batched operations on all posts pulled (it's sending content off to a remote server for processing, and running that on the_content would generate N requests per page, rather than 1 request per page, incurring massive slowdown). The docs do not forbid transformation by the_posts, and I can't find a more appropriate hook to use, nor is there a hook to invalidate the contents of the object cache before editing happens.

This used to be correct, but was changed in r4517 - the commit message there is kind of bogus. I've attached a patch with the fix.

Attachments (2)

cache_fix.patch (475 bytes) - added by Antiarc 4 years ago.
9500.diff (808 bytes) - added by Denis-de-Bernardy 3 years ago.

Download all attachments as: .zip

Change History (15)

Antiarc4 years ago

comment:1   ryan4 years ago

  • Owner changed from anonymous to ryan
  • Keywords has-patch added
  • Keywords tested added
  • Milestone changed from Unassigned to 2.8

I imagine the goal was to also cache the results of that filter, so as to not run it twice.

the filter is only used once in WP, but there almost certainly are plugins that rely on it in a way or another.

imo, patch is a good idea, unless there's a very popular plugin whose behavior is changed by this.

comment:4   ryan4 years ago

  • Component changed from General to Cache
  • Keywords commit added

comment:6   ryan4 years ago

Looking at plugins to judge the impact. The current behavior is not good behavior, but it has been that way for awhile and bugs have a way of becoming interface.

maybe we should add an extra hook in there instead?

will this one get committed, or will it rot for a few years?

comment:9   ryan4 years ago

  • Keywords early added
  • Milestone changed from 2.8 to 2.9

patch still applies clean

  • Keywords needs-testing added; tested commit removed

I've uploaded an improved patch that ensures posts are sanitized before they're cached.

  • Keywords 2nd-opinion added

That being said, I'm 80% sure that this should get a wontfix: the more I look at it (I'm working on a query caching plugin, which could actually make use of that hook if WP had a few more in there), the more convinced I am that the current behavior is the correct one on this front.

I'm pretty certain that you should be using the loop_start or the the_post filter (both were introduced in 2.8) for whatever you are trying to do. Or maybe even hook into the wp hook directly...

  • Milestone 2.9 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I've gone back and forth on whether this should change. It's going to break certain scenarios either way. Let's call it wontfix.

Note: See TracTickets for help on using tickets.