WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 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 6 years ago.
16622.2.diff (1.8 KB) - added by SergeyBiryukov 6 years ago.
16622.3.diff (1.8 KB) - added by markjaquith 6 years ago.

Download all attachments as: .zip

Change History (23)

#1 @msafi
6 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.

#2 @ryan
6 years ago

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

@markjaquith
6 years ago

#3 @ryan
6 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.

#4 @markjaquith
6 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.

#5 @ryan
6 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

#6 follow-up: @nacin
6 years ago

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

#7 in reply to: ↑ 6 @markjaquith
6 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.

#8 @markjaquith
6 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.

#9 @scribu
6 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.

#10 @scribu
6 years ago

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

#11 @SergeyBiryukov
6 years ago

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

#12 @Otto42
6 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');

#13 @markjaquith
6 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.

@markjaquith
6 years ago

#14 follow-up: @SergeyBiryukov
6 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.

#15 in reply to: ↑ 14 @markjaquith
6 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.

#16 @ryan
6 years ago

Looks good.

#17 @markjaquith
6 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

#18 @markjaquith
6 years ago

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

#19 @markjaquith
6 years ago


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

#20 @markjaquith
6 years ago


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