Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#34278 closed task (blessed) (fixed)

Add support for embeds in the theme template hierarchy

Reported by: johnbillion's profile johnbillion Owned by: swissspidy's profile swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch 4.5-early commit
Focuses: template Cc:

Description

oEmbed embeds (introduced in #32522) use wp-includes/embed-template.php as the template file. The embed_template filter is available to override this.

The addition of an embed.php file to the template hierarchy should be considered, to make it easier for theme developers to style the output. Portions of the current embed template (particularly the share dialog markup) could be split into template functions to aid this.

Thoughts?

Attachments (7)

34278.diff (2.4 KB) - added by swissspidy 8 years ago.
twentysixteen-wp-embed.png (1.5 MB) - added by swissspidy 8 years ago.
Demo of a custom embed template in Twenty Sixteen
34278.2.diff (2.4 KB) - added by swissspidy 8 years ago.
34278.3.diff (2.4 KB) - added by swissspidy 8 years ago.
34278.4.diff (2.4 KB) - added by swissspidy 8 years ago.
New patch using embed.php
34278.get_embed_template.patch (2.6 KB) - added by ChriCo 8 years ago.
added get_embed_template() function with template hierarchy for embed-{$object->post_type}-{$object->post_name}.php, embed-{$object->post_type}.php, embed.php and fallback to WP-default ABSPATH . WPINC . '/embed-template.php'
34278.5.diff (2.4 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (57)

#1 @dshanske
8 years ago

I think this should be a theme issue because it deals with design.

Splitting the pieces up to make custom templates easier is also worth it.

#2 @swissspidy
8 years ago

It's worth noting that the embed template isn't just a regular theme template. For example, there's no wp_head, but a do_action( 'oembed_head' ); instead. That's why I hesitated to introduce a get_embed_template function and add it to the "waterfall" in template-loader.php.

Happy to reconsider this, but we need to make sure people don't mess it up.

@swissspidy
8 years ago

#3 @swissspidy
8 years ago

  • Component changed from Themes to Embeds
  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.4

34278.diff checks for an embed.php file in the theme before including the default file before including wp-includes/embed-template.php.

Benefit of this solution: We don't mess with the dynamic $type_template hook.

embed.php is a bit generic though and could already be used by some themes. What about wp-embed.php?

#4 @johnbillion
8 years ago

I like this, but I agree that because it's not a regular theme template it does become somewhat of an outlier. It will be the only template in the default template hierarchy that acts differently to others.

Maybe we need an example template for Twenty Fifteen or Twenty Sixteen, in order to see if this works well in practice.

#5 @peterwilsoncc
8 years ago

My inclination is embed-template.php or wp-embed.php. Should it be renamed, renaming the core template would help for the purposes of consistency.

I am very much +1 on the template being in the template hierarchy from 4.4. Should it be included, would moving the core default template to the wp-includes/theme-compat/ directory be warranted?

#6 @swissspidy
8 years ago

Yeah, wp-embed.php sounds better for the theme template, though I wouldn't necessarily move the core default template to wp-includes/theme-compat/ as the files in that directory are deprecated and only there for backwards compatibility reasons.

Maybe we need an example template for Twenty Fifteen or Twenty Sixteen, in order to see if this works well in practice.

I'll play around with Twenty Sixteen a bit and post screenshots here.

@swissspidy
8 years ago

Demo of a custom embed template in Twenty Sixteen

#7 @swissspidy
8 years ago

Attached is a demo of a custom embed template in Twenty Sixteen. Although most of the styling can be done using the various hooks in place, having complete control over the template made it easier to customize it.

One risk would be that plugin would have a hard time adjusting the template when themes remove those hooks.

#8 @wonderboymusic
8 years ago

  • Type changed from enhancement to task (blessed)

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

@swissspidy
8 years ago

#10 @swissspidy
8 years ago

34278.2.diff is an updated patch that changes the theme template name to wp-embed.php.

Do we want to rename the default template or move it to the theme-compat directory?

#11 @swissspidy
8 years ago

Here's an example wp-embed.php file for Twenty Sixteen, in case it helps: https://gist.github.com/swissspidy/cba93d791f6ce031aec0

#12 @johnbillion
8 years ago

  • Keywords 2nd-opinion removed

I'm recommending that #34561 happens before this. It could still happen in 4.4.

Regarding the file name, I'm not a fan of the wp- prefix. embed-template.php could be an option.

#13 @juliobox
8 years ago

embed-template.php sounds good for me. and i agree to resolve #34561 first. i just added another patch.

Last edited 8 years ago by juliobox (previous) (diff)

@swissspidy
8 years ago

#14 @swissspidy
8 years ago

34278.3.diff is an updated version of the first patch with fixed whitespace.

This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.


8 years ago

#16 @DrewAPicture
8 years ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

#17 @DrewAPicture
8 years ago

  • Keywords 4.5-early added
  • Owner DrewAPicture deleted

[35689] introduced the ability to further control display of some elements in the embed template via hooks. Let's pick up adding this to the template hierarchy in 4.5-early.

#18 @DrewAPicture
8 years ago

  • Milestone changed from 4.4 to Future Release

#19 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.5

Since it's already a template file, naming it embed-template.php seems redundant to me. What about embed.php?

I'm not aware of any theme that uses this template, at least I didn't find one in the SVN repo.

Last edited 8 years ago by swissspidy (previous) (diff)

#20 @Otto42
8 years ago

BTW, the "embed_template" filter is a bad filter name because it is shared with the *_template filter in get_query_template. If you hook to embed_template and then call get_query_template('embed') from inside that function, you have created an infinite loop without knowing why. Ran into that this morning.

@swissspidy
8 years ago

New patch using embed.php

#21 @swissspidy
8 years ago

  • Keywords needs-testing added

Thanks for your feedback, @Otto42! Another good reason to change this.

I just updated the patch for 4.5. Needs testing because of the is_embed() check before is_404().

#22 @swissspidy
8 years ago

#35074 was marked as a duplicate.

#23 @frizull
8 years ago

Would it be feasible to have different template files for different post types? Something akin to embed-post_type.php, like we have for archives?

#24 follow-up: @Otto42
8 years ago

I'd kind of prefer embed-post_format, honestly. For post types, embeds really don't make a heck of a lot of sense except in special cases, and filters would work better for adding special formats to go with the post types. For themes that support post formats though, special templates for embed per format make more sense.

#25 follow-up: @peterwilsoncc
8 years ago

Basing it on the post hierarchy as closely as possible, the embed hierarchy would be:

  1. embed-{post-type}-{slug}.php
  2. embed-{post-type}.php
  3. embed.php
  4. built in template

Post type allows for posts, pages, CPT so I'm more inclined for that as post-format is post only.

#26 in reply to: ↑ 25 @ChriCo
8 years ago

Replying to peterwilsoncc:

+1 embed-templates should have the same hierarchy as post/page-templates. Maybe embed-{post_ID}.php (same as page-{post_ID}.php) could be added as well.

Last edited 8 years ago by ChriCo (previous) (diff)

@ChriCo
8 years ago

added get_embed_template() function with template hierarchy for embed-{$object->post_type}-{$object->post_name}.php, embed-{$object->post_type}.php, embed.php and fallback to WP-default ABSPATH . WPINC . '/embed-template.php'

#27 @swissspidy
8 years ago

I'd kind of prefer embed-post_format, honestly. For post types, embeds really don't make a heck of a lot of sense except in special cases, and filters would work better for adding special formats to go with the post types. For themes that support post formats though, special templates for embed per format make more sense.

I tend to think the same, but post formats are currently not really taken into account in the template hierarchy. Themes are usually using get_template_part() for different post formats. embed-format-{post-format}.php could be an option though.

Other than that, 34278.get_embed_template.patch looks good. We just need to make sure nothing breaks when checking for is_embed() before is_404() and that customizing the embed template is actually easy enough.

That's why I think this should go hand in hand with #34561.

#28 @ramiy
8 years ago

With embed-format-{post-format}.php you are creating inconsistency in the template hierarchy.

@peterwilsoncc suggested to use embed-{post-type}-{slug}.php (comment 25), based on post and pages hierarchy. The use of post types is more common in wordpress template hierarchy.

#29 in reply to: ↑ 24 @ramiy
8 years ago

Replying to Otto42:

For themes that support post formats though, special templates for embed per format make more sense.

I understand your logic, different embed templates for text, video, audio and other formats. It's nice. But the problem with post formats is that it's a closed list of formats. Developers can't add new formats. And (my) clients usually skip the formats - they prefer to add another taxonomy term for videos.

Also, I work with israeli startups and almost all my projects use custom post types (+95%). With post types I can create new "formats" like code snippets, presentations, maps, polls, etc. Post types provides more control to developers with single-{post-type}.php and archive-{post-type}.php.

Each one of those new post-types needs a different embed template. This is why I think embed-{post-type}.php is better - its consistent with the current template hierarchy.

#30 @swissspidy
8 years ago

Note that the latest patch on #34561 would make supporting post formats easier, in a way theme authors are used to.

#31 follow-up: @Otto42
8 years ago

But the problem with post formats is that it's a closed list of formats. Developers can't add new formats.

That's the benefit of Post Formats. That's a good thing, not a downside.

More to the point, post types don't need a hierarchy for embeds, because that's not the purpose of post types. A custom Post Type is not just some special kind of "Page" after all.

Custom post types should not be considered for inclusion in this hierarchy at all. It doesn't make sense to include them.

#32 in reply to: ↑ 31 @ramiy
8 years ago

Replying to Otto42:

More to the point, post types don't need a hierarchy for embeds, because that's not the purpose of post types. A custom Post Type is not just some special kind of "Page" after all.

Custom post types should not be considered for inclusion in this hierarchy at all. It doesn't make sense to include them:

I have many examples why you should considered post-types and why it makes sense to include them:

  • Code snippets post-type, when embedded, will show the code in a beautiful code editor, with it's own JS and CSS. I can do it easily using embed-snippet.php.
  • Maps post-type, when embedded, will show only the map. I don't need the site title, the description, or the other elements the default embed template present. With embed-map.php I can embed only the map.
  • Video post-type, when embedded, will show a player like youtube. The player has it's own embed button. We don't want to use the the default embed buttons. embed-video.php is the solution.
  • Presentations post-type, when embedded, will show the presentations player using embed-presentation.php.

The new embed-templates for specific post-types will make it easier to build complex application using WordPress.

#33 follow-up: @Otto42
8 years ago

Post types are created in plugins. If you need to include a special embed format for a post type, then that format should really be in the same plugin that contains the post type. It doesn't make a whole lot of sense to include post-type support for them in the theme, because the theme doesn't necessarily need to know about the post types. Post types and themes don't really mix well.

I'm not saying that post types can't get special embed handlers, but separation of concerns means that it makes more sense to include said embed handler in the plugin where the post type is created in the first place.

For example, for your code snippet post type, the plugin that adds this could hook in and include the embed-code in that plugin, and then the theme would remain independent of it and available for use elsewhere.

#34 in reply to: ↑ 33 @ramiy
8 years ago

Again, I understand the logic, post types are plugin territory. So why do we support single-{post-type}.php and archive-{post-type}.php in themes? And if WordPress already supports those template files, why not embed-{post-type}.php?

#35 follow-ups: @Otto42
8 years ago

I never said that what we already have was consistent. :)

But just because some existing part is wrong doesn't mean we should continue along those lines. Post types should be the domain of plugins, really. Making themes have additional knowledge of them is bad in general.

#36 in reply to: ↑ 35 @ramiy
8 years ago

Replying to Otto42:

I never said that what we already have was consistent. :)

Although you say it's wrong, I think we should have the same implementation, for the sake of consistency.

Making themes have additional knowledge of them is bad in general.

Actually, as a theme developer, I can say that additional theme functionality is a blessing. More options to expand WordPress to new frontiers. :-)

From my experience I can tell you that It's much easier to use custom templates than it is to use dozens of filters and actions what tweak minor element.

#37 @Otto42
8 years ago

Plugins can just as easily add custom templates too. That's not really the issue.

The issue is one of separation of presentation and content. Themes should not have special cases for custom post types because post types are a form of content and themes are the means by which the content is presented. If the content is tied tightly to the theme, then that's a form of lock-in. It limits the ability to change the theme (alter the presentation) without losing some portion of the content. That limitation should be avoided when it is possible to do so.

#38 in reply to: ↑ 35 @peterwilsoncc
8 years ago

Replying to Otto42:

I never said that what we already have was consistent. :)

But just because some existing part is wrong doesn't mean we should continue along those lines. Post types should be the domain of plugins, really. Making themes have additional knowledge of them is bad in general.

I think the current WordPress structure is consistent, templates for CPT's are {archive|single}-{post-type}.php. My proposal in comment 26 is to maintain this consistency with {archive|single|embed}-{post-type}.php.

Pages are an exception, let's not further complicate the theming experience by adding another. It's logical that if a theme does not style a CPT it falls back to the default.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


8 years ago

@swissspidy
8 years ago

#40 @swissspidy
8 years ago

  • Keywords needs-testing removed

34278.5.diff is a refreshed patch after #34561.

The hierarchy is as follows: embed-$post_type-$post_format.php -> embed-$post_type -> embed.php.

It makes sense to include this in 4.5 because these tickets basically go hand in hand.

@DrewAPicture @ocean90 Since you both worked on #34561 as well, I'd love to have your opinion on this.

#41 follow-up: @DrewAPicture
8 years ago

@swissspidy We can't just remove the embed_template filter, we need to support it for back-compat. Otherwise, looks pretty good, other than that I'd probably also add a check on $object following initialization with get_queried_object().

#42 @DrewAPicture
8 years ago

@swissspidy Oh, nvm. I forgot about comment:20. In that case, we should consider whether a special case should be added in get_query_template() for the expected theme-compat path that precedes the hook currently, even though it was changed in 4.5.

#43 in reply to: ↑ 41 @swissspidy
8 years ago

Replying to DrewAPicture:

@swissspidy We can't just remove the embed_template filter, we need to support it for back-compat. Otherwise, looks pretty good, other than that I'd probably also add a check on $object following initialization with get_queried_object().

Regarding the $object check, that's not done in any/most of the other template functions like get_category_template() or get_single_template().

As for the filter, I'm not sure it's worth it / really needed. Let me think about it for a bit.

#44 @DrewAPicture
8 years ago

  • Keywords commit added

Thought this through a little bit and conferred with @swissspidy, and the consensus is that we never should have introduced the embed_template hook in 4.4 because of the conflict pointed out in comment:20

The good news is that @obenland has kindly checked a slurp of the themes repository and it looks like no themes have yet implemented embed.php templates, which means we're in the clear to dump the explicit embed_template filter and simply rely on the fallback behavior in locate_template() to infer the /theme-compat/embed.php path put in place in [36693].

#45 @DrewAPicture
8 years ago

Disclaimer: If it turns out some theme somewhere outside the themes repository is using an embed.php template, I would think we've done due diligence to ensure backward compatibility in terms of the template hierarchy. @obenland is simply the ack master in this scenario and makes no guarantees nor grants liability for the results of his search.

#46 @jorbin
8 years ago

  • Owner set to swissspidy

Making sure this has an owner and since swissspidy led the original work on embeds, he can give a second set of eyes to Drew's commit tag.

#47 @swissspidy
8 years ago

If there are no objections to the suggested hierarchy (embed-$post_type-$post_format.php -> embed-$post_type -> embed.php), this one is good to go.

I checked some other plugins where people filtered embed_template and they wouldn't be affected by this change either.

Unfortunately because loca_template() uses the STYLESHEETPATH and TEMPLATEPATH constants to locate the theme templates, we can't really add a unit test for the hierarchy in a blank theme.

#48 @swissspidy
8 years ago

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

In 36876:

Embeds: Add support for embeds in the theme template hierarchy.

This allows themes to directly override the default template. The order in which the template is retrieved is as follows: embed-$post_type-$post_format.php -> embed-$post_type.php -> embed.php.

The embed_template filter gets replaced by the dynamic {$type}_template filter in get_query_template().

Props ChriCo, swissspidy.
See #34561. Fixes #34278.

#49 @DrewAPicture
8 years ago

In 36963:

Docs: Use a third-person singular verb in the summary for get_embed_template(), introduced in [36876].

See #34561, #34278. See #35986.

#50 @DrewAPicture
8 years ago

In 36965:

Docs: Correct a typo in the DocBlock summary for get_embed_template(), introduced in [36963].

Props swissspidy.
See #34561, #34278. See #35986.

Note: See TracTickets for help on using tickets.