WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#16622 closed defect (bug) (fixed)

[17246] Breaks category exclusion

Reported by: markjaquith Owned by:
Milestone: 3.1.1 Priority: high
Severity: critical Version: 3.1
Component: Query Keywords: has-patch dev-feedback
Focuses: Cc:

Description

[17246] broke category exclusion using this code (change the excluded cats to match something in your install).

function exclude_category($query) {
	if ( $query->is_feed ) {
		$query->set('cat', '-5,-6');
	}
	return $query;
}

add_filter('pre_get_posts', 'exclude_category');

Reported by Mike Davidson here.

Attachments (3)

16622.diff (1.8 KB) - added by markjaquith 5 years ago.
16622.2.diff (1.8 KB) - added by SergeyBiryukov 5 years ago.
16622.3.diff (1.8 KB) - added by markjaquith 5 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 @msafi5 years ago

I use pre_get_posts to only include certain categories in the feed and homepage, like:

add_filter('pre_get_posts', 'featured_posts');
function featured_posts($query)
{
        if (is_single()) return;

	if ($query->is_home) 
	{
	        $query->set('cat', '50');
		return $query;
	}
	
	if ($query->is_feed)
	{
		$query->set('cat', '50,172');
		return $query;
	}
	
	return $query;
}

But none of this code actually works after 3.1.

comment:2 @ryan5 years ago

The quick fix is to lose $parsed_tax_query and take the hit of having to call get_term_children() twice.

@markjaquith5 years ago

comment:3 @ryan5 years ago

There is still a possible problem with that though. Setting 'cat' adds to the category inclusions and exclusions, rather than replacing them. Trying to override a 'cat' query won't work as before. I don't think this is a common usage though.

comment:4 @markjaquith5 years ago

  • Keywords has-patch dev-feedback added

Take a look at that patch. I suggested this before. Hashes the state of the query args, and compares it to the state of the current state, to determine whether something has changed.

comment:5 @ryan5 years ago

pre_get_posts is an unreliable place to do set(). You are setting a query var after the query vars are parsed. It happens to work for a few things in 3.0, but it isn't uniformly reliable. It also don't set the query flags to match the query. But, I guess these things have become features. :-)

Anyhow, the patch seems a good approach. The query vars array is usually pretty small so I don't think the serialize will be a big hit. Going forward, we might want to

comment:6 follow-up: @nacin5 years ago

md5() probably isn't necessary, unless I'm missing something.

comment:7 in reply to: ↑ 6 @markjaquith5 years ago

Replying to ryan:

pre_get_posts is an unreliable place to do set(). You are setting a query var after the query vars are parsed.

The same would be true of parse_query too, then. They're identical hooks. And they're fairly useful in my experience.

Replying to nacin:

md5() probably isn't necessary, unless I'm missing something.

Just trying to not balloon the size of that object. Tradeoff between memory/CPU.

comment:8 @markjaquith5 years ago

Also, for secondary requests, what other hook is available to go in and do ->set()s? It seems like parse_query/pre_get_posts are the only option.

comment:9 @scribu5 years ago

Hashing the query vars seems like a good solution. Might end up with a few false positives sometimes, but it's better to err on the safe side.

md5() is written for hashing large amounts of data, so that shouldn't be a problem.

comment:10 @scribu5 years ago

Oh, since you're hashing all the query vars, you should rename 'parsed_tax_query_hash' to 'query_vars_hash'.

@SergeyBiryukov5 years ago

comment:11 @SergeyBiryukov5 years ago

Updated the patch to reduce the number of serialize() calls.

comment:12 @Otto425 years ago

Workaround that should work for those who need it:

function exclude_category($query) {
	if ( $query->is_feed ) {
		$query->set('category__not_in', array(5,6));
	}
	return $query;
}

add_filter('pre_get_posts', 'exclude_category');

comment:13 @markjaquith5 years ago

Sergey — In that case you're serializing the array before changes are potentially made to it, so when you stash it at the end, it could already be out of date.

@markjaquith5 years ago

comment:14 follow-up: @SergeyBiryukov5 years ago

Is there a way for $this->query_vars to be modified between those lines? I don't see one, but I may have missed something. I also checked the patch with both of pre_get_posts filters above.

comment:15 in reply to: ↑ 14 @markjaquith5 years ago

Replying to SergeyBiryukov:

Is there a way for $this->query_vars to be modified between those lines? I don't see one, but I may have missed something. I also checked the patch with both of pre_get_posts filters above.

Yes. Because $q is a reference to $this->query_vars.

comment:16 @ryan4 years ago

Looks good.

comment:17 @markjaquith4 years ago

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

(In [17489]) Reparse the taxonomy query if query vars change. fixes #16622 for trunk

comment:18 @markjaquith4 years ago

(In [17490]) Reparse the taxonomy query if query vars change. fixes #16622 for 3.1

comment:19 @markjaquith4 years ago


Last edited 4 years ago by markjaquith (previous) (diff)

comment:20 @markjaquith4 years ago


Last edited 4 years ago by markjaquith (previous) (diff)
Note: See TracTickets for help on using tickets.