Opened 21 months ago

Last modified 4 days ago

#18614 reopened defect (bug)

post_type_archive_title doesn't work when tax_query is added to wp_query

Reported by: wonderboymusic Owned by:
Priority: normal Milestone: Future Release
Component: Template Version: 3.2
Severity: normal Keywords: has-patch needs-refresh
Cc: bill.erickson@…, styledev, benjmay

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 (3)

patches-post-type-archive-title.diff (486 bytes) - added by wonderboymusic 21 months ago.
Revised Patch for this ticket
18614.diff (4.0 KB) - added by nacin 16 months ago.
18614.2.diff (4.2 KB) - added by nacin 16 months ago.

Download all attachments as: .zip

Change History (30)

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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:

function change_queried_object() {
  global $wp_query;

  if ( is_post_type_archive() )
    $wp_query->queried_object = get_post_type_object( get_post_type() );
}
add_action( 'template_redirect', 'change_queried_object' );
Last edited 21 months ago by scribu (previous) (diff)

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

Revised Patch for this ticket

comment:3 follow-up: ↓ 5   scribu21 months ago

Ok, and what happens if 'post_type' is set to an array by some other code that alters $wp_query?

PS: When you upload a revised patch, don't overwrite the previous version.

comment:5 in reply to: ↑ 3 ; follow-up: ↓ 6   wonderboymusic21 months 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"
}

comment:6 in reply to: ↑ 5   wonderboymusic21 months 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

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 is to be used only within The Loop.

Last edited 5 months ago by scribu (previous) (diff)

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 );
  • 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.

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.

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.

  • 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.

From #19035:

Example: When viewing example.com/my_post_type/?my_taxonomy=my_term, the return value of get_queried_object() is the term object, not the post type object. This causes a PHP notice when post_type_archive_title() attempts to access the label for the post type object, and results in a blank title.

  • 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.

nacin16 months ago

nacin16 months 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.

  • 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.

  • Milestone changed from 3.4 to Future Release

Closed #20994 as a duplicate.

comment:21 follow-up: ↓ 22   styledev5 months 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.

comment:22 in reply to: ↑ 21   SergeyBiryukov5 months ago

Replying to styledev:

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() )

Related: [21984], [22483] (for #21648).

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

Ben, I guess the notices weren't the same and for your notices there is a patch for that in ticket #21394

  • 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

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

comment:27   ryan4 days ago

  • Milestone changed from 3.6 to Future Release
Note: See TracTickets for help on using tickets.