Make WordPress Core

Opened 13 years ago

Closed 9 years ago

Last modified 9 years ago

#15337 closed defect (bug) (fixed)

fix get_attachment_template() to pass templates array

Reported by: willnorris's profile willnorris Owned by: johnbillion's profile johnbillion
Milestone: 4.3 Priority: normal
Severity: normal Version: 2.0
Component: Themes Keywords: has-patch needs-docs
Focuses: template Cc:

Description

get_attachment_template() currently queries for a number of templates based on the mime-type of the attachment. It does so by checking each one individually, and returning as soon as it finds one. It should instead build an array of templates, and pass the entire array to get_query_template().

patch attached.

Attachments (6)

16230.diff (842 bytes) - added by willnorris 13 years ago.
template.php.diff (962 bytes) - added by jfarthing84 12 years ago.
Reorder attachment template hierarchy, change underscore to dash in most specific scenario and pass all templates as an array.
15337.patch (1.1 KB) - added by SergeyBiryukov 11 years ago.
15337.2.patch (1.3 KB) - added by SergeyBiryukov 11 years ago.
15337.3.patch (1.5 KB) - added by SergeyBiryukov 11 years ago.
15337.4.patch (1.7 KB) - added by DrewAPicture 10 years ago.
refresh + list()

Download all attachments as: .zip

Change History (28)

@willnorris
13 years ago

#1 @willnorris
13 years ago

to be honest, I also think that order it attempts the templates is completely backwards. For an "image/jpeg" attachment, it currently attempts:

  • image.php
  • jpeg.php
  • image_jpeg.php
  • attachment.php

the mime-type portion of this list is going from general to specific... shouldn't it instead start by looking for the most specific template, and then back off to the most general? so ...

  • image_jpeg.php
  • jpeg.php
  • image.php
  • attachment.php

Now I realize this is not something that can be easily changed since it could potentially break a number of themes. Though I'd be highly surprised if there are that many themes that have different templates for "image.php" and "image_jpeg.php".

perhaps this should be filed as a separate bug so it can be discussed, and not hold up this other patch?

#2 @nacin
13 years ago

  • Milestone changed from Awaiting Review to 3.1

Reversing the order has been mentioned in previous tickets, but I don't think we should bother with thar.

Patch makes sense.

#3 follow-up: @greenshady
13 years ago

Note that the patch will break older hooks:

  • $type[0]_template
  • $type[1]_template
  • $type[0]_$type[1]_template

We'll only be left with the attachment_template hook. I actually think this makes more sense anyway because of the addition of the $type_template_hierarchy hook added to the get_query_template()function.

I don't think the change would really break compatibility with many (if any) themes/plugins though.

#4 @nacin
13 years ago

  • Cc has-patch removed
  • Keywords 2nd-opinion added
  • Milestone changed from 3.1 to Future Release

#5 @johnbillion
13 years ago

  • Cc johnbillion@… added

#6 @chipbennett
12 years ago

  • Cc chip@… added

#7 @jfarthing84
12 years ago

  • Cc jeff@… added

Related #21053.

#8 @jfarthing84
12 years ago

  • Keywords dev-feedback added; 2nd-opinion removed

#9 follow-up: @jfarthing84
12 years ago

Also just noticed that with the most specific template, the intended result isn't even used. For instance, a JPEG image would be image_jpeg.php. However, get_query_template uses a regular expression that removes the underscore, thus changing the template it looks for to imagejpeg.php. I suggest that the underscore be changed to a dash, thus matching the WordPress file naming convention and avoiding this problem.

@jfarthing84
12 years ago

Reorder attachment template hierarchy, change underscore to dash in most specific scenario and pass all templates as an array.

#10 @jfarthing84
12 years ago

  • Keywords has-patch added

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

Replying to jfarthing84:

However, get_query_template uses a regular expression that removes the underscore, thus changing the template it looks for to imagejpeg.php.

Related: #21213

#12 @SergeyBiryukov
11 years ago

#23151 was marked as a duplicate.

#13 @SergeyBiryukov
11 years ago

  • Component changed from Themes to Template
  • Milestone changed from Future Release to 3.6
  • Version set to 2.0

Two refreshed patches:

  • 15337.patch keeps the backwards compatibility, except for the underscore issue in comment:9 (i.e. image_jpeg.php, which previously didn't work, would now do).
  • 15337.2.patch reverses the order per comment:1 and changes the underscore to a dash.

Both patches also use get_queried_object() instead of the $posts global for consistency with other get_*_template() functions.

#14 in reply to: ↑ 3 @SergeyBiryukov
11 years ago

15337.3.patch is an attempt to keep backwards compatibility for filters, per comment:3.

#15 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#16 @nacin
10 years ago

  • Component changed from Template to Themes
  • Focuses template added

Sure would be nice to fix this. I think I could go for 15337.2.patch.

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


10 years ago

@DrewAPicture
10 years ago

refresh + list()

#18 @obenland
10 years ago

  • Keywords dev-feedback removed

#19 @johnbillion
9 years ago

  • Keywords 4.3-early needs-docs added
  • Owner set to johnbillion
  • Status changed from new to assigned

#20 @obenland
9 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

#21 @wonderboymusic
9 years ago

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

In 32804:

In get_attachment_template(), pass an array of templates to get_query_template( 'attachment', $templates ), instead of bailing on the first found template.

Props willnorris, jfarthing84, SergeyBiryukov, DrewAPicture, wonderboymusic.
Fixes #15337.

#22 @johnbillion
9 years ago

  • Keywords changed from has-patch, needs-docs to has-patch needs-docs

\o/

Note: See TracTickets for help on using tickets.