Errors in get_page_by_path()

[dd32]

Example of accessing an Image through it's not post url:

Expected url: /2008/06/10/post-format-test-gallery/canola2/
Accessed url: /canola2/

get_page_by_path() will result in these 2 warnings:

NOTICE: wp-includes\post.php:3167 - Trying to get property of non-object
NOTICE: wp-includes\post.php:3162 - Trying to get property of non-object

Similar warnings are now emmited by trunk when accessing child pages as well

[solarissmoke]

Patch removes the restriction on post type for the parent query. My thinking here was that the $post_type argument should only apply to the page being requested, and not to its parents (which could hypothetically belong to any post type).

[dd32]

ah approach that both me and duck_ have used here, is to break from the while loop when $curpage == null. This still allows the request to be parsed correctly (even if it is at the wrong url, but that's not the issue at hand)

[dd32]

Thought I better upload the patch since I've run into it again.

.2 includes a bad attempt at increasing performance, .3 does the job though.

[Viper007Bond]

Needs a patch refresh due to [18541]. Would be nice to get this in to 3.3.

[SergeyBiryukov]

Looks like after [18541], notices are displaying for any child page, not just wrong attachment URLs. 17670.4.diff​ should fix this.

[chrisbliss18]

Benchmarks the original code verses two different fixes

[chrisbliss18]

Just added fix-get_page_by_path-warnings-rewrite.diff. I was chasing this before I knew a ticket existed for it. This fixes the issue that is affecting all child pages.

The rewrite is because I noticed that some optimizations could be made, and since this runs on all pages when permalinks are used, efficiency should be kept in mind.

From my testing, my supplied patch showed a ~16% decrease in execution time as compared to the existing code when using a test set of one levels of depth, two sets of two levels of depth, and one set of three levels of depth. I came up with a simple fix that is very similar to 17670.4.diff, and it improved performance by ~8%, so the rewrite showed more benefit.

The get_page_by_path-benchmark.php attachment can be run to try these benchmarks for yourself.

[dd32]

It looks like that approach passes with breakneck speed, however the testing doesn't cover non-existent url's (such as level-5/level-2/level-3/ and /level-2/level-2/level-3/) which erroneously return level-3 as being the page we're on, even though the parents don't match.

I'm attaching the same benchmark script with a few alterations I've made (Extra few fail test sets and my rewrite of it - I'm leaning towards the rewrite as it ultimately ends up looking cleaner than a simple patch). It's benchmarking about the same for me.

We'll need some proper ​Unit Test coverage for this change however, to catch any other edge cases I've missed

[duck_]

There is an issue with 17670.5.diff. If the URL being visited only has one part, e.g. /page-slug, and there are 2+ pages that match that slug but are in different trees then the wrong page could be displayed as the the loop within 1 == $revparts_count doesn't check that $page->post_parent == 0.

I've attached 17670.6.diff which is my take on fixing the issues. I haven't attempted a load of optimisation as I think that should be dealt with separately -- fix the notices and other issues first.

[ocean90]

#18892, #18919

[gautamgupta]

Tested the patch, works for me :)

[dd32]

I think duck_'s patch is safe, but I thought mine was too :) I'd have liked some unit tests here, but I've tried and failed a few times at constructing the environment for it.

Please don't leave me to point on this ticket though, Don't assume I'm handling it..

[CoenJacobs]

Patch seems to work now. Haven't completely tested it. It is an annoying little bastard, that's for sure.

[duck_]

Replying to dd32:

Please don't leave me to point on this ticket though, Don't assume I'm handling it..

I've had this ticket as a permanent tab in my browser for a while now, but I just haven't found the time to properly write some comprehensive tests.

[dd32]

I've had this ticket as a permanent tab in my browser for a while now

So have I.. I've just been so frustrated by these loops that I applied your patch and moved on.

[duck_]

I was looking at the latest patch again and working through its logic when I noticed a major problem both with it and the current code in trunk.

Create two similar hierarchies of pages, but with one missing the highest level page, e.g.

level-0 > level-1 > level-2

level-1 > level-2

where > indicates "parent of". Make sure you give them content that you can distinguish them by, e.g. their slug and the slugs of their ancestors, and create them in the order displayed so that the deeper "level-2" has a lower ID.

Now visit /level-1/level-2 and you will receive the wrong level-2 page.

The problem currently in trunk is that when we try to lookup the parent of level-1 we get NULL as level-0 isn't in the array of pages, so the while loop (line 3171) stops and the if statement (line 3178) passes since the count is correct.

17670.6.diff is broken in a similar way, but without the PHP notices.

Attaching 17670.7.diff to check that we've reached the root of a tree of pages before accepting.

I think that the issue raised in this comment is a blocker for release. This ticket should either be repurposed to reflect that or closed as dupe of a new ticket, any preferences?

[nacin]

I think that the issue raised in this comment is a blocker for release. This ticket should either be repurposed to reflect that or closed as dupe of a new ticket, any preferences?

[Otto42]

That logic is downright mind-bending, but 17670.7.diff looks good to me.

[ryan]

In [19075]:

Fix notices and logic errors in get_page_by_path(). Props duck_. see #17670

[ryan]

Trying on 17670.7.diff.