Appending registered query vars to home URL sets is_home to true when should be false

[mordauk]

I think I've discovered a bug with how custom query vars are handled on the home page.

Assume your site is http://yoursite.com and you have a query var called foo registered with the query_vars filter, and the front page is set to a static page.

Going to http://yoursite.com loads the front page as expected and shows the contents of the static page.

Going to http://yoursite.com/?foo=1 loads the front page but instead of showing the contents of the static page, it acts like the blog page and loads the posts list.

I haven't tracked it down fully, but I believe it's because is_home is improperly set to true in query.php. is_page is also incorrectly set to false I think.

To reproduce:

1. Enable pretty permalinks
2. Set the front page to a static page
3. Register a query var:

function foo_query_vars( $vars ) {
	$vars[] = 'foo';
	return $vars;
}
add_filter( 'query_vars', 'foo_query_vars' );

4. Go to ​http://yoursite.com - This will work correctly
5. Go to ​http://yoursite.com/?foo=1 - This will be incorrect

Note, this only happens if the query var is registered with WP.

[SergeyBiryukov]

Duplicate of #18114 and #24473.

[mordauk]

I don't believe this is a duplicate. I looked at both of those tickets.

Neither of those tickets correspond to the front page breaking with custom query vars, unless I missed something.

[johnjamesjacoby]

Replying to mordauk:

I don't believe this is a duplicate. I looked at both of those tickets.

Neither of those tickets correspond to the front page breaking with custom query vars, unless I missed something.

Agree. There are comments in those tickets that elude to the problem, but they don't deal with the incorrect is_front_page calculations.

This looks like a separate (and legitimate) bug to me. Setting a page as home should never result in the blog index appearing in its place.

[dd32]

I would argue the current behaviour is expected at present, and is very close to the before mentioned duplicates.

Replying to johnjamesjacoby:

This looks like a separate (and legitimate) bug to me. Setting a page as home should never result in the blog index appearing in its place.

And the blog index isn't showing on the front page, The blog posts are showing on /?foo=custom, which is not the front page.

WordPress currently has no differentiation between a query var "which affects the current page" and one which "modifies the query", it assumes that all query vars will modify the query, which results in /?foo=custom not being equal to/? just the same that /?p=123 is not the same as /?.

If we had some way of detecting "This query var will not affect the query" then showing the front page on /?foo=custom would be appropriate.

[mordauk]

Is the answer to just not register the query vars then?

[johnjamesjacoby]

Replying to dd32:

I would argue the current behaviour is expected at present, and is very close to the before mentioned duplicates.

Replying to johnjamesjacoby:

This looks like a separate (and legitimate) bug to me. Setting a page as home should never result in the blog index appearing in its place.

And the blog index isn't showing on the front page, The blog posts are showing on /?foo=custom, which is not the front page.

WordPress currently has no differentiation between a query var "which affects the current page" and one which "modifies the query", it assumes that all query vars will modify the query, which results in /?foo=custom not being equal to/? just the same that /?p=123 is not the same as /?.

If we had some way of detecting "This query var will not affect the query" then showing the front page on /?foo=custom would be appropriate.

Sounds like the current behavior is out of alignment with the safe assumption of how it should work, regardless of our expectations. It should at least 404 instead and not show the is_home results when the user is not viewing the blog river.

The output shouldn't change just because a plugin added a variable to query_vars.

[mordauk]

I agree with JJJ.

[wonderboymusic]

Would love a patch here

[mordauk]

I just ran into this in the ​wild again.

In AffiliateWP, we allow use a URL variable to detect when an affiliate's URL is being used. The URL looks something like this: http://site.com/?ref=1

That is all well and good and works just fine, so long as we don't register ref as an actual query arg.

We recently added "pretty" affiliate URL support by adding an endpoint: add_rewrite_endpoint( $this->get_referral_var(), EP_ALL );

This results in http://site.com/?ref=1 and http://site.com/ref/1 loading the blog page since is_home gets set to true.

http://site.com/page-name/?ref=1 and http://site.com/page-name/ref/1 continue to work just fine though.

I'm going to dig back into it to see if I can work up a patch.

[dyspersion]

After a cursory look, seems to me like you wouldn't ever want to register a query_var unless it will definitely impact the DB query, and further, the prance down the template hierarchy. Otherwise, if it's just an HTTP GET variable, then I wouldn't think you would ever want/need to register it to WP as something that WP needs to know about.

Are you saying that not registering the ref var still negatively affects your add_rewrite_endpoint?

[mordauk]

Calling add_rewrite_endpoint() registers a query var automatically.

[dyspersion]

Replying to mordauk:

Calling add_rewrite_endpoint() registers a query var automatically.

Well, that's unexpected. :D

Thinking out loud...

But, it also adds a rewrite rule and uses a different (undocumented) mechanism to register the query var.

I wonder if there's a timing issue here. You have your call to add_rewrite_endpoint() attached to the init hook which may try to add the query_var to the wp object before the wp object is instantiated, maybe? It might be firing in time for the rewrite to be computed, but before $public_query_vars is even setup, maybe?

Can you verify that calling add_rewrite_endpoint() on init actually ends up in a query_var being registered and used in the actual DB query?

Additional thoughts - I see much potential confusion between the terms "query variable" and "querystring" where they involve two completely different components. One, involving a database query and one involving the end of the URL (HTTP GET), which may not interrelate at all. Registering an HTTP GET variable as a DB query_var that isn't actually used in the DB query seems unwise at best and a potential security attack vector at worst.

Also, I'd advise renaming your HTTP GET variable to something specific to your plugin. ;)

[mordauk]

Yep, confirmed that add_rewrite_endpoint() results in a query arg being registered.

The HTTP GET arg is definable in the plugin ;)

[mordauk]

I believe I've found a way to fix this that (as far as I have found) does not have any negative consequences.

It's a two step process.

1. First setup an array of the default query vars that WordPress registers

2. Inside of the existing if ( $this->is_home && 'page' == get_option('show_on_front') && get_option('page_on_front') ) { conditional, we add a second conditional for when $_query is not empty, meaning there is a query var on the home page, but one that WordPress core doesn't do anything with. We then loop through the query vars in $_query and check that none of them are default query vars from WordPress. If none of them are, that means we are dealing with a query var that has been registered by a plugin. In this case, WordPress should NOT be changing the default behavior since it does not know what the proper behavior for that query var is.

This patch is contingent on the idea that WordPress should more or less ignore query vars in the query that are not directly tied to a core behavior (front page, taxonomy, page, etc). When a plugin registers their query var, it is their responsibility to modify the query appropriately in order to get their results.

The biggest concern I have with this change is the possibility of plugins breaking that have registered a query var and are then doing something with it on the home page. As of now, they may be accustomed to getting the blog page when their query vars are present. With this patch, they would get the static front page (if one is configured).

I have tested the patch against several of my own plugins that register custom query vars and it successfully fixes the problem for them, without any break points.

[mordauk]

25143-tests.patch adds new unit tests.

[toscho]

Why is $default_query_vars public? This shouldn’t be writable from the outside, should it?

[mordauk]

Good catch, fixed.

[boonebgorges]

The biggest concern I have with this change is the possibility of plugins breaking that have registered a query var and are then doing something with it on the home page. As of now, they may be accustomed to getting the blog page when their query vars are present. With this patch, they would get the static front page (if one is configured).

Yeah, I'm concerned about this too. It seems like far too broad a fix for a very particular problem.

If the specific point is with registering endpoints, why not make it possible to call add_rewrite_endpoint() in a way that will skip registering the query var? It seems to me that the general rule should be: if you need a $_GET flag, but don't want it to affect the query, then don't register the query var; so our strategy should be to find all the places where query vars are silently registered (assuming that these places are related to features - like rewrite endpoints - that could realistically be used by plugin authors in a way that does not affect the query), and make sure it's possible to use these features without registering query vars.

[mordauk]

Allowing add_rewrite_endpoint() to work without registering a query var would also work for this. I hadn't thought of that approach!

I do still think, however, that the behavior of setting is_home to true when any customer query var is present is wrong, but at least having a way around it would be a significant improvement.

Right now, to get around it, we have to do things like this:

add_action( 'init', array( $this, 'rewrites' ) );
add_action( 'pre_get_posts', array( $this, 'unset_query_arg' ) );
add_action( 'redirect_canonical', array( $this, 'prevent_canonical_redirect' ), 0, 2 );

/**
 * Registers the rewrite rules for pretty affiliate links
 *
 * @since 1.3
 */
public function rewrites() {
	add_rewrite_endpoint( $this->get_referral_var(), EP_ALL );
}
/**
 * Removes our tracking query arg so as not to interfere with the WP query, see https://core.trac.wordpress.org/ticket/25143
 *
 * @since 1.3.1
 */
public function unset_query_arg( $query ) {
	if ( is_admin() || ! $query->is_main_query() ) {
		return;
	}
	$key = affiliate_wp()->tracking->get_referral_var();
	$ref = $query->get( $key );
	if ( ! empty( $ref ) ) {
		$this->referral = $ref;
		// unset ref var from $wp_query
		$query->set( $key, null );
		global $wp;
		// unset ref var from $wp
		unset( $wp->query_vars[ $key ] );
		// if in home (because $wp->query_vars is empty) and 'show_on_front' is page
		if ( empty( $wp->query_vars ) && get_option( 'show_on_front' ) === 'page' ) {
		 	// reset and re-parse query vars
			$wp->query_vars['page_id'] = get_option( 'page_on_front' );
			$query->parse_query( $wp->query_vars );
		}
	}
}
/**
 * Filters on canonical redirects
 *
 * @since 1.4
 * @return string
 */
public function prevent_canonical_redirect( $redirect_url, $requested_url ) {
	if( ! is_front_page() ) {
		return $redirect_url;
	}
	$key = affiliate_wp()->tracking->get_referral_var();
	$ref = get_query_var( $key );
	if( ! empty( $ref ) || false !== strpos( $requested_url, $key ) ) {
		$redirect_url = $requested_url;
	}
	return $redirect_url;
}

[boonebgorges]

Yeah, those workarounds are pretty unpleasant.

I do still think, however, that the behavior of setting is_home to true when any customer query var is present is wrong

I think what I'm suggesting is that a query_var, by definition, should modify the query. If you don't want your URL params to modify the query, then don't register them as query vars. My solution would give you a way to use WP's rewrite goodies for your URL params without also having them be query vars.

[mordauk]

Replying to boonebgorges:

Yeah, those workarounds are pretty unpleasant.

I do still think, however, that the behavior of setting is_home to true when any customer query var is present is wrong

I think what I'm suggesting is that a query_var, by definition, should modify the query. If you don't want your URL params to modify the query, then don't register them as query vars. My solution would give you a way to use WP's rewrite goodies for your URL params without also having them be query vars.

Yep, I understood that :)

Sounds like that would work fine for addressing the primary pain points here. I'll see if I can put together another patch for that.

[mordauk]

25143.3.patch Adds an additional parameter called $add_query_var to add_rewrite_endpoint() and add_endpoint() in WP_Rewrite.

This appears to work perfectly.

Now when an endpoint is registered like this:

add_rewrite_endpoint( $this->get_referral_var(), EP_ALL,  $this->get_referral_var(), false );

The home page can be properly loaded with /ref/1 appended to it.

25143-tests.2.patch contains updated unit tests for this new approach.

[boonebgorges]

Yeah, let's do something like this for 4.3.

[cklosows]

Tested out this patch. Seems to work for me just fine using the add_rewrite_endpoint() method of registering things.

[johnbillion]

It would make more sense to use the existing $query_var parameter for this, rather than introducing a fourth parameter. It could accept boolean false to disable the query var. With 25143.3.patch​, registering an endpoint with no query var requires that you pass in a useless value to the $query_var parameter.

[mordauk]

Replying to johnbillion:

It would make more sense to use the existing $query_var parameter for this, rather than introducing a fourth parameter. It could accept boolean false to disable the query var. With 25143.3.patch​, registering an endpoint with no query var requires that you pass in a useless value to the $query_var parameter.

I wanted to take advantage of that but I'm not sure we can safely do that. When $query_var is left empty, it defaults to the $name parameter. Too many people register endpoints with just two parameters for us to safely use that parameter.

[boonebgorges]

Replying to johnbillion:

It would make more sense to use the existing $query_var parameter for this, rather than introducing a fourth parameter. It could accept boolean false to disable the query var. With 25143.3.patch​, registering an endpoint with no query var requires that you pass in a useless value to the $query_var parameter.

Yeah, I was thinking something similar. The only odd part about this is that passing null would have the opposite effect of passing false, which feels strange.

Alternatively, we could change the default value from null to true, and fall back on $name when true === $query_var. This would break backward compatibility for anyone who is literally passing null to the function: add_rewrite_endpoint( $foo, $bar, null ). But ten bucks says that no one is doing that.

[mordauk]

Replying to boonebgorges:

Replying to johnbillion:

It would make more sense to use the existing $query_var parameter for this, rather than introducing a fourth parameter. It could accept boolean false to disable the query var. With 25143.3.patch​, registering an endpoint with no query var requires that you pass in a useless value to the $query_var parameter.

Yeah, I was thinking something similar. The only odd part about this is that passing null would have the opposite effect of passing false, which feels strange.

Alternatively, we could change the default value from null to true, and fall back on $name when true === $query_var. This would break backward compatibility for anyone who is literally passing null to the function: add_rewrite_endpoint( $foo, $bar, null ). But ten bucks says that no one is doing that.

That sounds like a good approach to me.

[mordauk]

25143.4.patch is an updated patch that sets $query_var to true by default and then checks if it is explicitly true before setting it to $name.

[boonebgorges]

25143.5.patch​ adds a little trick to 4.patch that ensures that anyone currently passing null to the function (probably no one, but hey) will continue to see the same behavior. I've also added some more explicit unit tests related to backward compatibility, and modified mordauk's original test to ensure that rewrite rules are flushed after registering the endpoint.

Anyone want to take a look at this and make sure we haven't missed anything?

[mordauk]

Looks good to me!

[boonebgorges]

In 32293:

Allow rewrite endpoints to be registered without also registering query vars.

Passing false to add_rewrite_endpoint() will now allow you to take
advantage of the rewrite API without thereby modifying the way that WP sets up
the main query from the request URI.

This new functionality allows developers to work around certain edge-case bugs,
such as when a proper endpoint request (such as /test/1/) would short-
circuit is_home() calculation when a static front page is set.

Props mordauk, boonebgorges.
Fixes #25143.

[johnbillion]

In 35256:

Move the test_is_home_should_be_false_when_visiting_custom_endpoint_without_a_registered_query_var_and_page_on_front_is_set() test into a more appropriate place.

See #25143

[gongwan33]

I have encountered the same problem in 4.7.2. Haven't the patch been included in the new WordPress version yet?

[seforoth]

Replying to gongwan33:

I have encountered the same problem in 4.7.2. Haven't the patch been included in the new WordPress version yet?

Can confirm same issue on the same version of WordPress.

[avengers]

This issue needs to be reopened, it still exists in 4.7.3

[boonebgorges]

@gongwan33 @seforoth @avengers See above:

This ticket was closed on a completed milestone.
If you have a bug or enhancement to report, please open a new ticket. Be sure to mention this ticket, #25143.

Please be certain to described, in detail, the following:

• What you're doing (ie, what URL you're requesting)
• What you are seeing (ie is_home being set to something unexpected)
• What you expect to be seeing