Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#17670 closed defect (bug)

PHP Notices from get_page_by_path() — at Version 8

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

Change History (16)

4 years ago

#1 @solarissmoke
4 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).

#2 @dd32
4 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)

4 years ago

#3 @dd32
4 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.

4 years ago

#4 @Viper007Bond
4 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.

#5 @SergeyBiryukov
4 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.

4 years ago

Benchmarks the original code verses two different fixes

#6 @chrisbliss18
4 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.

#7 @dd32
4 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

4 years ago

#8 @dd32
4 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()
Note: See TracTickets for help on using tickets.