Allow private taxonomies (public = false)

[wpsmith]

In a discussion with jaredatch & nacin, we discovered that the public arg is not working properly within WordPress query.

So I merged Limiting publicly queried post_types to those that are publicly_queryable with limiting publicly queried taxonomies to those that are public instead of just duplicating the code and making it for taxonomies.

[wpsmith]

First pass

[nacin]

This is probably easier as two separate foreach()s. Also, let's avoid having publicly_queryable for taxonomies, as it is a mess.

We never coded 'public' to work (it is marked as a @todo in register_taxonomy()) so setting this to be an enhancement.

That said, it's a solid enhancement, so let's do it in 3.5.

[wpsmith]

2nd pass

[wpsmith]

Added a helper method _limit_public() to reduce redundant code. However, this checks 'taxonomy' and '{taxonomy_name}' to determine whether the taxonomy is public removing if not. If 'taxonomy' is set (instead of '{taxonomy_name}'), then term is removed.

I tested the following successfully basic queries:
​http://localhost:8888/www/wordpress-trunk/?taxonomy=wps_theme_colors&term=blue
​http://localhost:8888/www/wordpress-trunk/?wps_theme_colors=blue

[wpsmith]

3rd pass. Breaks out everything to own section. Removed helper method.

[nacin]

How does 21949.diff​ look?

[jaredatch]

Seems to do the trick. +beer @nacin and @wpsmith!

[nacin]

Could use some additional testing of all of the scenarios.

[ericlewis]

In 21949.1.diff​, I did a couple things.

Reworked the key => value variable usage in the first foreach loop (the key variable wasn't being used, and had misleading naming).

Fixed a variable naming typo ($queryiable_taxonomies should be $queryable_taxonomies).

I also removed the urlencoding, because it's unnecessary, isn't it? Since we're within the WP class, the query vars are being populated directly from the URL, and literal spaces are not part of valid URLs.

[wonderboymusic]

Patch still applies cleanly, let's look at it

[nacin]

This appears to be unit-testable.

[wonderboymusic]

No unit tests

[samuelsidler]

Would be great to get some unit tests for this so it can go into core, especially if the REST API needs it.

[boonebgorges]

21949.2.diff​ adds a couple simple unit test.

I also simplified the logic quite a bit. Unless I'm totally missing something, parse_request() does not, and never did, accept an array of values for the 'taxonomy' parameter. Thus example.com?taxonomy=foo,bar and example.com?taxonomy[]=foo&taxonomy[]=bar are both invalid, and we don't need to handle these cases.

Also, I'm not sure I understand why nacin suggested an is_admin() exception. We don't appear to make a similar exception anywhere else when parsing the request, and I'm not sure what the justification would be. (If we need exceptions, they should probably be based on caps.) In any case, parse_request() doesn't run during a normal admin pageload. Maybe I'm missing something obvious here?

[nacin]

@boonebgorges: I believe my thought process was the use of wp() on wp-admin/edit.php, via wp_edit_posts_query(). The taxonomy would still need to be queryable in the admin in order to apply a filter.

[boonebgorges]

In 34247:

Allow taxonomies to be non-public.

[13216] introduced the 'public' argument for register_taxonomy(). This param
was used to set defaults for 'show_ui' and a number of other params, but it
never did anything itself.

With this changeset, taxonomies registered with public=false will no longer
be queryable on the front end, ie via taxonomy archive queries.

Props wpsmith, ocean90, nacin, ericlewis, boonebgorges.
Fixes #21949.

[johnbillion]

In 34516:

Add a @since entry for the implementation of the $public argument in register_taxonomy().

See #21949

[mboynes]

We may want to re-think this implementation; this commit broke a section of a site I built.

I had a semi-public taxonomy where the taxonomy terms were used in url structures for posts, but not accessed directly. I set public => false in this taxonomy because I didn't want the terms ever being queried directly. For the purposes of my rewrites, I added a rewrite tag with the same name as the taxonomy (so it was clear that there was a relationship between the rewrite tag and the taxonomy). This update unsets the query var that my rewrite tag added because the rewrite tag and the taxonomy shared the same name.

My usage here may be fairly exceptional, but I'm a little concerned that there are situations where a private taxonomy inadvertently shares a name with a query var, and this update will now unset that query var.

Looking over the fix, is there any reason why we're letting register_taxonomy() add the query var, and then later unsetting it? Why not simply prevent register_taxonomy() from adding it? We could replace...

if ( false !== $args['query_var'] && ! empty( $wp ) ) { ... }

... with...

if ( false !== $args['query_var'] && false !== $args['public'] && ! empty( $wp ) ) { ... }`

... and that should prevent the issue I encountered while simultaneously simplifying the fix for this issue. Thoughts?

[boonebgorges]

Hey mboynes - Sorry for any unintended breakage.

My first impression is that your technique is kinda weird :) The 'public' argument previously didn't do anything except provide a default for 'show_ui' and 'show_in_nav_menus', and it *didn't* do what you wanted it to do, namely to prevent the terms from being queried.

As for your suggestion: I assume that the reason why we allow non-public taxonomies to register rewrite rules and then control access during parse_request() is because *post types* do it that way, and as far as I can see, have done it that way since the feature was introduced [12923]. So the question is: is there any legitimate reason why one would want to register a rewrite rule that can never be tripped because of a block in parse_request()? I can think of a couple possibilities: (1) rewrite rules are not generated on the fly, which means that switching a post_type/taxonomy to public would not automatically register the rules; (2) currently, you can use the 'parse_request' action to override some of the access protection in parse_request(), a task which would be (arguably) more complex if the rewrite rules weren't registered.

I guess I don't have a particular problem making the change that mboynes is suggesting, but it'd be nice to have someone else weigh in. Is there a reason, other than "we've always done it this way", why we let non-public post types register their rewrite rules?

[DrewAPicture]

@boonebgorges What's left here?

[boonebgorges]

Hi @mboynes - Rereading the above, I think I misunderstood what was being suggested. Rewrite rules and query vars are registered separately. And post types do indeed use the technique you've suggested for deciding whether to register the query var - they use the new is_post_type_viewable().

We still need to keep some logic in parse_request() to detect whether the nonpublic taxonomy is set as the value of 'taxonomy' - this can't be handled at the time that the taxonomy is registered. It's a bit awkward to have the 'public' protection in two different places depending on the query var structure, but I think it's what we have to do if we want to allow the registration of custom query vars that match nonpublic taxonomy names. See [21949.3.diff].

[boonebgorges]

In 35333:

Prevent non-public taxonomies from registering aquery var.

[34247] made the 'public' paramater of register_taxonomy() work by blocking
requests for non-public taxonomy archives during parse_request(). Blocking
taxonomy archive requests this late means that it's impossible to register an
independent query var that matches the slug of a non-public taxonomy. By
moving the block to register_taxonomy() - not allowing these taxonomies to
register their query vars in the first place - we free up the slug for other
use. In addition, we free up a bit of processing (no need to look for the query
var in parse_request() and better parallel the way non-public post types
work. See register_post_type().

Non-public taxonomy archives that are requested using ?taxonomy=tax_name are
still blocked during parse_request. It's only custom query vars -
?tax_name=term - that are affected by this change.

Props mboynes.
Fixes #21949.

[Chouby]

Just a comment from a developper doing weird things with taxonomies.

In a plugin, I use a non public taxonomy with a query var because my understanding of the register_taxonomy arguments were as follows:
'query_var' => 'foo' tells WP to handle the taxonomy in queries
'public' => false tells themes developpers "please don't display terms on public site"
'show_ui' => false tells WP not to use the default ui.

Now I will of course set public => true for my taxonomy but how to tell themes developpers not to display the taxonomy terms?

[boonebgorges]

Replying to Chouby:

'public' => false tells themes developpers "please don't display terms on public site"
...
Now I will of course set public => true for my taxonomy but how to tell themes developpers not to display the taxonomy terms?

Can you say more about how this previously worked? In what sense did public => false "tell" theme developers not to display the terms? Was it just by convention and politeness :) ? Or did public => false previously prevent theme devs from doing something in a programmatic way?

[Chouby]

Just a convention.

Fortunately, most themes authors display only taxonomy they know (i.e. default taxonomies) as done by default themes.

But a few themes authors think "Oh! Plugins can add their own taxonomies, so let's display them". get_taxonomies() to build the list and it's done ;-)

Then I have users coming to the forum: "Hey I have weird "categories" displayed aside my categories and post tags. How to remove them?" and I use to answer: "Please ask your theme author not to display non public taxonomy". Using get_taxonomies( array( 'public" => true ) ) instead of get_taxonomies() made the trick until now.

[GregLone]

@Chouby, can't you use 'public' => false, 'publicly_queryable' => true when declaring your taxonomy?

[Chouby]

'publicly_queryable' is not an argument of register_taxonomy() though it is available for register_post_type(). It would indeed make sense to have the same arguments making the same thing for both functions.

[GregLone]

Oh yes, my bad, confusion.
I guess in the worst case scenario you'll need to add the query var and rewrite rules yourself after registering the taxonomy. It doesn't feel weird to me, your case is a bit edgy (I guess you're talking about Polylang).

[boonebgorges]

Yeah, I think a publicly_queryable param, which inherits its default from public, is probably the answer (for this reason among others). Please feel free to open an enhancement ticket along these lines.

[Chouby]

Done :)
See #34491

[Chouby]

I'd like to reopen this ticket as it introduces a backward incompatibility.

In WP 4.3.1, setting 'public' => false, 'query_var' => 'lang' allows to completely hide the taxonomy on admin side and to query the taxonomy on frontend.

In WP 4.4, I need to set 'public' => true in order to be able to query the taxonomy on frontend but setting 'show_ui' => false is not sufficient to completely hide the taxonomy on admin side. The taxonomy appears among the nav menus metaboxes. This is confusing for Polylang users (I already got some feedback from a user testing the dev version of Polylang with WP 4.4 beta).

#34491 is one solution to solve this backward compatibility issue.

Another solution could be to replace 'public' => false by 'show_ui' => false everywhere on admin side, but I fear that it could introduce other side effects.

[boonebgorges]

The taxonomy appears among the nav menus metaboxes.

We have a parameter for this: show_in_nav_menus. By default, it inherits public, but you should set it to false. Can you let me know whether this is sufficient?

[Chouby]

Ooops... Yes of course you are right! I missed that parameter. Sorry for bothering you. Please re-close the ticket.

[boonebgorges]

Cool, thanks, Chouby!

[Chouby]

'public' => false tells themes developpers "please don't display terms on public site"
...
Now I will of course set public => true for my taxonomy but how to tell themes developpers not to display the taxonomy terms?

I just updated Polylang to be ready for WordPress 4.4 and thus replaced 'public' => false by 'public' => true, 'show_ui'=> false, 'show_in_nav_menus' => false and already got my first support request for the languages "tags" being displayed. Because the convention not to display non public taxonomies seems not to be just mine. See: ​https://themes.trac.wordpress.org/browser/customizr/3.4.15/inc/parts/class-content-post_metas.php#L467

[jeremyfelt]

Reopening for what looks to be the opposite of what was raised in 37.

Replying to boonebgorges:

Also, I'm not sure I understand why nacin suggested an is_admin() exception. We don't appear to make a similar exception anywhere else when parsing the request, and I'm not sure what the justification would be. (If we need exceptions, they should probably be based on caps.) In any case, parse_request() doesn't run during a normal admin pageload. Maybe I'm missing something obvious here?

This threw me for a bit of a loop today with a taxonomy that's only used to organize content in the admin.

Given a taxonomy registered with:

array(
    'public' => false,
    'show_ui' => true,
    'show_in_menu' => true,
    'show_admin_column' => true,
)

In 4.3.1:

• http://foo.bar/?custom_taxonomy=term_slug - The query variable is ignored, as expected.
• http://foo.bar/wp-admin/edit.php?custom_taxonomy=term_slug shows only posts from that taxonomy/term combo in the list table.

In 4.4:

• http://foo.bar/?custom_taxonomy=term_slug - The query variable is ignored, as expected.
• http://foo.bar/wp-admin/edit.php?custom_taxonomy=term_slug shows all posts in the list table.

I would expect that the posts list table continues to allow for filtering via the taxonomy/term in the admin without making the taxonomy public.

I can also justify any meaning for the public argument when I stare at it too long, so it may be that just documenting this here is enough. :)

[boonebgorges]

I can also justify any meaning for the public argument when I stare at it too long, so it may be that just documenting this here is enough. :)

The history of this ticket is a great lesson in why we should not introduce function parameters with a @todo implement annotation. Without an enforced definition, please turn the parameter into whatever they'd like.

That being said: As far as the admin/non-admin stuff is concerned, we ought to be consistent. [34247] made it so that ?taxonomy=foo&term=bar worked on the admin even if foo is non-public. So we should do the same thing for ?foo=bar. See 21949.4.diff​. @jeremyfelt Does this seem right to you?

[jeremyfelt]

Replying to boonebgorges:

See 21949.4.diff​. @jeremyfelt Does this seem right to you?

Yep, this does the trick and looks sane. +1

[boonebgorges]

In 35680:

Allow filtering by non-public taxonomies on edit.php.

[35333] enforces protection for taxonomies by preventing non-public taxonomies
from registering query_vars in register_taxonomy(). This broke the use of
taxonomy query_vars on edit.php, breaking backward compatibility and
creating inconsistency with the treatment introduced in [34247], which allowed
taxonomy=foo filtering on the Dashboard, even when foo is non-public. In
this changeset, we make the same Dashboard exception for the query_var.

Fixes #21949.