Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#17040 closed defect (bug) (fixed)

Completely ignore ?post_type=page

Reported by: scribu's profile scribu Owned by: markjaquith's profile markjaquith
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: has-patch
Focuses: Cc:

Description

Follow-up to #17033

If you go to ?post_type=page, you will get a list of pages, displayed as posts, even though the 'page' post type doesn't have 'has_archive' => true.

That is very unexpected behaviour.

The 'post_type' arg should be stripped out before it reaches $wp_query, so that visitors can't access this view, unless explicitly enabled by a plugin.

Attachments (4)

17040.diff (2.6 KB) - added by scribu 13 years ago.
block-page-post_type-query.diff (532 bytes) - added by chrisbliss18 13 years ago.
17040.chrisbliss18.2.diff (3.7 KB) - added by chrisbliss18 13 years ago.
This patch changes publicly_queryable to only apply to direct post_type query arg requests. Changed public to be used in its place in appropriate locations.
17040.2.diff (3.6 KB) - added by nacin 13 years ago.

Download all attachments as: .zip

Change History (33)

#1 @scribu
13 years ago

Huh, in class WP, 'post_type' is set as both a public and a private query var.

@scribu
13 years ago

#2 @scribu
13 years ago

  • Keywords has-patch added; needs-patch removed

#3 @scribu
13 years ago

  • Milestone changed from Future Release to 3.2

Moving to 3.2, as this is technically a regression, introduced along with CPT archives in WP 3.1.

#4 @nacin
13 years ago

This isn't a regression. Not from 3.1 anyway.

We shouldn't trip out post_type. That'd create a regression.

We should selectively block post_type=page.

#5 @nacin
13 years ago

  • Version set to 3.0

#6 @nacin
13 years ago

  • Keywords has-patch removed

#7 @scribu
13 years ago

Please describe the regression that 17040.diff would introduce.

#8 @nacin
13 years ago

Querying ?post_type=my_custom_post_type where there is no has_archive, i.e. a CPT archive page I rolled myself pre-3.1.

#9 @chrisbliss18
13 years ago

  • Cc gaarai@… added
  • Keywords has-patch added

I may be oversimplifying things, but isn't all that's needed is a modification to the arguments used to register the page post type? The publicly_queryable argument is available but it defaults to true if public is true, which it is for pages.

I'm attaching a patch showing what I'm talking about.

#10 @scribu
13 years ago

Way to think outside the box. :)

We should still fix 'post_type' being set as both a private and a public query var, though.

Last edited 13 years ago by scribu (previous) (diff)

#11 @chrisbliss18
13 years ago

Yeah. I noticed that while taking a gander at class-wp.php. Odd to say the least.

I haven't dug into that one to find out the possible consequences of changing it.

#12 @nacin
13 years ago

Clever, but that will do some damage. We use publicly_queryable in a few different ways.

#13 @chrisbliss18
13 years ago

To me, publicly_queryable should do what it says on the tin: allow the post type to be queried publicly. If that isn't all that it means, then it stands to reason (IMHO) that those additional uses should be broken out to separate arguments.

Is there a reason that this does more than just allow the post type to be queried publicly?

#14 @nacin
13 years ago

To me, pagename=about or page_id=2 is querying that post type publicly. Perhaps I'm wrong in that thought.

See #13301, #14321.

#15 @nacin
13 years ago

Unless I'm missing something, I would not object to walking that back, so publicly_queryable is for WP() only, and the other checks in core (there's a few) are moved to public.

#16 @chrisbliss18
13 years ago

Dang. I wasn't think of using query vars for that. I had my site set up with permalinks and those worked just fine with it set to false.

Seems like the string for has_archive might have to be tugged to disallow public queries for archives of post types with that set to false. Thoughts?

#17 @nacin
13 years ago

Even with has_archive => false, post_type=foo should still work, where foo != page.

I like your idea of re-defining publicly_queryable to being the query var itself and restricted to the WP class. We should revert other changes and move them to public.

@chrisbliss18
13 years ago

This patch changes publicly_queryable to only apply to direct post_type query arg requests. Changed public to be used in its place in appropriate locations.

#18 @chrisbliss18
13 years ago

@nacin -- Hmm... And here I thought I was completely off base and didn't know what I was talking about. Go figure.

I made a patch based upon what you said. I'm not terribly familiar with modifying the query structures, but I believe it does what you were suggesting:

  1. It replaces the publicly_queryable arg with public in places where decisions are made on whether or not a post type is able to be displayed publicly.
  2. It changes the page post type to have publicly_queryable set to false.

I did some tests on this and it allowed for queries of ?page_id and ?pagename but produced a home page query when ?post_type=page was requested.

It definitely needs more testing though by someone much more capable in this regard than myself.

#19 @nacin
13 years ago

A home page query when ?post_type=foo is requested, when has_archive = false, is expected. The important point is that the query does not look for pages; rather, it is ignored.

I also just wrote the same patch, so I've uploaded it.

@nacin
13 years ago

#20 @markjaquith
13 years ago

This is interesting, but I think it's too big of a low level change to make during beta.

#21 @nacin
13 years ago

Sounds fine. I'd like to change from publicly_queryable to public now -- or at least on the checks we introduced during 3.2. The others were all on 3.1, those should also be safe. The pages bit, that can wait.

#22 follow-up: @scribu
13 years ago

I don't really understand the need to redefine publicly_queryable.

Even with has_archive => false, post_type=foo should still work, where foo != page.

It should work, as long as it's paired with name=bar, i.e. when viewing a single post.

Last edited 13 years ago by scribu (previous) (diff)

#23 in reply to: ↑ 22 @nacin
13 years ago

Replying to scribu:

I don't really understand the need to redefine publicly_queryable.

It's not redefining it, it's restoring its original intention.

Even with has_archive => false, post_type=foo should still work, where foo != page.

It should work, as long as it's paired with name=bar, i.e. when viewing a single post.

No, that's incorrect. ?post_type=foo must be allowed to work on non-single posts. The sole exception is when post_type=page.

#24 @scribu
13 years ago

For my own reference (each item in the list implies the previous one):

  • public: ?post_type=foo&name=bar
  • publicly_queryable: ?post_type=foo
  • has_archive: rewrite rules and query flags

The docblock for 'public' should be updated, as it's currently the same as show_ui.

Version 0, edited 13 years ago by scribu (next)

#25 @scribu
13 years ago

Related: #17609

#26 @scribu
13 years ago

  • Component changed from General to Post Types

#27 @nacin
13 years ago

Even if we don't commit the register_post_type call, which we should, I'd like to change every incorrect publicly_queryable reference to public. And if we can't do that, then let's at least change the ones we did in 3.2.

#28 @markjaquith
13 years ago

After some discussion with nacin and some helpful clarity from scribu's comment, I think this should go in. The use of publicly_queryable was a mistake.

#29 @markjaquith
13 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In [18234]:

Fix mistaken use of publicly_queryable when public was what was intended. props nacin. fixes #17040

Note: See TracTickets for help on using tickets.