WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 4 weeks 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 22 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)

comment:1 scribu22 months ago

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

As a workaround for your problem, 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' );
Version 1, edited 22 months ago by scribu (previous) (next) (diff)

comment:2 wonderboymusic22 months 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

wonderboymusic22 months ago

Revised Patch for this ticket

comment:3 follow-up: scribu22 months ago

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

comment:4 scribu22 months ago

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

comment:5 in reply to: ↑ 3 ; follow-up: wonderboymusic22 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 wonderboymusic22 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

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

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

comment:8 wonderboymusic22 months 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 );

comment:9 scribu22 months 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.

comment:10 dd3222 months 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.

comment:11 johnbillion20 months 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.

comment:12 nacin19 months 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.

comment:13 SergeyBiryukov19 months ago

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.

comment:14 billerickson19 months 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.

nacin16 months ago

nacin16 months ago

comment:15 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.

comment:16 scribu16 months 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.

comment:18 ryan14 months ago

  • Milestone changed from 3.4 to Future Release

comment:19 SergeyBiryukov12 months ago

Closed #20994 as a duplicate.

comment:21 follow-up: styledev6 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 SergeyBiryukov6 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).

comment:23 benjmay6 months 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 ); 

comment:24 markoheijnen6 months ago

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

comment:25 wonderboymusic6 months 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

comment:26 styledev3 months 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

comment:27 ryan4 weeks ago

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