WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 3 months ago

#25143 closed defect (bug) (fixed)

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

Reported by: mordauk Owned by: boonebgorges
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.6
Component: Query Keywords: has-patch needs-testing
Focuses: Cc:

Description

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' );
  1. Go to http://yoursite.com - This will work correctly
  2. Go to http://yoursite.com/?foo=1 - This will be incorrect

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

Attachments (7)

25143.patch (1.1 KB) - added by mordauk 4 months ago.
25143-tests.patch (1.1 KB) - added by mordauk 4 months ago.
25143.2.patch (1.1 KB) - added by mordauk 4 months ago.
25143.3.patch (2.0 KB) - added by mordauk 4 months ago.
25143-tests.2.patch (744 bytes) - added by mordauk 4 months ago.
25143.4.patch (1.8 KB) - added by mordauk 4 months ago.
25143.5.patch (5.0 KB) - added by boonebgorges 3 months ago.

Download all attachments as: .zip

Change History (40)

comment:1 @SergeyBiryukov2 years ago

  • Keywords needs-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #18114 and #24473.

comment:2 follow-up: @mordauk2 years ago

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.

comment:3 in reply to: ↑ 2 ; follow-up: @johnjamesjacoby2 years ago

  • Keywords needs-patch added
  • Milestone set to Awaiting Review
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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.

comment:4 in reply to: ↑ 3 ; follow-up: @dd322 years ago

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.

comment:5 @mordauk2 years ago

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

comment:6 in reply to: ↑ 4 @johnjamesjacoby2 years ago

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.

comment:7 @mordauk2 years ago

I agree with JJJ.

comment:8 @wonderboymusic15 months ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Would love a patch here

comment:9 @mordauk10 months ago

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.

comment:10 @dyspersion10 months ago

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?

comment:11 follow-up: @mordauk10 months ago

Calling add_rewrite_endpoint() registers a query var automatically.

comment:12 in reply to: ↑ 11 @dyspersion9 months ago

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. ;)

comment:13 @mordauk9 months ago

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

The HTTP GET arg is definable in the plugin ;)

@mordauk4 months ago

comment:14 @mordauk4 months ago

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
  1. 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.

comment:15 @mordauk4 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

@mordauk4 months ago

comment:16 @mordauk4 months ago

  • Keywords needs-unit-tests removed

25143-tests.patch adds new unit tests.

comment:17 @toscho4 months ago

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

@mordauk4 months ago

comment:18 @mordauk4 months ago

Good catch, fixed.

comment:19 @boonebgorges4 months ago

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.

comment:20 @mordauk4 months ago

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;
}

comment:21 follow-up: @boonebgorges4 months ago

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.

comment:22 in reply to: ↑ 21 @mordauk4 months ago

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.

@mordauk4 months ago

@mordauk4 months ago

comment:23 @mordauk4 months ago

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.

comment:24 @boonebgorges4 months ago

  • Keywords 4.3-early added

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

comment:25 @cklosows4 months ago

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

comment:26 follow-ups: @johnbillion4 months ago

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.

comment:27 in reply to: ↑ 26 @mordauk4 months ago

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.

comment:28 in reply to: ↑ 26 ; follow-up: @boonebgorges4 months ago

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.

comment:29 in reply to: ↑ 28 @mordauk4 months ago

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.

@mordauk4 months ago

comment:30 @mordauk4 months ago

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.

@boonebgorges3 months ago

comment:31 @boonebgorges3 months ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

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?

comment:32 @mordauk3 months ago

Looks good to me!

comment:33 @boonebgorges3 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.