WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#21949 closed enhancement (fixed)

Allow private taxonomies (public = false)

Reported by: wpsmith Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch rest-api
Focuses: Cc:

Description

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.

Attachments (9)

21949.wp-class (1.7 KB) - added by wpsmith 8 years ago.
First pass
21949.patch (1.7 KB) - added by ocean90 8 years ago.
21949.wp-class.2.patch (3.2 KB) - added by wpsmith 8 years ago.
2nd pass
21949.wp-class.3.patch (2.2 KB) - added by wpsmith 8 years ago.
3rd pass. Breaks out everything to own section. Removed helper method.
21949.diff (1.8 KB) - added by nacin 8 years ago.
21949.1.diff (1.7 KB) - added by ericlewis 8 years ago.
21949.2.diff (2.5 KB) - added by boonebgorges 5 years ago.
21949.3.diff (2.8 KB) - added by boonebgorges 5 years ago.
21949.4.diff (782 bytes) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (54)

@wpsmith
8 years ago

First pass

#1 @nacin
8 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from Awaiting Review to 3.5
  • Severity changed from major to normal
  • Type changed from defect (bug) to enhancement

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.

#2 @johnbillion
8 years ago

  • Cc johnbillion added

#3 @jaredatch
8 years ago

  • Cc jared@… added

@ocean90
8 years ago

@wpsmith
8 years ago

2nd pass

#4 @wpsmith
8 years ago

  • Keywords dev-feedback has-patch added; needs-patch removed

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
8 years ago

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

@nacin
8 years ago

#5 @nacin
8 years ago

  • Keywords dev-feedback removed

How does 21949.diff look?

#6 @jaredatch
8 years ago

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

#7 @nacin
8 years ago

  • Keywords commit added

Could use some additional testing of all of the scenarios.

#8 @nacin
8 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release
  • Summary changed from public in register_taxonomy is not working properly to Allow private taxonomies (public = false)

#9 @helen
8 years ago

  • Milestone changed from Future Release to 3.6

#10 @sabreuse
8 years ago

  • Cc sabreuse added

@ericlewis
8 years ago

#11 @ericlewis
8 years ago

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.

#12 @ryan
7 years ago

  • Milestone changed from 3.6 to Future Release

#13 @wonderboymusic
7 years ago

  • Milestone changed from Future Release to 3.7

Patch still applies cleanly, let's look at it

#14 @nacin
7 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.7 to 3.8

This appears to be unit-testable.

#15 @wonderboymusic
7 years ago

  • Keywords 3.9-early added; 3.6-early removed
  • Milestone changed from 3.8 to Future Release

No unit tests

#16 @DrewAPicture
7 years ago

  • Cc xoodrew@… added

#17 @rmccue
5 years ago

  • Keywords rest-api added

#18 @samuelsidler
5 years ago

  • Keywords 4.4-early added; commit 3.9-early removed

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

@boonebgorges
5 years ago

#19 follow-up: @boonebgorges
5 years ago

  • Keywords needs-unit-tests 4.4-early removed
  • Milestone changed from Future Release to 4.4

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?

#20 @wonderboymusic
5 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

#21 @nacin
5 years ago

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

#22 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#23 @johnbillion
5 years ago

In 34516:

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

See #21949

#24 @mboynes
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

#25 @boonebgorges
5 years ago

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?

#26 @DrewAPicture
5 years ago

@boonebgorges What's left here?

#27 @boonebgorges
5 years ago

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
5 years ago

#28 @boonebgorges
5 years ago

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

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.

#29 follow-up: @Chouby
5 years ago

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?

#30 in reply to: ↑ 29 ; follow-ups: @boonebgorges
5 years ago

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?

#31 in reply to: ↑ 30 @Chouby
5 years ago

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.

#32 follow-up: @GregLone
5 years ago

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

#33 in reply to: ↑ 32 @Chouby
5 years ago

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

#34 @GregLone
5 years ago

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

#35 follow-up: @boonebgorges
5 years ago

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.

#36 in reply to: ↑ 35 @Chouby
5 years ago

Done :)
See #34491

#37 @Chouby
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#38 @boonebgorges
5 years ago

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?

#39 @Chouby
5 years ago

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

#40 @boonebgorges
5 years ago

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

Cool, thanks, Chouby!

#41 in reply to: ↑ 30 @Chouby
5 years ago

'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

#42 in reply to: ↑ 19 @jeremyfelt
5 years ago

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
5 years ago

#43 follow-up: @boonebgorges
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

#44 in reply to: ↑ 43 @jeremyfelt
5 years ago

Replying to boonebgorges:

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

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

#45 @boonebgorges
5 years ago

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

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.

Note: See TracTickets for help on using tickets.