Make WordPress Core

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's profile wonderboymusic Owned by: wonderboymusic's profile 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)

patches-post-type-archive-title.diff (486 bytes) - added by wonderboymusic 13 years ago.
Revised Patch for this ticket
18614.diff (4.0 KB) - added by nacin 13 years ago.
18614.2.diff (4.2 KB) - added by nacin 13 years ago.
18614.3.diff (4.2 KB) - added by wonderboymusic 11 years ago.
18614.4.diff (4.0 KB) - added by wonderboymusic 11 years ago.
18614.5.diff (5.6 KB) - added by wonderboymusic 11 years ago.

Download all attachments as: .zip

Change History (47)

#1 @scribu
13 years 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.

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 13 years ago by scribu (previous) (diff)

#2 @wonderboymusic
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

@wonderboymusic
13 years ago

Revised Patch for this ticket

#3 follow-up: @scribu
13 years ago

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

#4 @scribu
13 years ago

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

#5 in reply to: ↑ 3 ; follow-up: @wonderboymusic
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 @wonderboymusic
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 @scribu
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 is to be used only within The Loop.

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

#8 @wonderboymusic
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 @scribu
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 @dd32
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 @johnbillion
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 @nacin
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 @SergeyBiryukov
13 years 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.

#14 @billerickson
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.

@nacin
13 years ago

@nacin
13 years ago

#15 @nacin
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 @scribu
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.

#18 @ryan
13 years ago

  • Milestone changed from 3.4 to Future Release

#19 @SergeyBiryukov
12 years ago

Closed #20994 as a duplicate.

#21 follow-up: @styledev
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.

#22 in reply to: ↑ 21 @SergeyBiryukov
12 years 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).

#23 @benjmay
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 @markoheijnen
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 @wonderboymusic
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 @styledev
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

#27 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#28 @Frank Klein
11 years ago

  • Cc contact@… added

#29 @dominicp
11 years ago

  • Cc solaveritasteliberum@… added

#30 @wonderboymusic
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 @wonderboymusic
11 years ago

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

In 25291:

Ensure that the post type object is the queried object when a post type has been registered with has_archive => true. Ensure it is not stomped when decorated with tax_query. Adds unit tests.

Props nacin.
Fixes #18614.

#32 @nacin
11 years ago

  1. 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.
  1. 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 @wonderboymusic
11 years ago

  1. 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.
  1. As long as is_tax() is false when the post_type obj is the queried object, I think it is safe.

#34 @wonderboymusic
11 years ago

In 25292:

Check the value passed to get_post_type_object(). If it's an array, use the first item. get_query_var( 'post_type' ) can be an array if the query has been altered via filters/actions. There are several places in core that pass the query var. Adds unit tests.

In template-loader.php, move is_post_type_archive() and is_tax() directly below is_home().

See #18614, [25291].

#35 @nacin
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 @wonderboymusic
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

#37 @nacin
11 years ago

By wonderboymusic, in [25312]: Move checks for post_type being an array inline. See [25291], [25292], #18614.

(The ticket(s) has to come before any changeset references for "see" or "fixes" to work.)

#38 @nacin
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;
}

#39 @kovshenin
11 years ago

  • Cc kovshenin added

#40 @nacin
11 years ago

See #25341 for problems.

#41 @nacin
11 years ago

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

This ticket still has a needs-unit-tests keyword, but seems like everything is good for 3.7.

Note: See TracTickets for help on using tickets.