Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#16622 closed defect (bug) (fixed)

[17246] Breaks category exclusion

Reported by: markjaquith Owned by:
Priority: high Milestone: 3.1.1
Component: Query Version: 3.1
Severity: critical Keywords: has-patch dev-feedback
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 2 years ago.
16622.2.diff (1.8 KB) - added by SergeyBiryukov 2 years ago.
16622.3.diff (1.8 KB) - added by markjaquith 2 years ago.

Download all attachments as: .zip

Change History (23)

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   ryan2 years ago

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

comment:3   ryan2 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.

  • 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   ryan2 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: ↓ 7   nacin2 years ago

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

comment:7 in reply to: ↑ 6   markjaquith2 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.

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.

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.

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

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

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');

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.

comment:14 follow-up: ↓ 15   SergeyBiryukov2 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   markjaquith2 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.

Looks good.

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

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

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

(In [17512]) Prevent double index.php preprend on PATHINFO custom taxonomy permalinks. Proper use of with_front. props greuben. fixes #16918. fixes #16622. see #15813. see #12659. For trunk

Version 0, edited 2 years ago by markjaquith (next)


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