#21290 closed defect (bug) (fixed)
Post types continue to be selected after removal from custom taxonomy registration
Reported by: | jondavidjohn | Owned by: | 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
- Create a custom taxonomy registration for 'post' and 'page' post types
- Add a few posts and pages to one of these custom taxonomies
- Check and make sure these posts/pages are showing up on the custom taxonomy archive page
- Now remove 'page' from the taxonomy registration
- Confirm that it is removed in the admin menus and page edit screen.
- 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)
Change History (38)
#3
@
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
@
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?
#5
follow-up:
↓ 6
@
12 years ago
I mean various plugins/themes that depend on the current behaviour.
#6
in reply to:
↑ 5
;
follow-up:
↓ 7
@
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:
↓ 8
@
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
@
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.
#11
@
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.
#12
@
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.
#14
@
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.
#16
@
12 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
@
12 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.
#19
@
12 years ago
- Keywords close added; revert removed
- Priority changed from high to normal
- Severity changed from blocker to major
Should be good.
#21
@
12 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
@
12 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
@
12 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
@
12 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
@
12 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
@
12 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.
#27
@
12 years ago
we typically describe way smaller issues as regressions, since most people wouldn't even be able to debug the issue
#28
@
12 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
@
12 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
@
12 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.
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.