post->ancestors isn't always set

[Denis-de-Bernardy]

when update_post_cache() gets called to cache the results of a query so as to avoid needless db calls to get permalinks, and then get_post() is used, posts can end up without the ancestor variable being set.

[Denis-de-Bernardy]

see also #10361

[westi]

Patch still applies.

If I understand correctly the issue here is that when we run the main Query we cache posts without fetching ancestor information so when we call get_post to get the info it it is in the cache we then return the cached data which is ancestor less.

So the question is should we fix that here by checking every cached post for ancestor info and thereby running the ancestor update query for the first get_post call for these posts or should we always pre-cache the ancestor info when querying.

It looks like all other calls to _get_post_ancestors are in get_post so this would be the place to fix it.

However, the supplied patch does I think achieve anything - it checks $_post->ancestors which can only be meaning full if we enter the if it is the else if for.

Think this needs some more thought - probably a good idea to have some tests around this functionality.

[Denis-de-Bernardy]

My reasoning when I initially opened the ticket was... I wanted to use the ancestors field to avoid DB calls. I then noticed it wasn't always set. It's not much of a big deal in practice, except that the object cache ends up with crummy data.

What the patch does is, it verifies that the ancestors are set and fixes the object cache in the event they aren't.

I ended up deciding to query the DB anyway and use a special cache bucket for ancestors, since the WP API was too unreliable on this front. Just close as wontfix if you think we can live with that.

[Denis-de-Bernardy]

related: #11292

[Denis-de-Bernardy]

I've uploaded an additional patch, that fixes update_post_cache() -- which in this case was the source of my problem.

Related: #6746 and #6702.

[Denis-de-Bernardy]

further follow-up on this one. testing reveals that the correct fix is the first one suggested. the second patch causes memory-related problems by generating get_post() loops.

attaching a new patch that doesn't needlessly hammer the DB.

[Denis-de-Bernardy]

Re the the two previous patched, I've been testing them indirectly since yesterday by playing around with a heavily modified version of Ryan's memcached-based object-cache.php file.

Initially, I fixed things in wp_cache_add(), but as noted above it was prone to memory problems. Adding the fix to wp_cache_get() instead is working fine so far.

[Denis-de-Bernardy]

an even fix from a memory usage standpoint -- avoids all potential loops and heavy memory usage

[Denis-de-Bernardy]

see also: #12812

[maorb]

I have this bug too, $post->ancestors is NULL in any case, so the css class current_page_ancestor not set as expected.

Does the fix suggested here works and causes no problems?

Thanks

[maorb]

I added the fix 10381.3.diff to post.php
Now I see that an array of ancestors does return properly, which is a progress, but still the current_page_ancestor class is not set to the page item, when using the wp_list_pages() function.
Any ideas about that?

Thanks

[maorb]

Ok, there is a progress on this (I don't know how to attach a .diff file link, so I attach the code in here):

On the wp-includes/classes.php inside function start_el (which starts on line #1168, in WP2.9.2) Instead:

		if ( !empty($current_page) ) {				
			$_current_page = get_page( $current_page );

Put this:

if ( !empty($current_page) ) {			
			global $_wp_using_ext_object_cache;
                         wp_cache_delete($current_page, 'posts');
			$_current_page = get_page( $current_page );

And now the css class of current_page_ancestor will be also added.
I don't know if this is the best or desired fix, so hope to hear your opinion.

[Denis-de-Bernardy]

@maorb: the tricky thing is, forcing WP to properly fill the object cache, when using memcache, makes WP hang a bit; and it makes WP consume an obscene amount of resources when not.

when I experimented with it, I found that flushing dirty cache items on a per need basis did a good enough job, considering that it'll end up with clean data at some point. My own goals were arguably different than yours though.

[shidouhikari]

But is it a good idea to keep deleting cache and resetting it?

I understand it's rarely needed, so every get_post() should delete post from cache and reset it?

Also, Denis is only deleting cache when *any* external cache is being used. What if core cache is used, a previous post query didn't set ancestors, and another query in the same pageload is done and cached object is used? Or if in some other place a code tests if object is in cache and if so it doesn't even call get_post(), also not calling _get_post_ancestors()? Complicated :/

It seems the best place to fix *ATM* this is indeed inside _get_post_ancestors(), because just after its 2 calls wp_cache_add() is called. So, wp_cache_add() will see there's no cache and set it. But what happens in, say, WP 3.5, _get_post_ancestors() is called somewhere else and everytime it happens cache is deleted? At that time nobody will know that wp_cache_delete() is being called inside it expecting for wp_cache_add() be used after it returns.

I believe a long run for this is to update cache everytime ancestors is needed, therefore testing cache when _get_post_ancestors() is used and verifying if it has ancestors set. If post is cached and it has ancestors, nothing else must be done.

Another note: regarding 10381.3.diff, since you are deleting post in cache everytime _get_post_ancestors() is called, regardless of if cache has ancestors or not. If cache has ancetors, there's no need to delete it.

I know I'm not skilled enought to say any of these patches are wrong, but they are expecting that 2 codes (_get_post_ancestors and wp_cache_add) in different places are run together. This fixes the issue for now but there's really nothing forcing them to run together. If in the future they are used separated, it may create an even bigger problem (ex: _get_post_ancestors() is used in another file and starts deleting posts from cache without need).

[shidouhikari]

Another attempt to deal with post ancestors and cached posts

[shidouhikari]

Let me suggest another solution.

If post from parameter is object, it has its ancestors set. Cache is queried and if ancestors isn't set on it cache is deleted, to be added again just following.

If post from parameter isn't object or is object and has filter set, get its ID. Cache is queried and if exists but again doesn't have ancestors set, update the object and set a new cache, without querying a whole object from database.

If cache exists and has ancestors, nothing must be done.

[ryan]

#21309 should address this.

[ryan]

In [21559]:

Introduce WP_Post class. Clean up ancestors handling. Props scribu, toppa. fixes #10381 see #21309