WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 3 years ago

Last modified 3 years ago

#15337 closed defect (bug) (fixed)

fix get_attachment_template() to pass templates array

Reported by: willnorris Owned by: 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 8 years ago.
template.php.diff (962 bytes) - added by jfarthing84 6 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 5 years ago.
15337.2.patch (1.3 KB) - added by SergeyBiryukov 5 years ago.
15337.3.patch (1.5 KB) - added by SergeyBiryukov 5 years ago.
15337.4.patch (1.7 KB) - added by DrewAPicture 4 years ago.
refresh + list()

Download all attachments as: .zip

Change History (28)

@willnorris
8 years ago

#1 @willnorris
8 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
8 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
8 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
8 years ago

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

#5 @johnbillion
7 years ago

  • Cc johnbillion@… added

#6 @chipbennett
7 years ago

  • Cc chip@… added

#7 @jfarthing84
6 years ago

  • Cc jeff@… added

Related #21053.

#8 @jfarthing84
6 years ago

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

#9 follow-up: @jfarthing84
6 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
6 years ago

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

#10 @jfarthing84
6 years ago

  • Keywords has-patch added

#11 in reply to: ↑ 9 @SergeyBiryukov
6 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
5 years ago

#23151 was marked as a duplicate.

#13 @SergeyBiryukov
5 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
5 years ago

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

#15 @ryan
5 years ago

  • Milestone changed from 3.6 to Future Release

#16 @nacin
4 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.


4 years ago

@DrewAPicture
4 years ago

refresh + list()

#18 @obenland
4 years ago

  • Keywords dev-feedback removed

#19 @johnbillion
3 years ago

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

#20 @obenland
3 years ago

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

#21 @wonderboymusic
3 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
3 years ago

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

\o/

Note: See TracTickets for help on using tickets.