#22223 closed defect (bug) (fixed)
Potential for post cache corruption since r21735
Reported by: | ryan | Owned by: | ryan |
---|---|---|---|
Milestone: | 3.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Cache API | Keywords: | |
Focuses: | Cc: |
Description
Since r21735, the potential for corrupting the cache with bad post objects is much higher. For example, when looping over posts that are not all for the same blog, posts that do not have a complete set of properties, posts that have extra properties, and dummy posts that don't actually exist in the db. All of these scenarios are proving common enough to be a problem.
Attachments (6)
Change History (26)
#2
@
11 years ago
The strictest way is to cache only if the object contains the core properties and only the core properties. If we do such a thing it'd be nice to skip the checks when calling get_post() in WP_Query::get_posts() since the objects there are safe.
#3
in reply to:
↑ 1
@
11 years ago
Replying to scribu:
Famous last words: we could just remove the
wp_cache_add()
call.
I've been thinking the same. If the post came from a previous get_post(), from WP_Query, or from another core function it will already be cached. If it didn't come from a core function it is very likely to be something we don't want to cache.
#4
follow-up:
↓ 6
@
11 years ago
I feel like we're getting in "is_a" territory, is there a compelling reason to have to have public props with defaults like the diff I just attached?
(is_a meaning it implements an expected interface - if it can't be missing properties)
#5
follow-up:
↓ 8
@
11 years ago
However, if both cache_results and split_the_query in WP_Query::get_posts() are false then the posts won't be added to the cache if get_post() doesn't wp_cache_add(). An environment using a persistent cache backend and a plugin such as advanced post cache that hasn't been updated to work with the split query would fit this scenario (cough, wordpress.com).
#6
in reply to:
↑ 4
@
11 years ago
Replying to wonderboymusic:
I feel like we're getting in "is_a" territory, is there a compelling reason to have to have public props with defaults like the diff I just attached?
(is_a meaning it implements an expected interface - if it can't be missing properties)
Since we decided to not use __get() for core properties, we should consider making these proper properties regardless of this ticket. Adding default while we're at it would certainly solve part of the problem, that of missing properties. There are still the other two issues -- garbage properties and post objects not intended for the current blog.
#7
@
11 years ago
Something like $post->filter = ‘no-cache’ might be useful for those situations where someone knows they are fudging a post object and don't want it to ever end up in cache.
#8
in reply to:
↑ 5
@
11 years ago
Replying to ryan:
However, if both cache_results and split_the_query in WP_Query::get_posts() are false then the posts won't be added to the cache if get_post() doesn't wp_cache_add(). An environment using a persistent cache backend and a plugin such as advanced post cache that hasn't been updated to work with the split query would fit this scenario (cough, wordpress.com).
This would be good, in part, because it would restore the 3.4 intent behind cache_results = false. get_post() in the array_map() would no longer be going behind its back and doing a bunch of wp_cache_add() calls anyway. But that would leave get_post() by post ID as the only way to get posts into cache. I suspect that passing objects to get_post() while rolling through The Loop is the main way posts are added to cache when cache_results = false, however.
#9
@
11 years ago
But that would leave get_post() by post ID as the only way to get posts into cache.
We need to make update_post_cache() work with a single post as well, then.
#10
follow-up:
↓ 12
@
11 years ago
Actually, I think setting the cache only when passing an ID to get_post() is the sane thing to do.
Beyond that, just let WP_Query handle the pre-caching, using update_post_caches().
#11
@
11 years ago
Note that cache corruption was possible in 3.4 with get_post( $post ). The difference in 3.5 is that get_post() can corrupt the cache with the global post. It'd be nice to address both, but just fixing the global post would be sufficient.
#12
in reply to:
↑ 10
;
follow-up:
↓ 13
@
11 years ago
Replying to scribu:
Actually, I think setting the cache only when passing an ID to get_post() is the sane thing to do.
It does fix all problems and does so by removing a single line of code.
#14
follow-up:
↓ 15
@
11 years ago
We might still want to use proper properties with defaults. We're seeing some homebrew objects that don't even have post_type set, resulting in warnings down the line and extra queries because the new magic get tries to fetch post_type from meta.
#15
in reply to:
↑ 14
@
11 years ago
Replying to ryan:
We might still want to use proper properties with defaults. We're seeing some homebrew objects that don't even have post_type set, resulting in warnings down the line and extra queries because the new magic get tries to fetch post_type from meta.
Would __get()
fire in that case? The property is declared, so __get()
would only get hit if someone takes a WP_Post object and then unsets a property.
#16
@
11 years ago
Oops, I thought that the properties were declared, but that's actually the inline blocks. Declaring the properties — even without default values — is enough to avoid it ever hitting __get()
.
#18
@
11 years ago
We don't need to go J-trip on it, but everything in the codebase should have Doc blocks on it for at least a fighting chance of generating usable docs
Famous last words: we could just remove the
wp_cache_add()
call.