Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34561 closed task (blessed) (fixed)

Abstract some embed template code into functions

Reported by: johnbillion's profile johnbillion Owned by: drewapicture's profile 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)

34561.diff (14.8 KB) - added by swissspidy 9 years ago.
34561.2.diff (14.9 KB) - added by swissspidy 9 years ago.
34561.3.diff (15.4 KB) - added by swissspidy 9 years ago.
34561.4.diff (8.2 KB) - added by juliobox 9 years ago.
34561.5.diff (15.7 KB) - added by swissspidy 9 years ago.
34561.6.diff (17.2 KB) - added by Nikschavan 9 years ago.
Renames the_excerpt_embed() to the_embed_excerpt() and the_excerpt_embed to embed_the_excerpt after 34561.5.diff
34561.7.diff (7.6 KB) - added by DrewAPicture 9 years ago.
34561.8.diff (9.9 KB) - added by swissspidy 9 years ago.
34561.9.patch (16.0 KB) - added by imath 9 years ago.
34561.9.diff (15.8 KB) - added by swissspidy 9 years ago.
34561.10.diff (36.7 KB) - added by DrewAPicture 9 years ago.
MVP

Download all attachments as: .zip

Change History (59)

@swissspidy
9 years ago

#1 @swissspidy
9 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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?

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

#2 @swissspidy
9 years ago

  • Milestone changed from Future Release to 4.4

@swissspidy
9 years ago

#3 @swissspidy
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: @juliobox
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 @swissspidy
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?

@swissspidy
9 years ago

#6 @swissspidy
9 years ago

34561.3.diff is a refreshed patch against trunk.

#7 @juliobox
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.

@juliobox
9 years ago

@swissspidy
9 years ago

#8 @swissspidy
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 @Nikschavan
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.

Ref. ticket:34272#comment:7

@Nikschavan
9 years ago

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

#10 @juliobox
9 years ago

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 ;) )

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

#11 @Nikschavan
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 @swissspidy
9 years ago

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.

#13 @juliobox
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.

#14 @DrewAPicture
9 years ago

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

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


9 years ago

@DrewAPicture
9 years ago

#16 @DrewAPicture
9 years ago

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.

#17 @DrewAPicture
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 @swissspidy
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 @swissspidy
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 @Otto42
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.

Last edited 9 years ago by Otto42 (previous) (diff)

@swissspidy
9 years ago

#21 @swissspidy
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), 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.

#22 @imath
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 ;)

#23 @DrewAPicture
9 years ago

  • Owner DrewAPicture deleted

#24 @ChriCo
9 years ago

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

#25 @imath
9 years ago

@swissspidy

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

@imath
9 years ago

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


9 years ago

#27 @chriscct7
9 years ago

  • Owner set to chriscct7
  • Status changed from reviewing to accepted

#28 @chriscct7
9 years ago

  • Owner chriscct7 deleted
  • Status changed from accepted to assigned

#29 @DrewAPicture
9 years ago

  • Owner set to DrewAPicture

#30 @swissspidy
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.

@swissspidy
9 years ago

#31 @swissspidy
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 @swissspidy
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

#34 @jorbin
9 years ago

  • Keywords commit added

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

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


9 years ago

#36 @DrewAPicture
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.

@DrewAPicture
9 years ago

MVP

#37 @ocean90
9 years ago

  • Type changed from enhancement to task (blessed)

#38 @DrewAPicture
9 years ago

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.

#39 @DrewAPicture
9 years ago

In 36694:

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

Props ocean90
See #34561.

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


9 years ago

#41 @DrewAPicture
9 years ago

In 36746:

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

See #34561.

#42 @swissspidy
9 years ago

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.

#43 @swissspidy
9 years ago

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

#44 @ocean90
9 years ago

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

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

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

#45 @DrewAPicture
9 years ago

In 36923:

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

See #34561. See #35986.

#46 @DrewAPicture
9 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.

#47 @DrewAPicture
9 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.

#48 @SergeyBiryukov
9 years ago

In 37232:

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

Props Frozzare.
Fixes #34561. See #34561.

Version 1, edited 9 years ago by SergeyBiryukov (previous) (next) (diff)
Note: See TracTickets for help on using tickets.