Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#21290 closed defect (bug) (fixed)

Post types continue to be selected after removal from custom taxonomy registration

Reported by: jondavidjohn's profile jondavidjohn Owned by: jondavidjohn's profile jondavidjohn
Milestone: 3.5 Priority: normal
Severity: major Version: 3.4.1
Component: Taxonomy Keywords: close
Focuses: Cc:

Description

Bug

Post's appear in the archive view for custom taxonomies that have post types that are not registered for the given custom taxonomy.

This happens when a post type is removed from the taxonomy registration that currently has posts with said post type with associated taxonomies applied. It doesn't seem like it should be assumed that taxonomy registration will always be handled in a static programmatic way that will never change.

Reproduce

  1. Create a custom taxonomy registration for 'post' and 'page' post types
  2. Add a few posts and pages to one of these custom taxonomies
  3. Check and make sure these posts/pages are showing up on the custom taxonomy archive page
  4. Now remove 'page' from the taxonomy registration
  5. Confirm that it is removed in the admin menus and page edit screen.
  6. Go back to the taxonomy archive page and you will see the pages you added still show up in the loop, now unable to manually remove the taxonomy term in the admin.

Solution

Currently the query for a custom taxonomy defaults to 'any' post type with the taxonomy applied, if a post type has not been filtered in at this point. (query.php:2217)

My proposed solution would be to recognize the taxonomy applied and use the post type registration to make the default post type selection, instead of assuming 'any'.

This will prevent the situation of orphaned posts still showing up in custom taxonomy archives even after their post type is removed from the registration.

Attachments (8)

query.diff (657 bytes) - added by jondavidjohn 12 years ago.
21290-2.diff (775 bytes) - added by jondavidjohn 12 years ago.
21290-3.diff (801 bytes) - added by jondavidjohn 12 years ago.
21290-4.diff (895 bytes) - added by jondavidjohn 12 years ago.
21290-5.diff (932 bytes) - added by jondavidjohn 12 years ago.
21290-6.diff (940 bytes) - added by SergeyBiryukov 12 years ago.
Same as 21290-5.diff, with proper formatting
21290-7.diff (804 bytes) - added by jondavidjohn 11 years ago.
Correct Attachment post type taxonomies.
tax-check.diff (1.3 KB) - added by wonderboymusic 11 years ago.

Download all attachments as: .zip

Change History (38)

@jondavidjohn
12 years ago

#1 @ejdanderson
12 years ago

  • Cc ejdanderson@… added

#2 @scribu
12 years ago

  • Keywords needs-testing added

Hey jondavidjohn.

The fact that taxonomy archives default to post_type=any has been debated in the past.

Since you're only proposing to restrict them to the post types that the taxonomy is registered for, I think it's more acceptable.

That said, I'm pretty sure there still are some edge cases that would negatively be affected by this change.

#3 @nacin
12 years ago

I can confirm the behavior here, as long as the post type remains registered, as 'any' gets rewritten to all registered post types. I'm with scribu at the moment — I sense there exist some edge cases here.

#4 @jondavidjohn
12 years ago

Replying to nacin:

I can confirm the behavior here, as long as the post type remains registered, as 'any' gets rewritten to all registered post types. I'm with scribu at the moment — I sense there exist some edge cases here.

Any cues on what these edge cases may be?

Last edited 12 years ago by jondavidjohn (previous) (diff)

#5 follow-up: @scribu
12 years ago

I mean various plugins/themes that depend on the current behaviour.

#6 in reply to: ↑ 5 ; follow-up: @jondavidjohn
12 years ago

Replying to scribu:

I mean various plugins/themes that depend on the current behaviour.

The problem is that the behavior doesn't make sense... post types showing up in the loop for taxonomies that don't have them registered?

I would say that it's far more likely that there are more instances of people being forced to create nasty workarounds, manually resetting the post_type of the main query based on a handful of loosely related conditions.

Can you even think of a theoretical scenario that someone would want a 'page', for instance, to show up in the loop for a taxonomy that is only registered for 'post'?

#7 in reply to: ↑ 6 ; follow-up: @scribu
12 years ago

A post (or page) will show up in The Loop only if it has the queried term assigned to it. If you don't want it to show up anymore, just un-assign the term.

#8 in reply to: ↑ 7 @jondavidjohn
12 years ago

Replying to scribu:

A post (or page) will show up in The Loop only if it has the queried term assigned to it. If you don't want it to show up anymore, just un-assign the term.

That's the point. You can't. Once the post_type is unregistered for the taxonomy, the UI to remove the term disappears from the admin.

#9 @scribu
12 years ago

  • Milestone changed from Awaiting Review to 3.5

Ok, I'm convinced. Let's do this.

#10 @jondavidjohn
12 years ago

  • Keywords needs-testing removed

#11 @jondavidjohn
12 years ago

Added another patch that accounts for multiple taxonomy queries. Doing a fully inclusive search for all post types registered for all queried taxonomies.

The only remaining edge case here is that taxonomy A registered for post type T queried along side taxonomy B registered for post type U, will potentially still return results of post type T once registered to taxonomy B.

Version 0, edited 12 years ago by jondavidjohn (next)

#12 @jondavidjohn
12 years ago

Latest patch accounts for taxonomies that specify a custom query_var or have no query_var.

Also doesn't loop through every taxonomy, only the taxonomies being queried for.

@SergeyBiryukov
12 years ago

Same as 21290-5.diff, with proper formatting

#13 @nacin
12 years ago

  • Keywords commit added

:-)

#14 @nacin
12 years ago

21290-5.diff still has some issues — $post_type is used as a variable for the foreach(), potentially overwriting 'any' if nothing ends up being found. I'm reworking the branching slightly.

#15 @nacin
12 years ago

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

In [21855]:

When doing a taxonomy query, search against the currently registered post types of the queried taxonomies.

Prevents posts of a type no longer assigned to a queried taxonomy from being returned.

props jondavidjohn. fixes #21290.

#16 @nacin
11 years ago

  • Keywords revert added; has-patch commit removed
  • Priority changed from normal to high
  • Resolution fixed deleted
  • Severity changed from normal to blocker
  • Status changed from closed to reopened

This broke one of our favorite things in the world: attachment taxonomies.

An attachment taxonomy can be register not just to 'attachment', but to a particular mime type, like attachment:image or attachment:video.

get_object_taxonomies() handles this, but only if it is passed a post object. At this stage in querying, we're obviously too early for that. So, when we come across the 'attachment' post type and do get_object_taxonomies( 'attachment' ), we don't get back any of the mime-type-specific taxonomies, and that causes us to avoid including 'attachment' in the query.

There *are* some potential fixes for this. Basically, we need an API that says "get me all attachment taxonomies", essentially looking for any object type that is either 'attachment' or starts with 'attachment:'.

I'm at the point where we revert this for 3.5 and revisit it later.

#17 @nacin
11 years ago

ocean90 reminds me we added get_taxonomies_for_attachments() in 3.5.

So, if $pt == 'attachment', use get_taxonomies_for_attachments(), otherwise use get_object_taxonomies( $pt ). Seems salvageable foe 3.5.

We should probably introduce get_taxonomies_for_post_type() (or _object_type()) at some point later, because having get_object_taxonomies() take either a post object or a post type name is confusing (and only for it to not work well for the 'attachment' post type). Then, get_object_taxonomies() can primarily accept post objects, while get_taxonomies_for_post_type() can accept post types. This gets weird though, because it isn't obvious whether 'attachment' should auto-expand the way _for_attachments() does.

@jondavidjohn
11 years ago

Correct Attachment post type taxonomies.

#18 @nacin
11 years ago

In 22718:

Account for taxonomies tied to specific kinds of attachments when setting up post types for a taxonomy query.

props jondavidjohn. see #21290.

#19 @nacin
11 years ago

  • Keywords close added; revert removed
  • Priority changed from high to normal
  • Severity changed from blocker to major

Should be good.

#20 @nacin
11 years ago

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

#21 @wonderboymusic
11 years ago

I am not in love with the side effects of this. If you have a taxonomy in mutiple blogs, there can be different post types assigned to the taxonomy in each blog.

Site # 1

add_action( 'init', function () {
	register_post_type( 'pizza', array( 'public' => true ) );
	register_post_type( 'ice_cream', array( 'public' => true ) );
	register_taxonomy( 'flavor', array( 'pizza', 'ice_cream' ) );
} );

add_action('pre_get_posts', function ( &$query ) {

	if ( $query->is_main_query() ) {
		$query->set( 'post_type', array( 'pizza', 'ice_cream' ) );
	}

});

Site # 2

register_taxonomy( 'flavor', 'post' );

Default WP_Query with post_type set to ice_cream and pizza in pre_get_posts:

SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  
WHERE 1=1  AND wp_posts.post_type IN ('ice_cream', 'pizza') 
AND (wp_posts.post_status = 'publish')  
ORDER BY wp_posts.post_date DESC LIMIT 0, 10

Now let's switch_to_blog and do a tax_query:

switch_to_blog(2);

$query = new WP_Query( array(
	'tax_query' => array(
	array(
		'taxonomy' => 'flavor',
		'include_children' => false,
		'field' => 'term_taxonomy_id',
		'operator' => 'IN',
		'terms' => array( 1 )
	)
) ) );

Previously this would look for posts, but not anymore:

SELECT SQL_CALC_FOUND_ROWS  wp_2_posts.ID 
FROM wp_2_posts  INNER JOIN wp_2_term_relationships 
ON (wp_2_posts.ID = wp_2_term_relationships.object_id) 
WHERE 1=1  
AND ( wp_2_term_relationships.term_taxonomy_id IN (1) ) 
AND wp_2_posts.post_type IN ('pizza', 'ice_cream') 
AND (wp_2_posts.post_status = 'publish') 
GROUP BY wp_2_posts.ID ORDER BY wp_2_posts.post_date

I personally view tax_query as a filter on defaults. post should be the assumption for post_type.

#22 @jondavidjohn
11 years ago

This use case confuses me...

Why are you filtering pre_get_posts in your example?

What's keeping this pre_get_posts from running on site 2 as well as site 1? ( which initially seems why you're getting those results )

How are you register taxonomies differently per blog?

#23 @wonderboymusic
11 years ago

I am showing posts from blog 3 on the homepage of blog 1. Both blogs have the region taxonomy registered. Blog 1 has tons of custom post types, blog 3 only has post. When 3.5 dropped, I had to find every instance of WP_Query that didn't have post_type passed and add it, otherwise it would try to load the custom post types from blog 1 when I called switch_to_blog(3) and then WP_Query inline without post_type set. I wrote about my use case a few weeks ago: http://scotty-t.com/2012/12/03/wordpress-regionalization/

#24 @jondavidjohn
11 years ago

Ok, interesting, but it seems like the problem is you were depending on tax queries querying all post_types ( which was the previous behavior ), whether or not they are currently registered for the given taxonomy.

switch_to_blog() does not switch the entire environment see #14941 , which means that even though you switch_to_blog(2) you are still running on the taxonomy registrations for blog 1. Rationally so you will only get results of post_types that are registered to that taxonomy.

#25 @wonderboymusic
11 years ago

The problem is that it is easy to assume that WP_Query uses defaults in all cases except maybe search, which is no longer true. I can see a benefit to having a ! ms_is_switched() somewhere in there

#26 @jondavidjohn
11 years ago

This patch changes those defaults, of a taxonomy query to the post_types that taxonomy is registered for, instead of blanketly querying the taxonomy for all post_types. I think the behavior you're describing as a problem is consistent and rationally sound in the way that tax queries work now. I think adding a special case for this specific scenario in core would only create confusion.

Last edited 11 years ago by jondavidjohn (previous) (diff)

#27 @wonderboymusic
11 years ago

we typically describe way smaller issues as regressions, since most people wouldn't even be able to debug the issue

#28 @wonderboymusic
11 years ago

I think maybe the reverse should have happened here - instead of looking at all post types and plucking out the public ones which are registered for the taxonomy, the passed post_types should be filtered by only those that are *still* registered with that taxonomy. It should safe to assume that post_type is going to default to 'post' in most cases, which is what are broadcasting to the universe here: ​http://codex.wordpress.org/Class_Reference/WP_Query#Type_Parameters

#29 @jondavidjohn
11 years ago

Not sure what you mean by *still* registered... again switch_to_blog does not change the environment, only the database the queries point at.

When you are querying a taxonomy you are changing the defaults. If you do a tax query for a taxonomy registered only to a custom post_type, you expect to get back posts instead of the type it's registered to? That would be (was) confusing. Maybe documentation needs to be updated, but this is decidedly a more logical and expected behavior.

Also, this patch does not stomp explicitly given post_type's in the WP_Query, if you provide 'post' as a post type to the query, it will use that.

It might be clearer to communicate your proposed solution via a patch.

#30 @wonderboymusic
11 years ago

I have added a patch which I think does the correct thing. Irony being, it doesn't solve my problem. But if it's going to use post when no post types are passed (which is how it used to work), my patch correctly identifies post as not being registered with region and returns nothing when I make the WP_Query after calling switch_to_blog(). That solves the original intent of the ticket - discarding post_types that are no longer registered with the taxonomy. Adding more post_types into the fray, IMO, breaks expected WP_Query default behavior.

I'm not going to reopen the ticket, but curious if anyone else is affected by this.

Note: See TracTickets for help on using tickets.