Abstract some embed template code into functions

[johnbillion]

I think this should be a pre-cursor to #34278.

If an embed template is to be introduced into the template hierarchy, some of the sections in wp-includes/embed-template.php should be abstracted into functions to avoid the need for themes to copy and paste large blocks of code.

The following sections should be abstracted into template functions:

• Header
• Featured image logic
• Share dialog
• Footer

[swissspidy]

I'd like to get this and #34278 into 4.4 so we have a good template right from the start.

34561.diff​ adds the_embed_header() (includes the featured image and the post title) and the_embed_footer() (includes the site title and sharing options) template tags. print_embed_sharing_dialog() is hooked to the embed_footer action to print the necessary markup for the sharing dialog.

This already leads to a greatly simplified template, consisting of only 88 lines!

Questions:

the_embed_header() is rather long. How could the featured image logic be separated out of it? The image is displayed before or after the heading, depending on the image shape. Maybe that part could be simplified?

Also, should the_excerpt_embed() function also be used to print the 'Nothing found' message on 404s?

[swissspidy]

34561.2.diff​ is a small update to the previous patch that removes a <div class="wp-embed-footer"> from the template I missed before.

the_embed_header() is rather long. Maybe that part could be simplified?

I'd say not in 4.4.

Also, should the the_excerpt_embed() function be used to print the 'Nothing found' message on 404s?

WordPress themes usually don't do that, so I'd say no.

[juliobox]

Please try to keep consistency between function names
*_embed() OR the_embed_*() OR embed_*()
and not a mix of that, thanks :)

[swissspidy]

Replying to juliobox:

Please try to keep consistency between function names
*_embed() OR the_embed_*() OR embed_*()
and not a mix of that, thanks :)

the_footer_embed or the_header_embed sound strange though and there's no equivalent function without this suffix (whereas the_excerpt_embed is the equivalent to the_excerpt()). It makes sense to use the the_ prefix, as these are template tags meant to be used to echo data in theme templates.

---

Anyway, it's a minor point. I'm more wondering if the overall approach seems reasonable. After that we can still change function names.

@johnbillion any opinion on the current patch?

[swissspidy]

34561.3.diff​ is a refreshed patch against trunk.

[juliobox]

I don't think that the_embed_footer should print the share button, since we can remove_filter on print_embed_sharing_dialog if i don't want this embed to be shared.
I just tested it and of course it's weird to have the share button with no effect on click.

The attached patch will separate the button and the dialogs by adding a new action hook.

[swissspidy]

I don't think that the_embed_footer should print the share button, since we can remove_filter on print_embed_sharing_dialog if i don't want this embed to be shared.

That makes sense, but we should print the comments button similarly. Note that there's already the embed_content_meta hook we can use here.

34561.5.diff​ leverages this hook with two new functions: print_embed_sharing_button and print_embed_comments_button.

[Nikschavan]

The latest patch looks and works perfectly as expected. while we are at it, should we also synchronize the names for the newly added functions and filters?

I think for the template functions the_embed_* and for the hooks embed_* seems to be followed.

Ref. ticket:34272#comment:7

[Nikschavan]

Renames the_excerpt_embed() to the_embed_excerpt() and the_excerpt_embed to embed_the_excerpt after 34561.5.diff

[juliobox]

embed_the_excerpt is weird. I rather prefer to have a hook with the same name of the function.
(no offense on your choice @Nik ;) )

[Nikschavan]

Huh! Now that I think about it, embed_the_excerpt does sound bit weird, Maybe only embed_excerpt or as @juliobox rightly suggested - the_embed_excerpt same as the function name.

[swissspidy]

I think for the template functions the_embed_* and for the hooks embed_* seems to be followed.

It's worth noting that the_excerpt_embed() is more similar to the_excerpt_rss(), that's why I chose it. Easier for devs to find while browsing the source.

If we're gonna rename it (which is not 100% necessary IMHO), it should be done in a separate patch/commit.

Note: I'd love to use the_excerpt() itself in the embed template, but too much plugins hook into that.

[juliobox]

I totally agree, don't reuse the front-end function, it's ok for me to use another function and hook for a new feature.

[DrewAPicture]

In 35689:

Embeds: Introduce print_embed_comments_button(), print_embed_sharing_button(), and print_embed_sharing_dialog(), which respectively output the comments button, sharing buttons, and sharing dialog elements in the embed template.

This change hooks these new output functions to existing hooks in the embed template, allowing for more straightforward display control of these elements.

Leaves the embed header and footer intact pending further modularization in a future release.

Props juliobox, swissspidy, DrewAPicture.
See #34561.

[DrewAPicture]

Following [35689] for 4.4, moving to future release for consideration of further modularizing the embed template header and footer.

[swissspidy]

So, let's have a look at this again for 4.5!

Some observations:

• #34278 is something we should strive for, which means we need to make it easy for people to customize the embed template.
• The whole thumbnail image size handling (square / attachment) should be simplified or moved to a function.
• Since the thumbnail is either before or after the heading, basically the whole header could be moved to a function. On the other hand, this could maybe be solved using CSS alone.
• The embed site title, and therefore basically the whole footer, is there twice and should be moved to a function

[swissspidy]

The theme directory now uses a custom template, with the following comment:

Replace original instead of using filters because the thumbnail image is not easily filterable

See ​https://meta.trac.wordpress.org/changeset/2190

We should take this into account to make customization easier.

[Otto42]

The embed template is pretty awful as-is. Replacing it entirely is pretty much the only option available for customizing it, and the only route that anybody is going to actually take, at present.

Either the entire thing should be abstracted into sections and pieces and every element made filterable, or the idea should be scrapped in favor of wholesale replacement. As it stands, you basically have to choose one or the other.

All that rectangular/square/attachment ID logic needs to be eliminated entirely, and moved into functions elsewhere. All that is needed in the template is a filter to put an image URL in. Anything more than that is super overkill.

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. Fix the filter names. Affects #34278 as well.

[swissspidy]

Thanks for your feedback, Sam! Good that we agree on things that need to be improved.

34561.8.diff​ is a suggested first step into this direction for 4.5.

• Adds new embed_heading_before (before <p class="wp-embed-heading">…, embed_content_before (between heading and excerpt), and embed_content_footer (inside <div class="wp-embed-footer">) hooks
• Adds $thumbnail_id parameter to embed_thumbnail_image_size and embed_thumbnail_image_shape filters
• Hooks a new print_embed_thumbnail() function to embed_heading_before and embed_content_before. The attachment is only displayed once depending on the size.
• Hooks a new print_embed_site_title() function to embed_content_footer.

Asked @imath for feedback as well as he implemented this in his ​WP Idea Stream plugin for embedding user profiles.

[imath]

Hi @swissspidy

Sorry it took me so long to reply and thanks a lot for the ping :)

Well WP Idea Stream is filtering embed_template and is using its own template not necessarly because of "not enough hooks" inside the embed-template.php file, but because a user is not a post.

As the embed content is only displayed if have_post() and as i had no way to filter the "not found" embed heading and excerpt, filtering embed-template.php seemed the only way for me.

I guess your patch is improving a bit this for the "not found" excerpt, but i'd still have to hide the "not found" heading using css or javascript. But, to be honest, on the other hand i don't see a reason to change something in my plugin because today it's avoiding a post query loop for a post that is not existing anyway :)

imho, i'm not sure having a lot of hooks is the best road, i saw you're adding hooks before and after almost each tag which is nice. I'm a bit used to it with BuddyPress templates :)

I think the problem with this is: i saw you are planing to use get_query_template() to eventually get the embed template within the theme. In this case having too much hooks can be risky because it gives the illusion to a plugin author he can safely play at a place that won't be there at 100% if the theme is removing some of these hooks within its template. And i've just looked at some theme's template, it looks like it's not a common practice to include hooks inside theme templates. Proof is: the only do_action available in twentysixteen is do_action( 'twentysixteen_credits' );;) So i guess adding hooks is great as soon as they are inside template tag functions.

I saw that you were also loading locale_stylesheet inside the embed_header, there might be a good reason i don't see, but it seems a bit weird to me to load a complete theme stylesheet for a few parts to style.

More globally:

I'm very interested about the get_query_template() road and your .4 patch on #34278. imho this road means you need to do things the way theme authors are used to. It looks a possible way to achieve this is to:

1/ Add new templates inside wp-includes/theme-compat :
header-embed.php where you could put the specific header for embed content.
footer-embed.php where you could put the specific footer for embed content.
content-embed.php where you could put embed content

2/ Edit the embed-template.php to use functions like this

get_header( 'embed' ) which would load the header-embed.php template of the theme or fallback in wp-includes/theme-compat
get_footer( 'embed' ) which would load the footer-embed.php template of the theme or fallback in wp-includes/theme-compat

3/ Rename print_embed_thumbnail() to something like the_post_embed_thumbnail() so that it sounds like a template tag :) and leave theme choose whether to use or not the thumbnail into their embed content.

4/ Create a new template tag to output the embed content, something like this

function get_embed_template_part() {
	// First try to use the content-embed.php template part provided by the theme
	if ( '' !== get_template_part( 'content', 'embed' ) ) {
		return;
	}

	// Nothing found, let's use the default output
	load_template( ABSPATH . WPINC . '/theme-compat/content-embed.php' );
}

This way embed-template.php could only take 3 lines and would become a lot more customizable :)

<?php get_header( 'embed' );
get_embed_template_part();
get_footer( 'embed' );

If you think it's a good idea, i can try to suggest a patch about it, but i guess it's requiring #34278 to be committed first ;)

[ChriCo]

Oh... i've uploaded to the wrong issue.. the upload should be in #34278, can someone remove my patch? :-)

[imath]

@swissspidy

hi, 34561.9.patch is illustrating my previous #comment:22

[swissspidy]

I think the problem with this is: i saw you are planing to use get_query_template() to eventually get the embed template within the theme. In this case having too much hooks can be risky because it gives the illusion to a plugin author he can safely play at a place that won't be there at 100% if the theme is removing some of these hooks within its template.

This is definitely a valid argument and the reason why I'm glad about having 34561.9.patch​ as a new proposal. It's completely different to 34561.8.diff​ and I now see that having dozens of new functions and hooks is not the best way forward.

34561.9.patch​ is a good step in the right direction with the different compatibility files. get_embed_template_part() and the_post_embed_thumbnail() make it feel like embeds are totally different to normal theme files.

I think we could just use get_template_part() and fall back to the theme-compat directory in locate_template(). Currently working on a patch for this.

[swissspidy]

34561.9.diff​ is a streamlined version of the previous patches in need of some testing.

By extending locate_template() to also look in wp-includes/theme-compat the template lookup can be greatly simplified. There shouldn't be any back-compat problems with that.

The usage of print_embed_thumbnail() in this patch might seem weird. If anyone has a better solution (perhaps CSS only positioning?), let me know.

[swissspidy]

We could move wp-includes/embed-template.php to wp-includes/theme-compat/embed.php as well. It would make it more consistent and help simplify #34278.

[jorbin]

Let's commit this and see if anything breaks during beta.

[DrewAPicture]

Added 34561.10.diff​ as an MVP as discussed with @ocean90 in the linked Slack discussion above.

Uses svn cp to create the new templates including copying embed-template.php to theme-compat/embed-template.php. MVP is to move the image gymnastics code directly into the embed-content.php template.

[DrewAPicture]

MVP

[DrewAPicture]

In 36693:

Embeds: Introduce embed templates into the template hierarchy via theme-compat.

Splits wp-includes/embed-template.php, introduced in 4.4, into five new templates that can be individually overridden by themes:

• embed.php
• embed-404.php
• embed-content.php
• header-embed.php
• footer-embed.php

Also introduces a new template tag for outputting the site title, the_embed_site_title().

The five new templates live in theme-compat, allowing for graceful fallbacks should themes prefer not to override any or all of them.

Props swissspidy, imath, ocean90, DrewAPicture.
See #34561.

[DrewAPicture]

In 36694:

Embeds: Update embed template paths and messages in tests, missed in [36693].

Props ocean90
See #34561.

[DrewAPicture]

In 36746:

Docs: Update the @deprecated tag comment for wp-includes/embed-template.php to reference the correct file path following [36693].

See #34561.

[swissspidy]

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.

[swissspidy]

@DrewAPicture @ocean90 Anything still to be done here after [36694] and #34278?

[ocean90]

I have nothing else, let's close it as fixed.

(A make/core post would be neat though.)

[DrewAPicture]

In 36923:

Docs: Improve the DocBlock summary for the_embed_site_title(), introduced in [36693].

See #34561. See #35986.

[DrewAPicture]

In 36963:

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

See #34561, #34278. See #35986.

[DrewAPicture]

In 36965:

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

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

[SergeyBiryukov]

In 37232:

Theme Editor: After [37217], add other embed templates to file descriptions.

Props Frozzare.
Fixes #36551. See #34561.