WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 16 months ago

#29660 new defect (bug)

Functions in wp_includes/query.php assume non-null return value from get_queried_object

Reported by: yellyc Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.0
Component: Query Keywords: 2nd-opinion
Focuses: Cc:

Description

There are several functions in wp_includes/query.php that use get_quried_object and assume non-null return values, namely:

  • is_attachment
  • is_author
  • is_category
  • is_tag
  • is_tax
  • is_page
  • is_single
  • is_singular

This oversight can result in frequent "Trying to get property of non-object" errors.

Attachments (2)

query-patch.diff (1.7 KB) - added by yellyc 4 years ago.
A patch to add null return checks for get_queried_object
29660.diff (439 bytes) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (26)

@yellyc
4 years ago

A patch to add null return checks for get_queried_object

#1 @SergeyBiryukov
4 years ago

  • Keywords reporter-feedback added

This was previously suggested in 21394.diff (see comment:6:ticket:21394), but the consensus in IRC was that these notices can sometimes reveal a developer error, so we probably should not just patch symptoms.

Is there a valid case for any of these template tags to be true while the corresponding queried object is empty? Could you provide a piece of code to reproduce the issue on a clean install?

#3 @yellyc
4 years ago

First of all, I am very surprised my search for open tickets did not yield the ticket in question, I apologise for the duplicate. As for a valid case in for use of these tags in cases in which the queried object is empty, I for one make a call to is_category in a function called at wp_enqueue_scripts, which is called on many occasions, some of which don't have a queried object.

#4 follow-up: @boonebgorges
4 years ago

As for a valid case in for use of these tags in cases in which the queried object is empty, I for one make a call to is_category in a function called at wp_enqueue_scripts, which is called on many occasions, some of which don't have a queried object.

Do you have any more details about these occasions? To draw out the logic of SergeyBiryukov's comment a bit more: https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php#L4326 The logic in WP_Query::is_page() would only get to the get_queried_object() check if $this->is_page is set (line 4327). It's unclear how this would happen without get_queried_object() having been set - it probably means that some plugin has been messing with the $wp_query global in an unsavory way. If you can share some more info about the situations where you're seeing the error - such as the other plugins you're running - we might be able to see more clearly whether we want to suppress these warnings in core.

#5 in reply to: ↑ 4 @yellyc
4 years ago

Replying to boonebgorges:

As for a valid case in for use of these tags in cases in which the queried object is empty, I for one make a call to is_category in a function called at wp_enqueue_scripts, which is called on many occasions, some of which don't have a queried object.

Do you have any more details about these occasions? To draw out the logic of SergeyBiryukov's comment a bit more: https://core.trac.wordpress.org/browser/tags/4.0/src/wp-includes/query.php#L4326 The logic in WP_Query::is_page() would only get to the get_queried_object() check if $this->is_page is set (line 4327). It's unclear how this would happen without get_queried_object() having been set - it probably means that some plugin has been messing with the $wp_query global in an unsavory way. If you can share some more info about the situations where you're seeing the error - such as the other plugins you're running - we might be able to see more clearly whether we want to suppress these warnings in core.

Like I mentioned, I use is_category() in a function that is called when wp_enqueue_scripts is triggered since I have a script that I only want enqueued for a certain category. wp_enqueue_scripts is triggered on any request, some of which don't have a queried object at all (such as date archives, home page and search).

#6 @boonebgorges
4 years ago

Yes, I saw that you're calling it at wp_enqueue_scripts. But on a vanilla WP installation, in each of the cases that you mentioned (home page, search, date archive), the 'is_page' flag at line 4327 that I mentioned above is false. So the get_queried_object() check at 4333 is never reached in these cases. The question is: what on your installation is making 'is_page' true at 4327? Please have a look at the plugins and other customizations that are running when this happens.

#7 @darkskipper
3 years ago

Hello, I've seen this on my site too.

Steps to reproduce:

  1. Install WordPress 4.1.1.
  2. All plug-ins are deactivated.
  3. Default theme "TwentyFifteen" in use.
  4. Set permalinks to custom structure "/%post_id%/".
  5. In the reading settings, set front page to display the "sample page" static page. Leave "posts page" unset.
  6. Log out.
  7. Make sure PHP error logging is enabled.
  8. In the browser, enter a URL for the comments feed of a non-existent post, e.g. "http://my.site.net/non/existent/place/feed/".
  9. Examine the PHP error log.
  • Expected: no extra log messages.
  • Actual: log messages, such as "Notice: Trying to get property of non-object: {path}/wp-includes/query.php(4349)". I got six similar messages in my case.
  • Impact: Such URLs can be requested by web crawlers and other "bots". This will result in the accumulation of log messages. These messages are the result of core WP code behaviour and not theme nor plug-in code.

I do not believe that these messages reflect a developer error on my part, therefore I hope this adds fuel to the argument for adding a patch to check the return value of WP_Query::get_queried_object() at line 4345 in query.php.

Regards, Kevin Machin.

#8 follow-up: @boonebgorges
3 years ago

darkskipper - Thanks very much for the detailed steps to reproduce. I've managed to verify. In your specific case, the problem arises from a combination of a non-existing feed request + the use of the "front page" setting in Dashboard > Settings > Reading. This leads to a number of places in redirect_canonical() where is_front_page() is invoked, which in turn calls is_page( get_option( 'page_on_front' ) ). It's this last bit that's causing the PHP notice. The correct fix here is not a band-aid over the notice, but to ensure that we never get this far in the is_front_page() chain when we're looking at a feed. I recommend something like 29660.diff - is_front_page() ought never to return true for feeds.

So, two questions. (1) Does anyone see a problem with this? Are there cases where is_front_page() might be true when is_feed? (2) yellyc - is your case similar to this one?

@boonebgorges
3 years ago

#9 @darkskipper
3 years ago

In the mean time, here's a little "band-aid" I'm using...

remove_action ('template_redirect', 'redirect_canonical',   10);
add_action    ('template_redirect', 'actRedirectCanonical', 10);

function actRedirectCanonical ()
{
    @redirect_canonical (); // Quietly
}

To make the above code theme-agnostic, it can be placed in a must-use plug-in. Otherwise it can just go in a theme's functions.php file.

#10 @SergeyBiryukov
3 years ago

#33729 was marked as a duplicate.

#11 @Howdy_McGee
3 years ago

It's really unfortunate that this doesn't get as much attention since it's a persistent problem. The biggest issue I think is that the errors being generated don't reflect the exact issue, where it's originating from, or the cause. Right now it's all the same generic message.

I can replicate errors like this in pre_get_posts just as I can in any other form but it doesn't tell me why it's happening. The duplicate ticket above #33729 errors out whenever passing strings to is_category() and the category isn't found. That's not very intuitive and to get around it I have to use some pretty funky logic. If it is decided that the core errors aren't worth fixing then I think we should also dump additional information to the developers as to why the errors occur. Though, personally I think that since the functions are being used correctly then the solution should be patching the core functions to reflect these cases.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#12 @SergeyBiryukov
3 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.5

#14 @nacin
2 years ago

Yeah, interestingly, that patch would butt up against #20899. Glad you remembered that one, @swissspidy.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


2 years ago

#16 @Howdy_McGee
2 years ago

I'm possibly repeating what is already known but since $wp_query->get_queried_object() sets the $query_object to null there's a possibly in current and future edge cases that it will return a non-object.

For example - I hook into pre_get_posts and conditionally pass a non-existing term slug to $wp_query->is_category( 'term-slug' ). It goes and looks for the term using get_term() and get_term_by() which both return false and then the $wp_query->is_category() function attempts to get properties from a null object.

This is not the case with $wp_query->is_tax( 'taxonomy', 'term-slug' ) because of this little conditional in the function:

if ( ! ( isset( $queried_object->taxonomy ) && count( $tax_array ) && in_array( $queried_object->taxonomy, $tax_array ) ) )

Now it seems like we either need to _always_ return an object from $wp_query->get_query_object() whose properties vary depending or start adding checks into the is_* functions for null values. Which one is more favorable because I could start patching the latter.

This ticket was mentioned in Slack in #core by jrf. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by howdy_mcgee. View the logs.


2 years ago

#19 @mikeschroder
2 years ago

  • Keywords 4.6-early has-patch needs-testing added
  • Milestone changed from 4.5 to Future Release

It seems like there is significant testing required to make sure we don't cause backcompat issues, and we're already mid-beta.

Since #20899 was moved to 4.6-early, doing so here as well.

This ticket was mentioned in Slack in #core by howdy_mcgee. View the logs.


2 years ago

#21 @boonebgorges
2 years ago

  • Keywords 2nd-opinion added; 4.6-early has-patch needs-testing removed

I remain concerned that adding empty() checks will paper over legitimate bugs.

The code path described by @Howdy_McGee in #33729 is a legitimate way to trigger the PHP notice, but IMHO this is due to an underlying problem with the semantics of our is_* functions.

What does it mean to say $this->is_category()? Does it mean that a category was *requested*? Or does it mean that you're viewing an actual category? The way WP_Query currently works, the answer depends on when you ask the question:

  1. At or before 'pre_get_posts', is_category is based solely on the results of the rewrite rule, *not* on a database query. So on example.com/category/foo, when 'foo' does not exist, $this->is_category is *true* and $this->is_404 is *false*.
  2. If you ask the question after WP::handle_404(), then $this->is_category is *false* and $this->is_404 is *true*.

Which is "correct"? It depends on whether you think the is_* flags should reflect the intent of the request, or the results of the request. It seems to me that is_404 always refers to the query results - why would you _intend_ to query a 404? - so I think the latter is the case. But if I'm right about this, then is_* queries are pretty much unreliable anytime before 'wp'. See also #14729.

Alternatively, it would be possible to modify the way that parse_query() works, so that flags like is_category are set back to false if the requested object does not exist. But this will mean doing an additional database query during every page request. There are a bunch of existing tickets related to this: #31355, #27015, #21790. It is a pretty major change to the way WP_Query works, but it may be required if we're going to promise developers that they can use the conditional is_* methods at 'pre_get_posts'.

In any case, given the above, I'm against papering over the error. Developers should be aware that is_category() is not accurate at 'pre_get_posts'. If anything, we should make the error message more descriptive.

#22 in reply to: ↑ 8 @Howdy_McGee
2 years ago

In the same light here's some other ways to trigger the is_page() query errors. The deeper issue has already been pointed out by boonebgorges in #comment8 that WP is looking for non-existent feeds and is_front_page() returns errors:

1) Assign a Front page 2) Create and Publish a new page 3) Change / Update the page slug and visit /old-slug/feed/

Alternatively, if you trash the page and visit the trashed slug /page-slug/feed/ you will also trigger the errors:

Notice: Trying to get property of non-object in /wp-includes/query.php on line 4520
Notice: Trying to get property of non-object in /wp-includes/query.php on line 4522
Notice: Trying to get property of non-object in /wp-includes/query.php on line 4524

#23 @mikejolley
22 months ago

Hey, I was pointed to this ticket from https://github.com/woothemes/woocommerce/issues/11778

Seeing as the codex states that conditional functions should work after posts_selection, I shouldn't need to worry about notices being thrown when using is_singular during template_redirect hook.

At the very least, the code should check it's dealing with objects to avoid notices.

disclaimer; not looked too deep at the code yet, just adding that this is indeed a bug.

#24 @sterlo
16 months ago

Confirming this is a thing.

Notice:  Trying to get property of non-object in /var/www/html/wp-includes/class-wp-query.php on line 3516
Notice:  Trying to get property of non-object in /var/www/html/wp-includes/class-wp-query.php on line 3518
Notice:  Trying to get property of non-object in /var/www/html/wp-includes/class-wp-query.php on line 3520

How I'm getting it to generate:

<?php
  add_filter( 'pre_get_posts', function( \WP_Query $query ) {
    if ($query->is_tax() || ( $query->is_category() && ! $query->is_category('blog') )) {
        $query->set( 'orderby', [ 'title' => 'ASC' ] );
    }

    return $query;
  });

That's rather simple, shouldn't be running into trouble with something like this.

  1. Is it a taxonomy? Yes.
  2. Is it a category? Yes.
  3. Is it THIS (blog) Category? *project explodes*.

Would be nice to fix this.

Note: See TracTickets for help on using tickets.