Opened 13 years ago
Closed 11 years ago
#18614 closed defect (bug) (fixed)
post_type_archive_title doesn't work when tax_query is added to wp_query
Reported by: | wonderboymusic | Owned by: | wonderboymusic |
---|---|---|---|
Milestone: | 3.7 | Priority: | normal |
Severity: | normal | Version: | 3.2 |
Component: | Template | Keywords: | has-patch needs-unit-tests |
Focuses: | Cc: |
Description
post_type_archive_title( ) gets called by wp_title( ) on Custom Post Type archive pages
To get the data for the post type object, post_type_archive_title( ) calls get_queried_object( ). This is problematic. If the query has been altered in any way (such as adding a tax_query in pre_get_posts filter), get_queried_object( ) will probably return a term, not a post type object.
Hence, accessing $post_type_obj->labels will fail and their will be no title returned
//old potentially broken way $post_type_obj = get_queried_object(); // new doesn't break way $post_type_obj = get_post_type_object( get_post_type() );
Attachments (6)
Change History (47)
#1
@
13 years ago
- Milestone Awaiting Review deleted
- Resolution set to wontfix
- Status changed from new to closed
#2
@
13 years ago
post_type_archive_title( ) exits if the current context is not post_type_archive( ) - meaning post_type is set in $wp_query.
The name of the variable is $post_type_obj which leads me to believe it should always be a post type object, which get_queried_object isn't guaranteed to return. Since post_type is in wp_query, get_post_type will work and get_post_type_object( get_post_type() ) will work and return a post_type_object, so then $post_type_obj->labels won't fail and log errors
This isn't a band aid, writing a filter to correct what should be default functionality is
#3
follow-up:
↓ 5
@
13 years ago
Ok, and what happens if 'post_type' is set to an array by some other code that alters $wp_query?
#5
in reply to:
↑ 3
;
follow-up:
↓ 6
@
13 years ago
Replying to scribu:
Ok, and what happens if 'post_type' is set to an array by some other code that alters $wp_query?
funny you should ask - I did some testing that looks at exactly that - removes the tax_query and instead sets post_type (also tested with tax_query AND post_type set to something custom)
the original post_type was "emusic_icon" and I added "post" and "emusic_spotlight"
// altered function function post_type_archive_title( $prefix = '', $display = true ) { global $wp_query; var_dump( $wp_query ); var_dump( get_post_type_object( get_post_type() ) ); exit(); if ( ! is_post_type_archive() ) return; $post_type_obj = get_post_type_object( get_post_type() ); $title = apply_filters('post_type_archive_title', $post_type_obj->labels->name ); if ( $display ) echo $prefix . $title; else return $title; }
Here's the output for $wp_query with an Array of post_types:
object(WP_Query)#129 (47) { ["query_vars"]=> array(58) { ["post_type"]=> array(3) { [0]=> string(4) "post" [1]=> string(11) "emusic_icon" [2]=> string(16) "emusic_spotlight" } .....
Here's the output for get_post_type_object( get_post_type() ) - returns the ORIGINAL post_type:
object(stdClass)#354 (26) { ["labels"]=> object(stdClass)#356 (14) { ["name"]=> string(5) "Icons" ["singular_name"]=> string(4) "Icon" ["add_new"]=> string(7) "Add New" ["add_new_item"]=> string(12) "Add New Icon" ["edit_item"]=> string(9) "Edit Icon" ["new_item"]=> string(8) "New Icon" ["view_item"]=> string(9) "View Icon" ["search_items"]=> string(12) "Search Icons" ["not_found"]=> string(14) "No Icons found" ["not_found_in_trash"]=> string(23) "No Icons found in Trash" ["parent_item_colon"]=> NULL ["all_items"]=> string(5) "Icons" ["menu_name"]=> string(5) "Icons" ["name_admin_bar"]=> string(4) "Icon" } ["description"]=> string(0) "" ["publicly_queryable"]=> bool(true) ["exclude_from_search"]=> bool(false) ["capability_type"]=> string(4) "post" ["map_meta_cap"]=> bool(true) ["_builtin"]=> bool(false) ["_edit_link"]=> string(16) "post.php?post=%d" ["hierarchical"]=> bool(false) ["public"]=> bool(true) ["rewrite"]=> array(4) { ["slug"]=> string(15) "music-news/icon" ["with_front"]=> bool(true) ["pages"]=> bool(true) ["feeds"]=> bool(true) } ["has_archive"]=> string(16) "music-news/icons" ["query_var"]=> string(11) "emusic_icon" ["register_meta_box_cb"]=> object(Closure)#353 (1) { ["static"]=> array(2) { ["type"]=> string(11) "emusic_icon" ["callback"]=> object(Closure)#352 (1) { ["parameter"]=> array(1) { ["$type"]=> string(10) "<required>" } } } } ["taxonomies"]=> array(5) { [0]=> string(6) "region" [1]=> string(5) "genre" [2]=> string(8) "location" [3]=> string(8) "post_tag" [4]=> string(8) "category" } ["show_ui"]=> bool(true) ["menu_position"]=> NULL ["menu_icon"]=> NULL ["permalink_epmask"]=> int(1) ["can_export"]=> bool(true) ["show_in_nav_menus"]=> bool(true) ["show_in_menu"]=> bool(false) ["show_in_admin_bar"]=> bool(false) ["name"]=> string(11) "emusic_icon" ["cap"]=> object(stdClass)#355 (14) { ["edit_post"]=> string(9) "edit_post" ["read_post"]=> string(9) "read_post" ["delete_post"]=> string(11) "delete_post" ["edit_posts"]=> string(10) "edit_posts" ["edit_others_posts"]=> string(17) "edit_others_posts" ["publish_posts"]=> string(13) "publish_posts" ["read_private_posts"]=> string(18) "read_private_posts" ["read"]=> string(4) "read" ["delete_posts"]=> string(12) "delete_posts" ["delete_private_posts"]=> string(20) "delete_private_posts" ["delete_published_posts"]=> string(22) "delete_published_posts" ["delete_others_posts"]=> string(19) "delete_others_posts" ["edit_private_posts"]=> string(18) "edit_private_posts" ["edit_published_posts"]=> string(20) "edit_published_posts" } ["label"]=> string(5) "Icons" }
#6
in reply to:
↑ 5
@
13 years ago
I think the above behavior is correct - not to mention that it returns a post_type label name, instead of whatever taxonomy label name for whatever term that get_queried_object returns
#7
@
13 years ago
Yes, but do you know why it works?
Because get_post_type() checks the $post global:
http://core.trac.wordpress.org/browser/trunk/wp-includes/post.php#L786
In other words, it's is to be used only within The Loop.
#8
@
13 years ago
ok - then this is still better than returning a term and trying to access a property that doesn't exist:
$type = get_query_var( 'post_type' ) ; $post_type_obj = get_post_type_object( is_array( $type ) ? $type[0] : $type );
#9
@
13 years ago
- Keywords 2nd-opinion added
- Milestone set to Awaiting Review
- Resolution wontfix deleted
- Status changed from closed to reopened
Fair enough. Will let someone else weigh in.
#10
@
13 years ago
Just to chime in here, This is something I've run into a few times - not this specifically, but relying upon get_queried_object() to return the same data as expected. If it's an "advanced Query" (ie. Taxonomy(s) or Authors set - any type of archive I believe) then the return may not be exactly what the calling code is expecting.
#11
@
13 years ago
It's worth noting that this is reproducable using public query vars (eg. example.com/my_post_type/?my_taxonomy=my_term
) as reported in #19035.
#12
@
13 years ago
- Version changed from 3.3 to 3.2
So is_post_type_archive() returns true but the queried object is wrong? I'm fine with the patch, but this seems like it's a larger issue.
#13
@
13 years ago
From #19035:
Example: When viewing
example.com/my_post_type/?my_taxonomy=my_term
, the return value ofget_queried_object()
is the term object, not the post type object. This causes a PHP notice whenpost_type_archive_title()
attempts to access the label for the post type object, and results in a blank title.
#14
@
13 years ago
- Cc bill.erickson@… added
+1 for the patch.
I just ran into this issue on taxonomy archives limited to a specific post type ( example.com/taxonomy/term/?post_type=my_post_type ).
I'm using the filter to rebuild the title and just ignoring the PHP notice for now.
#15
@
13 years ago
18614.diff is the first pass at different route to solve the larger problem here, which includes #19939 as well as this ticket.
18614.2.diff is a second take, which, primarily, does not re-order anything in get_queried_object(). What both patches do make sure of, though, is that is_post_type_archive() is always requested prior to taxonomy data.
This was a bit inconsistent in the original implementation. wp_title() is going to be correct since it will always allow a post type archive to override a taxonomy. Given the hierarchy inherent to posts and terms, I would think that a post type archive should nearly always take precedence. While it is nearly impossible to tell the difference between /tag/some-tag/?post_type=type and /type/?tag=some-tag (and certainly so when rewrite rules are off), I think a good assumption would be that if the post type is has_archive = true, it is going to be the main target of a drill-down, rather than an option of a drill-down. (Or, at least, it is a higher priority than a tag or category in the subject of a drill-down, generally speaking.)
I thought about get_queried_object( $type_of_object ), get_queried_objects(), and a few other things here. I think greater changes should wait for another day. For now, the attached patch attempts to bring post types above taxonomies when has_archive is true, and solve the current issue of template tags malfunctioning.
#16
@
13 years ago
- Keywords 2nd-opinion removed
- Milestone changed from Awaiting Review to 3.4
I haven't tested either of the patches, but I agree with the reasoning:
If we start with the premise that post types should never be mixed between them, then the post type should indeed take precedence over taxonomies, which can accept multiple terms.
#21
follow-up:
↓ 22
@
12 years ago
- Cc styledev added
It seems as if a part of 18614.2.diff was included in Wordpress 3.5 in general-template.php lines 1666 to 1669 in function feed_links_extra( $args = array() ):
} elseif ( is_post_type_archive() ) { $title = sprintf( $args['posttypetitle'], get_bloginfo('name'), $args['separator'], post_type_archive_title( '', false ) ); $href = get_post_type_archive_feed_link( get_queried_object()->name ); }
Which now is causing the following notices:
Notice: Undefined property: stdClass::$labels in /Users/dbushaw/Sites/trainelite/wp-includes/general-template.php on line 658
Notice: Trying to get property of non-object in /Users/dbushaw/Sites/trainelite/wp-includes/general-template.php on line 658
Note: If you apply the latest patch (18614.2.diff) then everything is fixed.
#23
@
12 years ago
- Cc benjmay added
I was getting the same notices as @styledev, however, it wasn't from wp_title as I was being clever and only using wp_title on non-CPT pages.
The error was coming from the 'extra' feeds in wp_header.
I was able to prevent the error without making mods to core by simply adding:
remove_action( 'wp_head', 'feed_links_extra', 3 );
#24
@
12 years ago
Ben, I guess the notices weren't the same and for your notices there is a patch for that in ticket #21394
#25
@
12 years ago
- Keywords needs-refresh added
- Milestone changed from Future Release to 3.6
While it is nearly impossible to tell the difference between /tag/some-tag/?post_type=type and /type/?tag=some-tag (and certainly so when rewrite rules are off)
WP::$matched_query
is always correct when it comes to precedence when rewrite rules are on.
When the URL is wordpress-core/tag/woo/?post_type=sport
:
["matched_query"]=> string(7) "tag=woo"
When the URL is wordpress-core/sport/?tag=woo
:
["matched_query"]=> string(15) "post_type=sport"
Relying on the WP
property will also prevent a tax_query
from stomping the matched_query
in get_queried_object
#26
@
12 years ago
Since this is taking a while to get fixed here is a function you can put in your functions.php file to turn of the is_tax on these types of queries. https://gist.github.com/styledev/5278527
#30
@
11 years ago
- Keywords needs-unit-tests added; needs-refresh removed
- Milestone changed from Future Release to 3.7
I forgot I even filed this ticket - @nacin patched this properly a long time ago, it was just never committed. I have refreshed against trunk, the scenario described in the ticket is still valid.
#31
@
11 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from reopened to closed
In 25291:
#32
@
11 years ago
get_post_type_object( get_query_var( 'post_type' ) );
— could that fail if the post_type QV is an array? That's the only thing that jumps out.
- Could we get away with demoting the is_post_type_archive() and is_tax() conditionals in template-loader.php to below is_front_page() and is_home()? Then is_home() will happen right before is_post_type_archive(), possibly setting the stage for that to be cleaned up in the future, as it is essentially the 'post' post type archive (plus some other special sauce). It would also make a lot more sense as is_tax() was kind of nonchalantly stuffed up at the top originally. Does anyone see any side effects?
#33
@
11 years ago
- This check needs to happen in
get_post_type_object()
. If an array is passed, in needs to do$post_type = reset( $post_type );
. There are 8-10 places in core where the query var is passed.
- As long as
is_tax()
is false when thepost_type
obj is the queried object, I think it is safe.
#35
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Sorry to be a pain, but I feel like the reset() in [25292] might not be the best place for this. Or at the very least, it needs docs.
If you are passing an array to get_post_type_object(), you *may* be doing it wrong. For us to choose the first one (essentially picking favorites) could mean we are suppressing issues. While it might make sense in some places to choose the first one, it might not make sense in all places. This could potentially paper over a developer error. (Including, yes, an error where we have done this 8-10 times ourselves.)
I was only musing for the template-loader.php change. Let's dig into that a bit more to make sure it won't break anything. Also, [25292] should have been two commits.
#36
@
11 years ago
yeah, I was a shade overeager - I'll inspect those places in core and see if the checks make more sense inline
#38
@
11 years ago
And regarding [25312], even better, some of these aren't even needed inline, because is_post_type_archive is only true when post_type
is *not* an array:
if ( !empty( $qv['post_type'] ) && ! is_array( $qv['post_type'] ) ) { $post_type_obj = get_post_type_object( $qv['post_type'] ); if ( ! empty( $post_type_obj->has_archive ) ) $this->is_post_type_archive = true; }
What about when you are on a taxonomy page and use 'pre_get_posts' to set an author etc. etc.? We can't apply band-aids like this.
You can reset
$wp_query->queried_object
to whatever you need, without modifying Core: