Add caching to get_page_by_path

[spacedmonkey]

The function is get_page_by_path is called lot in WP_Query. Currently is doesn't have any caching on it. This is performance issue if you are using persistent cache.

[spacedmonkey]

First pass at this patch. Pretty simple patch to get caching. The caching method is simple to the one in the get_pages function. This cache key use last_changed, a value set in the clean_post_cache function.

[spacedmonkey]

@ocean90 What unit tests should we be writing for this patch? I run the current tests and they are passing.

[boonebgorges]

Replying to spacedmonkey:

@ocean90 What unit tests should we be writing for this patch? I run the current tests and they are passing.

At a minimum, I'd write one test that demonstrates that two subsequent, identical queries result in only one database query; and one test that demonstrates that running a query, updating the page, and then running the same query results in two database queries (and that the results of the second query match the post-update page).

[spacedmonkey]

New patch with unit tests

[spacedmonkey]

36711.4 patch, is another way of doing caching for this function. It is better as it isn't invalidated every time the post cache is cleaned. The code isn't as clean through.

[boonebgorges]

In 37478:

Tests: Improve tests for get_page_by_path().

• Move existing test into its own file.
• Add tests that cover most pieces of functionality.

See #36711.

[boonebgorges]

@spacedmonkey Thanks for your work here.

The approach in 36711.4.patch​ will not work, because a page's path depends on pages other than itself. For example, a page /foo/bar with a parent of foo will get a new patch if foo changes to baz. It would probably be possible to build a more targeted invalidation routine. It'd mean that when a page's path is updated, the path cache of each of its descendants must also be updated. This is a lot of code and a lot of overhead for a small improvement, so I'd suggest we skip it for now and go with the more general last_changed technique.

36711.5.patch​ cleans up the improvements in 36711.3.patch​ and clarifies the tests. I think it's a pretty safe change, but I'd appreciate another set of eyes.

[spacedmonkey]

I have been running a patch similar patch on production ( millions of requests a day ) and it has massively cut down the number of queries to the posts table. Very happy to see this in core :D

Couple of notes on 36711.5.patch

The follow lines are problem for me.

    if ( false !== $cached ) { 
           return get_post( $cached ); 
    } 

I believe they should look something like this.

    if ( false !== $cached ) { 
           if ( $cached ) {
                return get_post( $cached, $output ); 
           }
           return;
    } 

Fixes 2 issues.

1. The format of output is now respected.
2. If $cached is 0 ( nothing found ), it will not give that to get_post. If you give get_post an empty value (null/0/false) it will just return the current post, which is incorrect. If the cached value is 0, the function should just return.

One thing I have never understand about this function, why doesn't it return anything if nothing is found? It could at least return null or false.

Once we have worked through all the issues with this patch, similar caching can be applied to the get_page_by_title function as well :D

[boonebgorges]

I believe they should look something like this.

Ah yes, thanks. I've made the change and added a test that describes it.

One thing I have never understand about this function, why doesn't it return anything if nothing is found? It could at least return null or false.

The answer, if there is one, lies in the log. Go to the log, my son. Go to the log. (And open a new ticket if you want to change it, and have supporting info from the log.)

Let's put this in and see what breaks.

[boonebgorges]

In 37479:

Cache queries in get_page_by_path().

Props spacedmonkey.
Fixes #36711.

[spacedmonkey]

@boonebgorges in 37479 the cache return value doesn't take the output param.

[boonebgorges]

@spacedmonkey I'm afraid I don't understand. Can you please explain in more detail, ideally with a test that demonstrates the problem?

[spacedmonkey]

If doesn't require a test. get_page_by_path takes a $output param. The original function passes $output to get_post like so.

if ( $foundid ) {
    return get_post( $foundid, $output );
}

However currently, if the value returned from cache it doesn't pass the output param.

} else {
   return get_post( $cached );
}

This line should look like this

} else {
   return get_post( $cached, $output );
}

Currently if you run the function like this.

$value = get_page_by_path( 'foo', ARRAY_A );

It would return an object.

[boonebgorges]

If doesn't require a test.

If it's a bug, it does.

Thanks for the explanation. I will write the tests.

[boonebgorges]

In 37481:

In get_page_by_path(), values fetched from cache should obey $output param.

Introduced in [37479].

Props spacedmonkey.
Fixes #36711.

[SergeyBiryukov]

In 46819:

Tests: Remove duplicate test_get_page_by_path_priority() test.

The test was moved into its own file in [37478], but never removed from its previous location.

See #36711.