WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 6 months ago

#25143 reopened defect (bug)

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

Reported by: mordauk Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.6
Component: Query Keywords: needs-patch needs-unit-tests
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.

Change History (13)

comment:1 @SergeyBiryukov19 months 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: @mordauk19 months 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: @johnjamesjacoby19 months 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: @dd3219 months 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 @mordauk19 months ago

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

comment:6 in reply to: ↑ 4 @johnjamesjacoby19 months 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 @mordauk19 months ago

I agree with JJJ.

comment:8 @wonderboymusic11 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 @mordauk6 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 @dyspersion6 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: @mordauk6 months ago

Calling add_rewrite_endpoint() registers a query var automatically.

comment:12 in reply to: ↑ 11 @dyspersion6 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 @mordauk6 months ago

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

The HTTP GET arg is definable in the plugin ;)

Note: See TracTickets for help on using tickets.