WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 6 years ago

#10381 closed defect (bug) (fixed)

post->ancestors isn't always set

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 2.8
Component: Cache API Keywords: dev-feedback needs-unit-tests early
Focuses: Cc:

Description

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.

Attachments (5)

fix-post-ancestors.diff (435 bytes) - added by Denis-de-Bernardy 9 years ago.
10381.diff (381 bytes) - added by Denis-de-Bernardy 9 years ago.
10381.2.diff (550 bytes) - added by Denis-de-Bernardy 9 years ago.
10381.3.diff (447 bytes) - added by Denis-de-Bernardy 9 years ago.
an even fix from a memory usage standpoint -- avoids all potential loops and heavy memory usage
post.php.diff (1.0 KB) - added by shidouhikari 8 years ago.
Another attempt to deal with post ancestors and cached posts

Download all attachments as: .zip

Change History (24)

#2 @westi
9 years ago

  • Cc westi added
  • Keywords reporter-feedback needs-unit-tests added; has-patch removed

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.

#3 @Denis-de-Bernardy
9 years ago

  • Keywords reporter-feedback removed

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.

#5 @ryan
9 years ago

  • Milestone changed from 2.9 to 3.0

#6 @Denis-de-Bernardy
9 years ago

  • Keywords dev-feedback added

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.

#7 @Denis-de-Bernardy
9 years ago

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.

#8 @Denis-de-Bernardy
9 years ago

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
9 years ago

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

#10 @ryan
8 years ago

  • Keywords early added
  • Milestone changed from 3.0 to 3.1

#11 @maorb
8 years ago

  • Cc maorb added

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

#12 @maorb
8 years ago

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

#13 @maorb
8 years ago

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.

#14 @Denis-de-Bernardy
8 years ago

@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.

#15 @shidouhikari
8 years ago

  • Cc shidouhikari added

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
8 years ago

Another attempt to deal with post ancestors and cached posts

#16 @shidouhikari
8 years ago

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.

#17 @nacin
8 years ago

  • Milestone changed from Awaiting Triage to Future Release

#18 @ryan
6 years ago

  • Milestone changed from Future Release to 3.5

#21309 should address this.

#19 @ryan
6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

In [21559]:

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

Note: See TracTickets for help on using tickets.