#34561 closed task (blessed) (fixed)
Abstract some embed template code into functions
Reported by: | johnbillion | Owned by: | DrewAPicture |
---|---|---|---|
Milestone: | 4.5 | Priority: | normal |
Severity: | normal | Version: | 4.4 |
Component: | Embeds | Keywords: | has-patch needs-testing commit |
Focuses: | Cc: |
Description
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
Attachments (11)
Change History (59)
#3
@
9 years ago
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.
#4
follow-up:
↓ 5
@
9 years ago
Please try to keep consistency between function names
*_embed() OR the_embed_*() OR embed_*()
and not a mix of that, thanks :)
#5
in reply to:
↑ 4
@
9 years ago
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?
#6
@
9 years ago
34561.3.diff is a refreshed patch against trunk.
#7
@
9 years ago
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.
#8
@
9 years ago
- Keywords dev-feedback removed
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
.
#9
@
9 years ago
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.
@
9 years ago
Renames the_excerpt_embed()
to the_embed_excerpt()
and the_excerpt_embed
to embed_the_excerpt
after 34561.5.diff
#10
@
9 years ago
embed_the_excerpt is weird. I rather prefer to have a hook with the same name of the function.
#11
@
9 years ago
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.
#12
@
9 years ago
I think for the template functions
the_embed_*
and for the hooksembed_*
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.
#13
@
9 years ago
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.
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#17
@
9 years ago
- Milestone changed from 4.4 to Future Release
Following [35689] for 4.4, moving to future release for consideration of further modularizing the embed template header and footer.
#18
@
9 years ago
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
#19
@
9 years ago
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.
#20
@
9 years ago
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.
#21
@
9 years ago
- Keywords needs-testing added
- Milestone changed from Future Release to 4.5
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), andembed_content_footer
(inside<div class="wp-embed-footer">
) hooks - Adds
$thumbnail_id
parameter toembed_thumbnail_image_size
andembed_thumbnail_image_shape
filters - Hooks a new
print_embed_thumbnail()
function toembed_heading_before
andembed_content_before
. The attachment is only displayed once depending on the size. - Hooks a new
print_embed_site_title()
function toembed_content_footer
.
Asked @imath for feedback as well as he implemented this in his WP Idea Stream plugin for embedding user profiles.
#22
@
9 years ago
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 ;)
#24
@
9 years ago
Oh... i've uploaded to the wrong issue.. the upload should be in #34278, can someone remove my patch? :-)
#25
@
9 years ago
@swissspidy
hi, 34561.9.patch is illustrating my previous #comment:22
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
9 years ago
#30
@
9 years ago
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.
#31
@
9 years ago
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.
#32
@
9 years ago
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.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by drew. View the logs.
9 years ago
#36
@
9 years ago
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.
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) andthe_embed_footer()
(includes the site title and sharing options) template tags.print_embed_sharing_dialog()
is hooked to theembed_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?