Make WordPress Core

Opened 5 years ago

Closed 21 months ago

#16373 closed defect (bug) (invalid)

Wrong page loaded requesting custom registered query_vars when correcting is_* for page_on_front requests

Reported by: jondavis Owned by: markjaquith
Milestone: Priority: high
Severity: normal Version: 3.0
Component: Query Keywords: has-patch 3.3-early needs-unit-tests
Focuses: Cc:


  • Install vanilla WP 3.0.4.
  • Register a new 'qv_test' query var in the default theme's function.php file.
	function custom_query_vars_test ($vars) {
		$vars[] = 'qv_test';
		return $vars;
  • Create a 'Home' page.
  • Under Settings → Reading set the 'Front page displays' setting to 'A static page (select below' and set the 'Front page:' setting to the 'Home' page.
  • Load the front end page.
  • Add the 'qv_test' query var to the request (e.g. http://blogurl.com/?qv_test=test)

The wrong page is loaded.

Adding an invalid query_var (one that is not registered) continues to correctly load the page.

The issue occurs regardless of permalink configuration.

This issue appears related to #12047 and is either a regression from the fixes applied to that issue, or is simply case not covered by the fixes. Changes in revision [14445] also relate to #12047.

This issue still exists as of at least 3.0.4 up to 3.1-RC3.

Attachments (2)

16373.000.diff (920 bytes) - added by jondavis 5 years ago.
Suggested fix (works for me)
test_page_on_front_query_var.diff (1.1 KB) - added by jondavis 3 years ago.
test/query.php unit test for page_on_front query vars

Download all attachments as: .zip

Change History (15)

@jondavis5 years ago

Suggested fix (works for me)

comment:1 @markjaquith5 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to markjaquith
  • Priority changed from normal to high
  • Status changed from new to accepted

Can reproduce the issue on 3.1-RC3, 3.0.4, and 3.0. As it is not a regression from 3.0, it'll have to wait until 3.2.

comment:2 @Otto424 years ago

  • Keywords 3.3-early added; front page query_vars 3.2-early removed
  • Version changed from 3.0 to 3.2.1

This looks like it got forgotten. Punting to 3.3-early, perhaps? Seems simple enough.

comment:3 @Otto424 years ago

  • Version changed from 3.2.1 to 3.0

Whoops, changing the version was not by design.

comment:4 @nacin4 years ago

if ( empty($_query) || !array_diff( array_keys($_query), array('preview', 'page', 'paged', 'cpage') ) ) {

Maybe someone with more knowledge of the history here can shed some light on what this is supposed to do, and why it avoids normal query vars.

comment:5 @dd324 years ago

  • Cc dd32 removed

Looking at the changes I made there, This case is simply a use case which was missed. It would cause the issue before the changes were made, and after. That code block is simply checking if any public query var other than those listed in array('preview', 'page', 'paged', 'cpage') are set, and if so, decides it's not the front page being viewed (as those query vars most likely map somewhere).

(also note, for anyone playing along at home $_query only contains registered query vars, /?something_not_registered=1 would still be mapped to the front page)

Jumping back before the changesets mentioned here, to [4956] where the last major change was, the bug at that point was that only / and /?preview=true would map to the front page, paging(of the page, and of comments) wouldn't work on static front page, and any public query vars being appended would load the Blog posts.

Fixing this bug however, The attached patch is basically ( empty($_query) || true ) as that array_diff should *always* return empty. I can't seem to find a use-case (with pretty permalinks enabled) which requires that if() branch to be there at all anymore, meaning, I think something else has changed in the query parsing in the last 4 years since everything -seems- to be working for me without the if. Things to check that still work with it removed: Endpoints, particually Feeds(posts, and comments)

comment:6 @jondavis3 years ago

  • Keywords needs-unit-tests added

The comments above being accurate, the issue still remains. After discussion with dd32 I will work on and submit unit tests to prove the problem, then follow-up with a more appropriate fix.

comment:7 @dd323 years ago

So the main issues here are:

  • We assume that if a Query var (other than the whitelisted vars: preview, page, paged, cpage) is set, then it's a archive/loop modifying query var
  • Query Vars do not directly map to a Query modification, It can affect plugins/Themes instead
  • We have no filters available to say that a query var is a non-query modifying query var
  • Changing this could have backwards compatibility issues -

To expand on that last one, If we simply wrapped a filter around the array of whitelisted vars, a plugin could do something such as this:

add_filter( 'singular_query_vars', function( $vars ) {
    $vars[] = 'qv_test';
    return $vars;
} );

but, that would still require multiple filters (One to add the query var, and one to set the 'context' of the query var.

Perhaps what we really need here, Is a better way to register query vars, and provide some kind of context at the same time (ie. "This query var maps to this WP_Query tag", "This query var only modifies content on a singular view", etc)

@jondavis3 years ago

test/query.php unit test for page_on_front query vars

comment:8 @jondavis3 years ago

It seems to me that creating a formal API for this situation is a bit overkill. Seems by designthat query vars shouldn't be used as a registry for plugins/themes to use as custom query string arguments. My own confusion on the issue was in my rush to move to doing things the "WordPress way" I was attempting to use the query vars as a whitelist of registered $_GET variables unnecessarily.

That said, there may be edge-cases I (or apparently anyone else) are not aware of where plugins/themes do need to use registered query vars but still expect to get page_on_front. I can't even imagine why that might be. In any event, for that use case I submitted test_page_on_front_query_var.diff​ as a use case to test against should the powers-that-be decide it is a worthwhile design consideration.

Given my new understanding of the design intent, I will be changing our plugin code to just use $_GET variables and ensure proper input sanitization is occuring.

As an action item, perhaps the result from this ticket is to update the codex to explicitly direct developers in the proper/expected/intended use-case strictly for modifying the WP_Query, and avoid any possible misunderstanding that WP_Query query vars should not be used for general purpose QUERY_STRING "query vars".

comment:9 @jondavis3 years ago

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

Closing as invalid.

comment:10 @helenyhou3 years ago

  • Milestone Future Release deleted

comment:11 @chipbennett21 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Reopening, as I found what I believe is a perfectly valid use-case for registering query variables on the front page, where the query variables aren't intended to modify the loop query.

While developing a Plugin to allow for front-end modification of Theme settings, as a Theme options "demo", I ran into this issue. The Plugin properly registers the query variables, and then sanitizes/validates and uses them, if set, to filter the front-end-modified Theme settings. Everything works as expected, unless the front page is set to display a static page. In that case, when the form is submitted, the page is refreshed and instead of displaying page_on_front, displays the blog posts index.

If this is too localized, feel free to close again. I just thought it worthy of consideration. I'm sure there are other, related use cases.

Note, should anyone else encounter this issue, the following work-around works for me:

function themeslug_force_static_front_page( $query ) {
	if ( $query->is_main_query() ) {
		if ( 'page' == get_option( 'show_on_front' ) ) {
			if ( '' != get_query_var( 'foobar' ) ) { // Registered custom query var
				$query->set( 'page_id', get_option( 'page_on_front' ) );
				$query->is_page = true;
				$query->queried_object = get_post(get_option('page_on_front') );
add_action( 'pre_get_posts', 'themeslug_force_static_front_page' );

Edit (2)

Edited to add a functional workaround, that results in is_front_page() returning true.

Last edited 21 months ago by chipbennett (previous) (diff)

comment:12 @Otto4221 months ago

I'm kinda confused here with this:

where the query variables aren't intended to modify the loop query

If the variables aren't intended to modify the main Loop, then they don't actually need to be registered as query_vars, do they?

The only thing registering a query_var does is to make the main Query pick it up and store it and have it available via get_query_var and such. Not registering it doesn't make a variable in the URL unavailable, as it's still there via the GET global and so forth.

So, the only reason to register a query_var is to have it available to the main query in some way, which would then be used to change the results of the main query, thus affecting the main Loop.

I think that this problem is primarily caused by people using the query_var system as an general means of grabbing data from the URL, when that isn't really the main intention of it. PHP already has ways to grab data from the URL, the query_var stuff is there to affect the query, not for any other purpose.

Try simply not registering your input URL variables as query_vars, unless they are affecting the Loop. Then they won't impact anything.

comment:13 @Otto4221 months ago

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