WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

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

Download all attachments as: .zip

Change History (23)

comment:1 msafi3 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 ryan3 years ago

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

markjaquith3 years ago

comment:3 ryan3 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 markjaquith3 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 ryan3 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: nacin3 years ago

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

comment:7 in reply to: ↑ 6 markjaquith3 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 markjaquith3 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 scribu3 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 scribu3 years ago

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

SergeyBiryukov3 years ago

comment:11 SergeyBiryukov3 years ago

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

comment:12 Otto423 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 markjaquith3 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.

markjaquith3 years ago

comment:14 follow-up: SergeyBiryukov3 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 markjaquith3 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 ryan3 years ago

Looks good.

comment:17 markjaquith3 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 markjaquith3 years ago

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

comment:19 markjaquith3 years ago


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

comment:20 markjaquith3 years ago


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