Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#34272 closed task (blessed) (fixed)

Audit embed function names

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

Description

In [34903] a number of functions relating to core's oEmbed implementation were introduced. Some of these functions use oembed in their name, some use embed in their name.

The following functions should be audited to determine whether any need to be renamed.

  • get_post_embed_url()
  • get_oembed_endpoint_url()
  • get_post_embed_html()
  • wp_oembed_excerpt_more()
  • the_excerpt_embed()
  • wp_oembed_excerpt_attachment()

Attachments (4)

34272.diff (27.7 KB) - added by swissspidy 8 years ago.
34272.2.diff (964 bytes) - added by peterwilsoncc 8 years ago.
34272.3.diff (1.7 KB) - added by peterwilsoncc 8 years ago.
34272.4.diff (1.1 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (21)

#1 @johnbillion
8 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

@swissspidy are you able to take a look?

#2 @swissspidy
8 years ago

The /embed/ rewrite endpoint isn't really dependant on oEmbed. That's why naming get_post_embed_url(), get_post_embed_html() and the_excerpt_embed() like that makes sense.

I suggest renaming wp_oembed_excerpt_more() and wp_oembed_excerpt_attachment() to use a wp_embed prefix (or perhaps just embed). There are also print_oembed_embed_styles() and print_oembed_embed_scripts() which could be renamed to print_embed_styles() and print_embed_scripts().

In the embed template, there are hooks like oembed_head and oembed_footer. From what I said above, we should consider using embed_head and embed_footer there.

An alternative approach is to just use oembed everywhere, if that's not too confusing.

#3 @johnbillion
8 years ago

  • Keywords needs-patch added; dev-feedback removed

My preference is to favour embed over oembed when the functionality isn't specific to oEmbed. I think all of the above can use embed instead of oembed.

#4 @johnbillion
8 years ago

The get_post_embed_html() function includes some JavaScript from js/wp-oembed.js. Is this JavaScript oEmbed-specific? If so, is get_post_embed_html() oEmbed-specific?

#5 @swissspidy
8 years ago

My preference is to favour embed over oembed when the functionality isn't specific to oEmbed.

Likewise.

If so, is get_post_embed_html() oEmbed-specific?

No. It can be used without having an oEmbed endpoint at all. Since it includes the wp-oembed.js, both aren't 100% oEmbed-specific.

@swissspidy
8 years ago

#6 @swissspidy
8 years ago

  • Keywords has-patch added; needs-patch removed

34272.diff renames files and functions. This makes it more clear what is oEmbed-specific and what isn't.

The renamed files need to be moved with svn cp when committing though.

#7 @DrewAPicture
8 years ago

  • Keywords commit added

For reference:

= Files =
wp-includes/css/wp-oembed-embed.css => wp-includes/css/wp-embed-template.css
wp-includes/js/wp-oembed-embed.js   => wp-includes/js/wp-embed-template.js
wp-includes/js/wp-oembed.js         => wp-includes/js/wp-embed.js

= Functions =
print_oembed_embed_scripts()   => print_embed_scripts()
wp_oembed_excerpt_more()       => wp_embed_excerpt_more()
wp_oembed_excerpt_attachment() => wp_embed_excerpt_attachment()
print_oembed_embed_styles()    => print_embed_styles()
print_oembed_embed_scripts()   => print_embed_scripts()

= Hooks =
oembed_html                  => embed_html
oembed_head                  => embed_head
oembed_thumbnail_image_size  => embed_thumbnail_image_size
oembed_thumbnail_image_shape => embed_thumbnail_image_shape
oembed_content               => embed_content
oembed_site_icon_url         => embed_site_icon_url
oembed_content_meta          => embed_content_meta
oembed_site_icon_url         => embed_site_icon_url
oembed_footer                => embed_footer

= Tests =
test_wp_oembed_excerpt_more_no_embed() => test_wp_embed_excerpt_more_no_embed()
test_wp_oembed_excerpt_more()          => test_wp_embed_excerpt_more()
Last edited 8 years ago by DrewAPicture (previous) (diff)

#8 @peterwilsoncc
8 years ago

If this is committed prior to #34204, #34204 will require a refresh.

#9 @SergeyBiryukov
8 years ago

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

In 35235:

Embeds: Rename files, functions, and hooks added in [34903] to make it more clear what is oEmbed-specific and what isn't.

See https://core.trac.wordpress.org/ticket/34272#comment:7 for full list of renamed functions and hooks.

Props swissspidy.
Fixes #34272.

#10 @peterwilsoncc
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In 34272.2.diff, fix for enqueueing and printing wp-embed.js missed in original commit.

#11 @swissspidy
8 years ago

Good catch! Seems like I missed that in the patch.

#12 @peterwilsoncc
8 years ago

  • Keywords needs-refresh added; commit removed

Running a quick refresh, I missed oembed_foot => embed_foot on L459 in default filters.php?

#13 @peterwilsoncc
8 years ago

  • Keywords needs-refresh removed

#14 @SergeyBiryukov
8 years ago

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

In 35253:

Embeds: After [35235], replace some missed oembed references with embed.

Props peterwilsoncc.
Fixes #34272.

#15 @johnbillion
8 years ago

In 35255:

Correct a test after r35253.

See #34272

@swissspidy
8 years ago

#16 @swissspidy
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

The TinyMCE wpoembed plugin should be renamed to wpembed as well (including the folder name). See 34272.4.diff .

#17 @SergeyBiryukov
8 years ago

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

In 35397:

Embeds: Rename TinyMCE wpoembed plugin to wpembed.

Props swissspidy.
Fixes #34272.

Note: See TracTickets for help on using tickets.