Make WordPress Core

Opened 12 years ago

Closed 12 years ago

#22956 closed defect (bug) (fixed)

Unpredictable template loading when multiple post types are involved

Reported by: scribu's profile scribu Owned by:
Milestone: 3.5.1 Priority: normal
Severity: normal Version: 3.5
Component: Posts, Post Types Keywords: has-patch commit fixed-major
Focuses: Cc:

Description (last modified by scribu)

[21861] sought to improve the template loading behaviour when multiple post types are involved, but I don't think it was the right approach.

1. It creates backward-incompatible behaviour.

Take a query of 'post_type' => array( 'serie', 'post' )

In 3.4, archive.php always ended up being loaded, since it was unlikely for archive-Array.php to exist.

In 3.5, it unexpectedly loads archive-serie.php.

2. The order of the post types becomes important.

For example, 'post_type' => array( 'serie', 'post' ) can potentially load a different template from 'post_type' => array( 'post', 'serie' ).

Possible alternatives:

a) always load archive.php if 'post_type' is an array.

b) choose the template based on get_queried_object().

Attachments (3)

22956.diff (646 bytes) - added by scribu 12 years ago.
22956.2.diff (647 bytes) - added by scribu 12 years ago.
22956.3.diff (663 bytes) - added by scribu 12 years ago.

Download all attachments as: .zip

Change History (22)

#1 @scribu
12 years ago

  • Description modified (diff)

@scribu
12 years ago

#2 @scribu
12 years ago

  • Keywords has-patch added

#3 @nacin
12 years ago

I was the committing developer on [21861]. You're right, that's the wrong approach.

Can we do $post_type = reset( $post_types );? It's a more common pattern for us than list(), which we usually use for more complex situations.

#4 @nacin
12 years ago

  • Component changed from General to Post Types

#5 @nacin
12 years ago

  • Keywords needs-refresh commit added

Not a huge deal for this one, but any possibility of unit tests here?

Marking as needs refresh to use reset().

@scribu
12 years ago

#6 @scribu
12 years ago

  • Keywords needs-refresh removed

Refreshed patch.

Not a huge deal for this one, but any possibility of unit tests here?

It would be possible if there was a filter on the list of templates: http://core.trac.wordpress.org/attachment/ticket/14310/template_hierarchy.4.diff

#7 @nacin
12 years ago

  • Keywords commit removed

get_query_var() likes to return '' when a query variable doesn't exist, and we like resetting query vars to '' as well. In this case, we'd actually go looking for an archive-.php file, I'm pretty sure.

@scribu
12 years ago

#8 @scribu
12 years ago

The array_filter() in 22956.3.diff should take care of that.

#9 @nacin
12 years ago

  • Keywords commit added

The whole patch is ugly, but effective. :-) Nice, thanks.

#10 follow-up: @wonderboymusic
12 years ago

Just want to go on the record for not liking this at all

#11 in reply to: ↑ 10 ; follow-up: @nacin
12 years ago

Replying to wonderboymusic:

Just want to go on the record for not liking this at all

Could you explain what you'd rather see? Considering we're currently trying to load archive-Array.php and archive-.php, the proper thing to do here would be to restore 3.4 behavior. This doesn't at all prevent us from adding items to the template hierarchy in the future, like checking individual post type archives in some order, or consulting a special multiple post type template, or adding in a filter, or something.

#12 @wonderboymusic
12 years ago

this is like doing a WP_Query with 'any' and then loading the archive for the post_type of the first item. That choice is arbitrary. You might as well pick a random post from the collection and load that archive. <IMO>archive.php is the correct template</IMO>. archive-Array and archive-.php both result in archive.php being chosen, so why switch to loading archive-you-pick.php?

#13 in reply to: ↑ 11 @SergeyBiryukov
12 years ago

Replying to nacin:

Considering we're currently trying to load archive-Array.php and archive-.php, the proper thing to do here would be to restore 3.4 behavior.

archive-Array.php was the 3.4 behavior (which #20867 attempted to fix).

22956.3.diff looks sane to me.

Replying to wonderboymusic:

this is like doing a WP_Query with 'any' and then loading the archive for the post_type of the first item.

This seems to describe the current behaviour, which 22956.3.diff fixes.

#14 @wonderboymusic
12 years ago

count( $post_types ) == 1 ... that makes sense, I am going back to my cave now

#15 @wonderboymusic
12 years ago

P.S. I think I freaked out from initially seeing the reset() which reminded me of tax_query and get_queried_object() picking the first one every time: http://core.trac.wordpress.org/browser/trunk/wp-includes/query.php#L2987

#16 @nacin
12 years ago

In 23249:

Ensure that get_archive_template() only loads a post type archive (archive-$post_type.php file) if there is exactly one post type in the query.

props scribu.
see #22956.
for trunk.

#17 @nacin
12 years ago

  • Keywords fixed-major added

#18 @nacin
12 years ago

In 23271:

Ensure that get_archive_template() only loads a post type archive (archive-$post_type.php file) if there is exactly one post type in the query.

Merges [23249] to the 3.5 branch.

props scribu.
see #22956.

#19 @nacin
12 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.