Custom hierarchical post type 404s when `get_queried_object` is called too early

[gradyetc]

I uncovered some confusing behavior while debugging a recent plugin issue -- a hierarchical custom post type was 404'ing unexpectedly. I tracked the problem down to the usage of is_singular() in a parse_query callback.

Steps to Reproduce

1. Enable pretty permalinks
2. Register a hierarchical custom post type
3. Hook in to the parse_query hook and call get_queried_object() or any function that sets $query->queried_object_id, e.g.:

<?php

function custom_parse_query( $query ) {
	// Will be NULL
	$post = get_queried_object();

	// Will throw PHP Notice and return false
	if ( $query->is_singular( 'foo' ) ) {
		// Whip up some custom query magic
	}
}

add_filter( 'parse_query', 'custom_parse_query' );

4. Get 404s for all singular requests for that post type

Root cause is special handling of queried_object / queried_object_id for single page requests in ​WP_Query#parse_query and ​WP_Query#get_posts.

I searched for warnings about using conditional tags inside WP_Query hook callbacks (e.g. parse_query, pre_get_posts, etc.) and found this in ​the Codex page on conditional tags:

Warning: You can only use conditional query tags after the posts_selection action hook in WordPress (the wp action hook is the first one through which you can use these conditionals). For themes, this means the conditional tag will never work properly if you are using it in the body of functions.php, i.e. outside of a function.
However: if you have a reference to the query object (for example, from within the parse_query or pre_get_posts hooks), you can use the WP_Query conditional methods (eg: $query->is_search())

Which is misleading IMHO; while some of them work (is_archive(), is_tax()), is_singular() and other single post-related checks will not until WP_Query#get_posts has finished and the queried object can be determined.

Some thoughts...

• A quick fix would be to unset queried_post_id in WP_Query#get_queried_post() if it was called too early.
• A better approach might be to calculate singular request IDs earlier so that conditional checks would return meaningful results in query hook callbacks. If we're willing to make the extra query to determine the queried page in WP_Query#parse_query (via get_page_by_path()), why not do the same for all post types?
• Otherwise, we should update the docs to clarify the reality: $query->is_single(), $query->is_singular(), $query->is_page() and $query->is_attachment() should not be used in callbacks for parse_query, pre_get_posts, etc.

[gradyetc]

Unit tests demonstrating the issue with a potential patch

[gradyetc]

Patch also includes a few checks in the is_* methods that prevent PHP Notices that were throwing Exceptions during test runs.

[boonebgorges]

gradyetc - Thanks for the extremely thorough report.

The fact that we set $queried_object_id in parse_query() is, as you note, the source of the anomaly. IMO, the fact that the query takes place here is a kind of bug. In theory, no queries should take place until after the query has been fully parsed; at 'parse_query' and 'pre_get_posts', you'd be working with a query object whose properties were fully set, but where no results had yet been fetched. You have, in effect, suggested the opposite - that we should perform *all* single object queries in parse_query(), regardless of post type - but this seems like it only passes the buck to the next developer who is confused at the proper order of WP's bootstrap.

The current behavior was introduced way back when [4956] as a way to support page_on_front. I just spent a little while trying to back it out - basically, converting the get_page_from_path() query in parse_query() so that it only sets a local variable, and then letting the actual page query take place in get_posts(). This would remove the exceptional behavior of single pages, and it would make it so that the is_*() functions would be unusable for *all* post types until after the full query had taken place. But making this change work requires some oddball property juggling, and I'm not sure that it won't introduce backward compatibility issues for plugins that expect 'pagename' to have been converted to $queried_object_id at pre_get_posts.

Given the complexity of the situation, I'm leaning toward leaving the code the way it is, and updating the documentation so that we officially don't support the use of the conditional is_*() functions at 'pre_get_posts' or before. What do you think?

[SergeyBiryukov]

Related: #21790, #27015.

[gradyetc]

Thanks for the review, Boone!

I agree with your assertion that the exceptional behavior for pages is the bug, not the other way around as my proposal implies. I wouldn't expect a "queried object" to be available before a query is run. (Though it is for non-singular requests).

I think the thing that threw me off in this case was the hidden dependency on $queried_object_id being unset and the unexpected side effect calling is_singular() had on the query. In fact the code I reference didn't actually need the is_singular() check; it just happened to be present in a function that was being used to detect whether or not a query was for a specific post type. (The parse_query hook was being used to modify sort order for archive queries of that post type).

In practice I suspect this isn't something that people run into all that often, and updating the documentation (and perhaps a _doing_it_wrong()?) would be sufficient IMHO.

[jazbek]

Don't initialize queried object id to 0 when getting it

[jazbek]

I have also just run into this bug.

As a simple fix, I'm wondering why we're setting $this->queried_object_id = 0; in $wp_query->get_queried_object() before we attempt to find the queried object?

​https://github.com/WordPress/WordPress/blob/master/wp-includes/query.php#L3929

Can we not set it to null, or unset it before we attempt to find it? Setting it to 0 gives it a value, which seems incorrect if the query hasn't been run, or if there is no queried object.

As an aside, I found this SO question with a number of people who have run into this bug, so it's not completely obscure

:
​http://stackoverflow.com/questions/25740115/post-type-hierarchical

[boonebgorges]

Can we not set it to null, or unset it before we attempt to find it? Setting it to 0 gives it a value, which seems incorrect if the query hasn't been run, or if there is no queried object.

Ah. It took me a few minutes to wrap my head around this. I assume that $this->queried_object_id = 0 was originally introduced an attempt to avoid PHP notices. Setting it to 0 means that it passes the isset check here https://core.trac.wordpress.org/browser/tags/4.3/src/wp-includes/query.php?marks=2610#L2606, which means that the request passes the weird check on line 2634, which means that "AND ($wpdb->posts.ID = 0)" gets appended to the WHERE clause of the query, which means that everything breaks. (You all already probably traced this. I'm writing it out here for posterity :) )

Changing the default value to null means that the isset() check *fails* (as expected).

It's still weird that objects have been queried at all by this point, but I think jazbek is correct that we can use null as a simple fix. gradyetc also pretty much suggested this in the ticket description too :) Let's go with it.

[boonebgorges]

In 34073:

Better default values in WP_Query::get_queried_object().

Setting the default value of the queried_object_id property to 0 meant
that, when called early enough in the WP bootstrap, get_queried_object()
could short-circuit the normal query by fooling it into thinking that the
request was for a page with id 0. Setting the default value to null instead
avoids this problem.

Props gradyetc, jazbek.
Fixes #31355.

[jazbek]

Thank you for typing out that trace Boone, yes that's exactly what I meant -- was a long day and I forgot everyone couldn't read my mind. :)

Appreciate the acceptance.