WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 6 months ago

Last modified 4 months ago

#20767 closed defect (bug) (fixed)

tax_query clobbers tag and category

Reported by: wonderboymusic Owned by: wonderboymusic
Milestone: 3.8 Priority: normal
Severity: major Version: 3.1
Component: Query Keywords: has-patch commit
Focuses: Cc:

Description

The main query should return a tag or category as its queried object if tax_query is applied to a tag or category archive page. It currently returns the first term derived from the WP_Query->tax_query->queries array which does not include the tag or category as a term in the request. WP_Query->tax_query->queries is a portion of the request, but not the request. Because WP_Query sets is_tag and is_category flags, they have to be captured before dealing with is_tax

The result of this bug is that theme functions return the wrong data because the wrong term is returned with get_queried_object.

Real world example: at eMusic, we pass regions into pre_get_posts to regionalize the content that is returned. On a tag page, if you are in the US, we pass the regions "ALL" and "US" to make sure you are looking at appropriate content. Here's what the title for the archives looks like - (spoiler: It's "ALL" when it should be "Daily Download"):

http://www.emusic.com/17dots/topics/daily-download/

I am attaching a patch.

Attachments (4)

tag-cat-queried-obj.diff (1.1 KB) - added by wonderboymusic 23 months ago.
Patch for WP_Query->get_queried_object
20767.diff (1.0 KB) - added by wonderboymusic 15 months ago.
20767.2.diff (3.6 KB) - added by wonderboymusic 7 months ago.
20767.3.diff (6.2 KB) - added by wonderboymusic 6 months ago.

Download all attachments as: .zip

Change History (20)

wonderboymusic23 months ago

Patch for WP_Query->get_queried_object

comment:1 follow-up: scribu23 months ago

I'm not sure using $this->get( 'cat' ) is much better: you can have ?cat=1,2,-3.

comment:2 nacin23 months ago

  • Version changed from 3.4 to 3.1

comment:3 in reply to: ↑ 1 wonderboymusic22 months ago

Replying to scribu:

I'm not sure using $this->get( 'cat' ) is much better: you can have ?cat=1,2,-3.

It works, if you have a URL like so: www.emusic.com/17dots/topics/daily-download/?cat=1,2,3,4

  • is_tag is true
  • is_category is true
  • cat returns 1 (the first category id)
  • tag_id returns 8376, which is correct

The public query is correct:

array(2) {
    'cat' =>
    string(7) "1,2,3,4"
    'tag' =>
    string(14) "daily-download"
}

is_tag takes precedence of is_category and should

Version 0, edited 22 months ago by wonderboymusic (next)

comment:4 wonderboymusic21 months ago

can I get a 3.5 amen from the congregation? tax_query has been breaking theme functions for a while now

comment:5 markoheijnen21 months ago

I really don't see the issue. If you handle category or post_tag as a custom taxonomy you wouldn't have the issue right?
I rather go for something like that then adding code for only category or post_tag

wonderboymusic15 months ago

comment:6 wonderboymusic15 months ago

  • Milestone changed from Awaiting Review to 3.6

The fact that we are already doing this:

if ( $this->is_category || $this->is_tag || $this->is_tax ) {
    ....
}

...means we need to check them before arbitrarily grabbing the first term in the stack

comment:7 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

comment:8 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

Patch still applies, this was a really annoying issue I had to work around in multiple places

wonderboymusic7 months ago

comment:9 wonderboymusic7 months ago

  • Keywords commit added

Added unit tests to cover tag / category pages with a tax_query injection - tag / category should be the queried object. Related: #5358.

comment:10 nacin7 months ago

  • Milestone changed from 3.7 to 3.8

Unfortunately it's too late for this, but let's do it in early 3.8.

wonderboymusic6 months ago

comment:11 wonderboymusic6 months ago

Added a new patch, gonna let this stew for a lil bit - I think WP_UnitTestCase::go_to() is doing something weird. Some of my assertions that I was trying were returning is_home() === true for a custom taxonomy, which means some other unit tests might be returning false positives. Will inwestigate.

comment:12 wonderboymusic6 months ago

In 26006:

WP_UnitTestCase::go_to() tried its best to clean up global space, but ultimately fell short. Because it was blowing away WP every time it was called, it was dropping all the query vars that were registered for custom taxonomies and custom post types (ouch).

Introduces _cleanup_query_vars(). This is a prerequisite for the unit tests on #20767. All unit tests pass with this change.

See #20767.
Fixes #25818.

comment:13 wonderboymusic6 months ago

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

In 26007:

Category and tag are typically checked before checking for a custom taxonomy. If the global query matches category or tag (even if it also has tax_query set), return category/tag as the queried object, instead of arbitrarily returning the first term in the tax_query stack (typically those added with 'pre_get_posts').

Real world example: http://www.emusic.com/17dots/topics/daily-download/ - "tag" page, regionalized for US-only content using pre_get_posts passing in the terms "US" and "ALL" for "region" (custom tax). All of the theme functions would output "ALL" as the term name. Even though it was a tag archive, the queried object was an arbitrary term from tax_query.

See [26006]. All unit tests pass.
Fixes #20767.

comment:14 Otto424 months ago

This appears to have caused a regression issue. Related: #26627

comment:15 Otto424 months ago

Specifically, on a default install with pretty permalinks, when you visit /category/uncategorized, then the 'cat' variable is not set.

Instead, the category_name is 'uncategorized', and there is a tax_query with
'category' as the taxonomy and 'uncategorized' as the term. You cannot only use get('cat') here. This probably breaks tags in a similar way for the same reasons.

comment:16 lkraav4 months ago

Yep, what @Otto42 said. This is breaking at least a few of my customers sites pretty bad. Moving over to #26627.

Note: See TracTickets for help on using tickets.