WordPress.org

Make WordPress Core

Opened 21 months ago

Closed 7 months ago

Last modified 7 months ago

#21394 closed defect (bug) (fixed)

query.php: get_queried_object() result cannot be assumed to be non-NULL

Reported by: lkraav Owned by: wonderboymusic
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.3.2
Component: Warnings/Notices Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

I'm dealing with a WPMUdev plugin called MarketPress. Its /store/products/ page is a special CPT catalog thing that seems to have wp_title() issues, so I dug into it.

Turns out that:

is_front_page() -> is_page() -> $page_obj = $this->get_queried_object() -> $page_obj == NULL

But is_page() (and other is_...()) go harrassing $page_obj assuming there's always something useful there. This creates a flood of "non-object access" notices.

I would expect something like this to make sense in these is_...() functions:

if (!is_null($page_obj)) return false;

Since WP_Query::get_queried_object() first sets its result as null and returning a value is actually conditional, I don't see a way around these checks. Enlighten me please if I'm wrong.

I've looked at #19035, #18614, #17662. All of them are situations where there's at least something there in $page_obj. None of them seem to discuss or patch NULL values.

I discovered this on 3.3.2, but todays trunk trunk/wp-includes/query.php@21248#L3340 has the same code.

Attachments (4)

is-page-bail-on-null.diff (343 bytes) - added by wonderboymusic 18 months ago.
21394.diff (1.9 KB) - added by ryan 18 months ago.
21394.general-template.diff (1.1 KB) - added by markoheijnen 16 months ago.
Fixes for general-template.php
21394.2.diff (5.0 KB) - added by wonderboymusic 9 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 lkraav21 months ago

  • Cc leho@… added

Bleh. Ignore the "patch" above.

if (is_null($page_obj)) return false;

comment:2 SergeyBiryukov20 months ago

  • Description modified (diff)

comment:3 wonderboymusic18 months ago

  • Component changed from Query to Warnings/Notices
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.5
  • Owner set to wonderboymusic
  • Status changed from new to accepted

This is a simple check for empty

Last edited 18 months ago by wonderboymusic (previous) (diff)

comment:4 ryan18 months ago

Related: #20519

ryan18 months ago

comment:5 ryan18 months ago

Empty check for all is_*() funcs.

comment:6 follow-up: ryan18 months ago

That patch adds a bunch of defensive checks, but we need to better understand why a template tag is true but its corresponding queried object is empty.

comment:7 ryan18 months ago

  • Keywords early added
  • Milestone changed from 3.5 to Future Release

Per bug scrub, punting so we can take a closer look and not just patch symptoms.

markoheijnen16 months ago

Fixes for general-template.php

comment:8 markoheijnen16 months ago

  • Keywords early removed
  • Milestone changed from Future Release to 3.6

in wp-includes/general-template.php there was also a case where the assumption was made that get_queried_object wasn't null.

comment:9 in reply to: ↑ 6 lkraav15 months ago

Replying to ryan:

That patch adds a bunch of defensive checks, but we need to better understand why a template tag is true but its corresponding queried object is empty.

Good thing I did a search for is_page(), I was apparently about to file a duplicate of this same thing.

Here's the scenario that causes the trifecta:

NOTICE: wp-includes/query.php:3348 - Trying to get property of non-object
NOTICE: wp-includes/query.php:3350 - Trying to get property of non-object
NOTICE: wp-includes/query.php:3352 - Trying to get property of non-object

This e-commerce plugin uses virtual pagename's for rewriting. /store/products/ gets translated to pagename=product_list request:

$new_rules[$this->get_setting('slugs->store') . '/' . $this->get_setting('slugs->products') . '/?$'] = 'index.php?pagename=product_list'; 

Then it has a wp action, that checks for query_var == "product_list" and builds up a "real" query with a query_posts call.

Now Yoast's WP SEO plugin is actually the one causing these notices, because for some reason it does wp_reset_query() in its wp action $this->metadesc() handler. Why reset the query here, I do not know, I'm not familiar at all with the innards of this plugin. Then WP SEO hits the system with is_front_page() etc that passes through is_page() with $wp_the_query (i.e. the original "virtual" pagename) as its target.

So I am not sure how much understanding here is needed, but it seems real world is somehow coming up with these ways to do things that causes get_queried_object() to be null. It doesn't feel like what folks are doing is too much off The Right Way either. Or...?

Last edited 15 months ago by lkraav (previous) (diff)

comment:10 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

wonderboymusic9 months ago

comment:11 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

Marko's patch had a few more stragglers, I found even more upon closer examination

comment:13 wonderboymusic8 months ago

#17427 was marked as a duplicate.

comment:14 wonderboymusic7 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 25310:

Make sure the queried object is non-null before accessing its properties.

Props markoheijnen, ryan.
Fixes #21394.

comment:15 SergeyBiryukov7 months ago

#24788 was marked as a duplicate.

Note: See TracTickets for help on using tickets.