Lazy load ancestors

[nacin]

It should be pretty easy to lazy load ancestors using a magic get function on 'ancestors', rather than querying for them individually via _get_post_ancestors().

johnjamesjacoby presented a use case where get_post_field() was triggering ancestors queries, with no way to avoid that if you know you need that information. Not sure how to solve that specific problem without thinking bigger.

[westi]

Can we have more detail on the use case:

• Maybe Some code
• What we are trying to avoid

At the moment this ticket has a very specific solution proposed but little detail on the problem - which makes it hard to know if this is the best solution or not.

[nacin]

The use case is get_post_field( 'post_type', $post_id ) or some other query that doesn't require populating the ancestors. It could even be a straight get_post() call -- really, the use case doesn't have anything to do with the suggestion here other than it gave me the idea.

Being able to lazy load ancestors can save a number of queries. They're fast, but with a complex hierarchy and a number of get_post() calls, they can add up. Then only the queries are triggered when needed.

Thinking about it some more, this will require a real WP_Post object -- #12267. And is probably only one of a few improvements that can come out of that.

[johnjamesjacoby]

+1 - This saves countless ancestral queries in the bbPress plugin where a simple look into the posts table with get_post_field() is needed without the need for walking up a tree of post_parent's.

[johnjamesjacoby]

Use get_posts in _get_posts_ancestors()

[johnjamesjacoby]

In the mean time, using get_posts in _get_post_ancestors() rather than hitting the DB directly with an uncached query cuts down on cache misses and DB hits in the double digits in the bbPress plugin, particularly in situations where there are Forum, Topic, or Reply widgets in sidebars, and/or subforums are involved.

[johnjamesjacoby]

Post type agnostic, and make diff do what it's supposed to

[filosofo]

How about instead we deprecate the post object's ancestors property with a magic __get method, and then use a working get_post_ancestors() function to get the ancestors in core WP?

That way, we maintain backwards-compatibility for those who are referencing the ancestors post property in plugins, but we are explicit about actually querying the post ancestors, in core (and as recommended practice).

Resource-intensive queries like that should be as explicit as possible, for code readability and debugging purposes. Hiding the query in a magic __get method is slightly slower, and it makes it harder to identify the conditions in which such queries will occur.

In other words, as a band-aid to handle deprecated properties, it's fine, but let's not make it standard practice, particularly for retrieving external resources (database records) as opposed to e.g. internal object properties, which I think was the intent of __get.

[johnjamesjacoby]

Tested my above patch further, and $post->ancestors gets polluted in between other calls to get_posts(), so that isn't a viable alternative quite yet.

[johnjamesjacoby]

Just use get_post instead of get_posts

[johnjamesjacoby]

Using get_post here also reduces cache misses with no adverse effects for things like single blog post views, where the next and previous posts are automatically added to wp_head, and then loaded again in the content area. Without this method, ancestor queries aren't cached. Same goes for pages in nav menus, etc...

[johnjamesjacoby]

As a general update, 16574.3.diff is still good. I've been running several .org installs with it since posted with only positive results. It doesn't add any fancy magic functions, but it does at least give ancestor queries the opportunity to hit the WP cache instead of always hitting the DB.

Any install with a page widget and a page menu would see a noticeable decrease in DB hits with this patch alone. The bbPress plugin sees a dramatic decrease (of sometimes 10+ queries) because it relies heavily on the hierarchy to manage the post relationships between forums/topics/replies.

If there are alternatives (magic functions, etc...) I'd love to collaborate on them to test the benefits.

[johnjamesjacoby]

Keeping in future release, removing 3.2-early keyword and adding 3.3-early keyword. Also added needs-testing keyword, because it needs testing.

Specifically looking to see if this causes an infinite loop situation, where an ancestor might call get_post() which calls _get_post_ancestors() which calls get_post() etc...

[scribu]

It seems to me that using a separate cache group just for post ancestors would fix a lot of problems related to cache pollution.

[scribu]

We could remove all the fragile logic around _get_post_ancestors() from get_post() and move the main work to get_post_ancestors().

[scribu]

Only negative implication would be that you can't use $post->ancestors directly. You would have to call get_post_ancestors() instead, which I don't think is too much to ask.

[scribu]

16574.4.diff​ introduces the 'post_ancestors' cache group and merges _get_post_ancestors() into get_post_ancestors().

[scribu]

Introduce 'post_ancestors' cache group

[nacin]

Doesn't seem like it would need a new group all together. The key in the posts cache group could simply be ancestors-$id.

That said, breaking compatibility with $post->ancestors is definitely not something we can get away with right now. Perhaps after the success of WP_Theme, someone (me?) might take a crack at a proper WP_Post in a future release, which could alleviate back compat issues.

I did very quick grep in the plugins directory for 'post->ancestors' and stopped it after seeing at least 50 plugins (100s of instances) using it.

[scribu]

Doesn't seem like it would need a new group all together. The key in the posts cache group could simply be ancestors-$id.

Why would that be better?

I did very quick grep in the plugins directory for 'post->ancestors' and stopped it after seeing at least 50 plugins (100s of instances) using it.

Sigh...

[scribu]

Introduce _WP_Post_Wrapper class

[scribu]

16574.5.diff​ introduces a _WP_Post_Wrapper class that only deals with $post->ancestors.

[johnjamesjacoby]

Add isset() check to magic get()

[johnjamesjacoby]

Gah! __

Replying to scribu:

16574.5.diff​ introduces a _WP_Post_Wrapper class that only deals with $post->ancestors.

Creating a whole new class for this one thing is overkill. It does fix the problem, but introduces two others:

---

First issue happens in the default BuddyPress theme:

Notice: Indirect modification of overloaded property _WP_Post_Wrapper::$object_id has no effect in /wp/wp-includes/nav-menu.php on line 591

Something with

$original_object = get_post( $menu_item->object_id );

is causing the object_id to be overloaded. Not sure where or why yet, if the problem is on BuddyPress's end or not. Should note there are no errors as is before 16574.5.diff​ is applied.

---

Second issue happens because of missing isset() in your magic __get()

Trying to get property of non-object in /wp/wp-includes/post.php on line 442

16574.6.diff​ fixes the second issue, above.

[scribu]

Creating a whole new class for this one thing is overkill.

It paves the way for WP_Post. I didn't name it that in the hopes that people won't start explicitly checking for it, but I'm not sure if that's a wise decision.

$original_object = get_post( $menu_item->object_id );

That's because get_post() takes it's first parameter by reference and $menu_item is now an instance of _WP_Post_Wrapper.

There was a similar notice in get_page_uri(), which is fixed in the patch.

[scribu]

As for the isset() in __get(), I'm not sure if it's a good idea to silently fail if the key doesn't exist.

[scribu]

Trying to get property of non-object in /wp/wp-includes/post.php on line 442

This seems to indicate that $this->post is not an object, which is really strange. isset() would just mask the problem, instead of fixing the root cause.

[scribu]

avoid Inception

[scribu]

16574.7.diff​ avoids wrapping a _WP_Post_Wrapper in another one.

[johnjamesjacoby]

Replying to scribu:

As for the isset() in __get(), I'm not sure if it's a good idea to silently fail if the key doesn't exist.

Plugins like BuddyPress and bbPress don't always load a real $post and rely on silent failure. The $post global is sometimes set with dummy (or semi relevant) information to satisfy the bare minimum of WordPress's API that expects it to exist. Silent failure is the way the WP_User magic methods work; given WordPress's pluggable nature, it's fitting that it prevent notices where it can.

[johnjamesjacoby]

Replying to scribu:

16574.7.diff​ avoids wrapping a _WP_Post_Wrapper in another one.

With 16574.7.diff​ BuddyPress's bp-default menu still shows the first notice I posted above.

[scribu]

Plugins like BuddyPress and bbPress don't always load a real $post and rely on silent failure.

I don't understand. With a plain stdClass, when the key doesn't exist, you would get a notice as well.

[scribu]

16574.8.diff​ makes __get() return by reference, which should fix the "Indirect modification of overloaded property ... has no effect" notice.

[scribu]

&__get()

[braydonf]

Replying to scribu:

16574.8.diff​ makes __get() return by reference, which should fix the "Indirect modification of overloaded property ... has no effect" notice.

It would be great to have an action/filter to be able to use different classes for each post depending on its post_type that would need to be extended from this WP_Post_Wrapper class.

[scribu]

@braydonf: This is already outside the scope of this ticket. _WP_Post_Wrapper was just a way to provide back-compat for $post->ancestors.

We should discuss the plan for the WP_Post class in a new ticket or on wp-hackers.

[braydonf]

Replying to scribu:

We should discuss the plan for the WP_Post class in a new ticket or on wp-hackers.

Okay, just made a new ticket for this #20259

[scribu]

Superseded by #21309

[johnjamesjacoby]

Related cache changesets : r21943, r21952.