Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#17670 closed defect (bug) (fixed)

Errors in get_page_by_path()

Reported by: dd32 Owned by:
Milestone: 3.3 Priority: highest omg bbq
Severity: blocker Version: 3.1
Component: Warnings/Notices Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description (last modified by 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

Attachments (10)

17670.diff (681 bytes) - added by solarissmoke 4 years ago.
17670.2.diff (778 bytes) - added by dd32 4 years ago.
17670.3.diff (543 bytes) - added by dd32 4 years ago.
17670.4.diff (1.0 KB) - added by SergeyBiryukov 4 years ago.
get_page_by_path-benchmark.php (5.8 KB) - added by chrisbliss18 4 years ago.
Benchmarks the original code verses two different fixes
fix-get_page_by_path-warnings-rewrite.diff (1.2 KB) - added by chrisbliss18 4 years ago.
get_page_by_path-benchmark.2.php (8.7 KB) - added by dd32 4 years ago.
17670.5.diff (2.1 KB) - added by dd32 4 years ago.
17670.6.diff (1.1 KB) - added by duck_ 4 years ago.
17670.7.diff (1.2 KB) - added by duck_ 3 years ago.

Download all attachments as: .zip

Change History (36)

@solarissmoke4 years ago

comment:1 @solarissmoke4 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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

comment:2 @dd324 years ago

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)

@dd324 years ago

comment:3 @dd324 years ago

  • Keywords dev-feedback removed

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.

@dd324 years ago

comment:4 @Viper007Bond4 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Future Release to 3.3

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

@SergeyBiryukov4 years ago

comment:5 @SergeyBiryukov4 years ago

  • Keywords has-patch added; needs-refresh removed

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

@chrisbliss184 years ago

Benchmarks the original code verses two different fixes

comment:6 @chrisbliss184 years ago

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.

comment:7 @dd324 years ago

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

@dd324 years ago

comment:8 @dd324 years ago

  • Description modified (diff)
  • Summary changed from PHP Notices when Attachment is accessed via wrong url via pagerewrite to PHP Notices from get_page_by_path()

comment:9 @ocean904 years ago

  • Cc ocean90 added

@duck_4 years ago

comment:10 @duck_4 years ago

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.

comment:11 @demetris4 years ago

  • Cc dkikizas@… added

comment:12 @mtekk4 years ago

  • Cc mtekkmonkey@… added

comment:14 @gautamgupta4 years ago

Tested the patch, works for me :)

comment:15 @markoheijnen4 years ago

  • Cc marko@… added

comment:16 follow-up: @dd324 years ago

  • Keywords needs-testing needs-unit-tests added

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

comment:17 @CoenJacobs4 years ago

  • Cc coenjacobs@… added
Version 0, edited 4 years ago by CoenJacobs (next)

comment:18 in reply to: ↑ 16 @duck_4 years ago

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.

comment:19 @dd324 years ago

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.

comment:20 @Otto424 years ago

  • Cc Otto42 added

comment:21 @duck_3 years ago

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?

@duck_3 years ago

comment:22 @nacin3 years ago

  • Priority changed from normal to highest omg bbq
  • Severity changed from normal to blocker
  • Summary changed from PHP Notices from get_page_by_path() to Errors in get_page_by_path()

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?

comment:23 @Otto423 years ago

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

comment:24 @ryan3 years ago

In [19075]:

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

comment:25 @ryan3 years ago

Trying on 17670.7.diff.

comment:26 @ryan3 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.