Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35089 closed defect (bug) (fixed)

Query var on non-public taxonomy remains boolean true since [35333]

Reported by: johnbillion's profile johnbillion Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Taxonomy Keywords: fixed-major
Focuses: Cc:

Description

Still trying to pin down the root cause of this one. Steps to reproduce:

  1. Register a non-public custom taxonomy with the following code:
register_taxonomy( 'foo', 'post', array(
	'public'    => false,
	'query_var' => true,
) );
  1. Call the following code on the front end of your site:
var_dump( get_taxonomies( array(
	'query_var' => 'bar',
), 'objects' ) );

Note that the output erroneously contains the foo taxonomy, despite its query var not matching. The taxonomy's query var remains as boolean true, whereas in <=4.3 it was converted to the taxonomy name (foo in this example).

The query var for a non-public taxonomy should probably be set to boolean false in this case.

This seems to have been introduced in [35333]. The problem does not occur in the admin area due to a tweak introduced in [35680].

Attachments (1)

35089.diff (1.2 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (9)

#1 follow-up: @boonebgorges
8 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch needs-unit-tests removed

Thanks for the ticket, @johnbillion.

It was decided in [35333] that the best way to prevent non-public taxonomies from being queryable on the front-end was to prevent them from registering a query_var. This is similar to the strategy that's used for non-public post types.

IMO it's a little bit weird to combine query_var = true with public = false. That being said, this does look like a legitimate bug.

The root problem, as I see it, is that wp_list_filter() (via wp_filter_object_list()) uses non-strict comparison ==, so that true == 'bar'. https://core.trac.wordpress.org/browser/tags/4.4/src/wp-includes/functions.php?marks=3332#L3302 I understand why we can't switch to strict comparison here, but I wonder how much stuff would break if wp_list_filter() explicitly avoided comparing booleans and non-empty strings - it's hard to imagine a legitimate use for this.

It's probably unlikely that we can change the behavior of wp_list_filter() or even get_taxonomies() here. So maybe a more local fix is that, in register_taxonomy(), non-public taxonomies have query_var forced to false. It's not very attractive, but it should minimize fallout. See 35089.diff.

@boonebgorges
8 years ago

#2 in reply to: ↑ 1 ; follow-up: @johnbillion
8 years ago

Replying to boonebgorges:

IMO it's a little bit weird to combine query_var = true with public = false.

Indeed it is. I only came across this bug because Jigoshop does this with its shop_order_status taxonomy.

Last edited 8 years ago by johnbillion (previous) (diff)

#3 in reply to: ↑ 2 ; follow-up: @boonebgorges
8 years ago

Replying to johnbillion:

Indeed it is. I only came across this bug because Jigoshop does this with its shop_order_status taxonomy.

Do you know off the top of your head why Jigoshop sets query_var = true? Will 35089.diff break something for them?

#4 @jorbin
8 years ago

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

#5 in reply to: ↑ 3 @boonebgorges
8 years ago

  • Keywords 2nd-opinion removed

Replying to boonebgorges:

Replying to johnbillion:

Indeed it is. I only came across this bug because Jigoshop does this with its shop_order_status taxonomy.

Do you know off the top of your head why Jigoshop sets query_var = true? Will 35089.diff break something for them?

I checked it out, and my guess is that Jigoshop does this only because of copy-and-pasting register_taxonomy() arguments. The plugin doesn't actually expect to see the query_var for non-public taxonomies anywhere.

I think that 35089.diff is the right move here: prior to 4.4, it was impossible for a query_var to be set to a boolean true, and it's true values that are causing the wp_list_filter() issues.

Last edited 8 years ago by boonebgorges (previous) (diff)

#6 @boonebgorges
8 years ago

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

In 36108:

Force non-public taxonomies to have a query_var of false.

[35333] implemented public=false for taxonomies. The implementation prevented
non-public taxonomies from having their archives accessed via query_var during
a normal request. But it didn't prevent non-public taxonomies from registering
their query vars in the $wp_taxonomies global. The latter implementation
details causes problems specifically when a taxonomy is registered with
query_var=true; for public taxonomies, register_taxonomy() translates this
into a query_var equivalent to the taxonomy name, but in the case of non-public
taxonomies, the query_var was set to the boolean itself. The boolean then
causes problems when using non-strict comparison to filter taxonomy objects by
query_var, as when using get_taxonomies().

This changeset addresses the issue by forcing the query_var property of
non-public taxonomies to false.

Fixes #35089.

#7 @boonebgorges
8 years ago

  • Keywords fixed-major added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.4.1.

#8 @boonebgorges
8 years ago

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

In 36109:

Force non-public taxonomies to have a query_var of false.

[35333] implemented public=false for taxonomies. The implementation prevented
non-public taxonomies from having their archives accessed via query_var during
a normal request. But it didn't prevent non-public taxonomies from registering
their query vars in the $wp_taxonomies global. The latter implementation
details causes problems specifically when a taxonomy is registered with
query_var=true; for public taxonomies, register_taxonomy() translates this
into a query_var equivalent to the taxonomy name, but in the case of non-public
taxonomies, the query_var was set to the boolean itself. The boolean then
causes problems when using non-strict comparison to filter taxonomy objects by
query_var, as when using get_taxonomies().

This changeset addresses the issue by forcing the query_var property of
non-public taxonomies to false.

Merges [36108] to the 4.4 branch.

Fixes #35089.

Note: See TracTickets for help on using tickets.