Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32454 closed defect (bug) (fixed)

WP_Query->parse_tax_query() can't handle arrays from URL querystring

Reported by: veraxus's profile Veraxus Owned by: johnbillion's profile johnbillion
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.1
Component: Query Keywords: has-patch needs-testing
Focuses: Cc:

Description

When using querystring variables to filter taxonomies (default WordPress behavior) WP_Query->parse_tax_query() assumes it's getting a string from the URL (?tax=1,2,3,4).

However, if using HTML form-based filtering, such as a checkbox list, an array is passed instead. When this happens, not only does the search/filter not work, a slew of warnings are generated since WP_Query->parse_tax_query() is only capable of handling strings.

Fortunately, this is easily fixed with just three lines. If the array is detected and imploded before being handled, everything works as expected/designed without generating any errors or warnings.

Patch file is attached.

Attachments (3)

parse_tax_query.patch (425 bytes) - added by Veraxus 9 years ago.
parse_tax_query() correctly detects & implodes arrays
parse_tax_query_unit_tests.patch (2.3 KB) - added by Veraxus 9 years ago.
Unit tests for WP_Query->parse_tax_query() patch.
32454-ut.patch (1.0 KB) - added by ocean90 9 years ago.

Download all attachments as: .zip

Change History (16)

@Veraxus
9 years ago

parse_tax_query() correctly detects & implodes arrays

#1 @johnbillion
9 years ago

  • Keywords has-patch needs-testing needs-unit-tests added
  • Version changed from trunk to 3.1

Thanks for the patch Veraxus.

This will need unit tests to show that it fixes the issue at hand and that it doesn't have any unintended side effects.

@Veraxus
9 years ago

Unit tests for WP_Query->parse_tax_query() patch.

#2 @Veraxus
9 years ago

I've included unit tests for single term and multi-term use cases.

Pre-patch:

  • Single term unit test passes
  • Multi-term unit test fails (error)

With patch:

  • Single term unit test passes
  • Multi-term unit test passes

#3 @Veraxus
9 years ago

  • Keywords needs-unit-tests removed

Is anything else needed for this? It would be ideal if we got this bug squished before 4.3

#4 @johnbillion
9 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Owner set to johnbillion
  • Status changed from new to reviewing

#5 @obenland
9 years ago

@johnbillion looks like this only needs a final review and commit?

#6 @boonebgorges
9 years ago

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

In 33095:

In WP_Query::parse_tax_query(), allow taxonomy querystring to be formatted as an array.

Props Veraxus.
Fixes #32454.

#7 @ocean90
9 years ago

@boonebgorges Looks like the PHPUnit tests doesn't pass in PHP > 5.2, see https://travis-ci.org/aaronjorbin/develop.wordpress/builds/69794056.

#8 @Veraxus
9 years ago

Taking a look... although at upload time I ran the tests successfully on 5.2.16 and 5.6.9

@ocean90
9 years ago

#9 follow-up: @ocean90
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think replacing assertEquals() with assertEqualSets() should fix this, see 32454-ut.patch.

#10 in reply to: ↑ 9 @Veraxus
9 years ago

Replying to ocean90:

I think replacing assertEquals() with assertEqualSets() should fix this, see 32454-ut.patch.

I'm not able to reproduce the errors on 5.3, 5.4, 5.5, or 5.6 with the latest trunk - and I manually checked the values being passed to assertEquals() as well.

Generally speaking, assertEqualSets() shouldn't be necessary… unless wp_list_pluck() is returning it's array out of order for some reason. It's a safe alternative in this case, at least.

Last edited 9 years ago by Veraxus (previous) (diff)

#11 @boonebgorges
9 years ago

Thanks, @ocean90. assertEqualSets() is correct here. The issue is that local instances of phpunit often run more quickly than those on Travis VMs, with the result that race conditions appear on Travis that aren't possible to reproduce locally.

#12 @boonebgorges
9 years ago

In 33102:

Use assertEqualSets() in WP_Query::parse_tax_query() tests.

Props ocean90.
See #32454.

#13 @ocean90
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.