Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#34115 closed enhancement (fixed)

oEmbed not working on author page without posts

Reported by: dannydehaan's profile dannydehaan Owned by: swissspidy's profile swissspidy
Milestone: 4.9 Priority: high
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch has-unit-tests commit
Focuses: template, performance Cc:

Description

Hi All.

We're working on a project for a client at the moment. It's a content publishing platform. They have multiple authors. all with their own detail page. At the profile edit page, they can fill their biography, they can also insert a youtube link.

At the front-end, we disply the biography as:

<?php echo apply_filters( 'the_content', get_the_author_meta( 'description' ) ); ?>

The strange thing is, that when they have authored one post, the oEmbed class does the job perfectly, but, when they don't have authored any post, the oEmbed doesn't work.

I've looked inside the core and that led to the file "wp-includes/class-wp-embed.php|" at line 181. It checks if there's a post, when not, it's not embedding.

When i delete this line, it works for authors without and with posts.

Attachments (3)

34115.patch (6.8 KB) - added by dannydehaan 8 years ago.
Work in Progress Patch file
34115.diff (17.2 KB) - added by swissspidy 8 years ago.
34115.2.diff (16.2 KB) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (48)

#1 @johnbillion
8 years ago

  • Component changed from Media to Embeds

Thanks for the report, Danny.

The post is used to cache the oEmbed response (in post meta) so it doesn't have to be fetched from the oEmbed provider on every call.

It looks like a filter or a fallback to a transient could be an option here.

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

#3 @dannydehaan
8 years ago

I've worked on this ticket a little bit and changed the cache to transients instead of post_meta. In my opinion this would be better because it saves space when a video is used on multiple pages.

I've attached the Work in Progress patch. Can you guys tell me if this could be something like a fix?

@dannydehaan
8 years ago

Work in Progress Patch file

#4 @johnbillion
8 years ago

  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement
  • Version changed from 4.3.1 to 2.9

oEmbeds in posts are cached in post meta because transients are just that, transient. If the transient cache is flushed (or becomes full and begins purging) then there's the potential for a cache stampede if an affected post is getting a lot of traffic.

I'd probably support a fallback mechanism for caching non-post oEmbeds in a transient, but the caching for posts cannot be changed.

#5 @swissspidy
8 years ago

#33917 was marked as a duplicate.

@swissspidy
8 years ago

#6 @swissspidy
8 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Milestone changed from Awaiting Review to Future Release

I spent the afternoon writing tests for WP_Embed and implement a transient fallback afterwards. The result is 34115.diff.

Nothing spectacular there, except that I added a pre_oembed_result filter to test wp_oembed_get() without doing any HTTP requests.

#7 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.6

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


8 years ago

#9 @chriscct7
8 years ago

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

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


8 years ago

#11 @swissspidy
8 years ago

@johnbillion Any chance I could get your feedback on my approach with the transient fallback?

Happy to create a separate patch only containing tests for the current implementation, too.

@swissspidy
8 years ago

#12 @swissspidy
8 years ago

34115.2.diff makes the patch apply cleanly again.

Need some additional feedback, otherwise it is good to go.

#13 @ocean90
8 years ago

Using transients was previously discussed in #14759 and #17210. The comment by @Viper007Bond ticket:14759:18 lists a few reasons why transients are a bad idea. We should take this into consideration even if the ticket is only about cases where no post ID is available.

I'd like to hear what @nacin, @helen or @markjaquith think about this.

#14 @swissspidy
8 years ago

Understandable, would love to hear your feedback! For the time being, we could split the patch up and only commit the tests for the current implementation for now. Or even anything but the get_transient() / set_transient() lines. Would perhaps make things a bit easier to read.

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


8 years ago

#16 @swissspidy
8 years ago

In 37892:

Embeds: Add tests for the WP_Embed class.

Fixes #37214. See #34115.

#17 @ocean90
8 years ago

  • Milestone changed from 4.6 to Future Release

#18 @swissspidy
7 years ago

  • Keywords needs-refresh added

Needs a new patch without the tests that were added in [37892].

#19 @westonruter
7 years ago

Since transients seem to be unfavorable, what if we introduced a new private post type specifically for the purpose of caching oEmbeds? This would allow us to fetch and cache oEmbeds without having a post as context, and it would then also allow for multiple posts to re-use the same oEmbed cache.

This global could also be employed the proposed oEmbed proxy endpoint: #40450.

This ticket was mentioned in Slack in #core-media by westonruter. View the logs.


7 years ago

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


7 years ago

#22 @westonruter
7 years ago

  • Milestone changed from Future Release to 4.8.1

This is a dependency for adding oEmbeds to the Text widget: #40854

#23 @swissspidy
7 years ago

#40926 was marked as a duplicate.

This ticket was mentioned in Slack in #core-customize by timmyc. View the logs.


7 years ago

#25 @westonruter
7 years ago

In 40939:

Widgets: Forcibly limit Video widget to only accept oEmbed URLs from YouTube and Vimeo (for now).

Amends [40640].
Props timmydcrawford.
See #34115, #39994.
Fixes #40935.

#26 @swissspidy
7 years ago

Using a post type could work. Not only could posts share the cached data, we also don't need the post meta anymore at all. Of course we'd still fetch from post meta for old posts.

How would we delete cached oEmbed responses when they're not used anymore?

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


7 years ago

#28 @westonruter
7 years ago

@swissspidy There could simply be a cron that deletes posts of this type with a certain age at whatever internal we specify as the TTL, similar to auto-draft and trash garbage collection.

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


7 years ago

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


7 years ago

#31 @westonruter
7 years ago

  • Milestone changed from 4.8.1 to 4.9

#32 @westonruter
7 years ago

In 41056:

Widgets: Forcibly limit Video widget to only accept oEmbed URLs from YouTube and Vimeo (for now).

Merges [40939] onto 4.8 branch.
Amends [40640].
Props timmydcrawford.
See #34115, #39994.
Fixes #40935 for 4.8.1.

#33 @westonruter
7 years ago

In 41361:

Widgets: Add shortcode support inside Text widgets.

  • Used now in core to facilitate displaying inserted media. See #40854.
  • The [embed] shortcode is not supported because there is no post context for caching oEmbed responses. This depends on #34115.
  • Add do_shortcode() to the widget_text_content filter in the same way it is added for the_content at priority 11, with shortcode_unautop() called at priority 10 after wpautop().
  • For Text widget in legacy mode, manually apply do_shortcode() (and shortcode_unautop() if auto-paragraph checked) if the core-added widget_text_content filter remains, unless a plugin added do_shortcode() to widget_text to prevent applying shortcodes twice.
  • Ensure that global $post is null while filters apply in the Text widget so shortcode handlers won't run with unexpected contexts.

Props westonruter, nacin, aaroncampbell.
See #40854, #34115.
Fixes #10457.

#34 @westonruter
7 years ago

With [41361] the only thing holding us back from adding embeds to Text widgets is (1) caching oEmbed responses somewhere else than postmeta (this ticket), and (2) incorporating WP_Embed::run_shortcode() logic into WP_Widget_Text::widget().

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#36 @westonruter
7 years ago

Good progress by @swissspidy on a patch. Anyone, please review: https://github.com/xwp/wordpress-develop/pull/254/files

#37 @westonruter
7 years ago

  • Focuses performance added

Copying the latest comment from the PR here for visibility:

It would be great if we could migrate post meta caches over to the post type instead of bumping the cache time there.

I think it's going to be more performant to continue using the postmeta cache by default. Consider having an index page for videos where you are showing 30 posts, each of which has an oEmbed. When postmeta caches are used, when the WP_Query is made, WordPress will update post caches of the postmeta for _all_ of the queried posts at once via update_post_caches().

If, however, oEmbed caches are stored in a custom post type, then these will not be able to be fetched en-mass and a separate DB query will result for every oEmbed in every post.

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


6 years ago

#39 @westonruter
6 years ago

  • Priority changed from normal to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

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


6 years ago

#41 @westonruter
6 years ago

  • Keywords commit added; needs-refresh removed

#42 @westonruter
6 years ago

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

In 41651:

Embeds: Cache oEmbeds in an oembed_cache custom post type instead of postmeta when there is no global $post.

Add processing of embeds to rich Text widget.

Props swissspidy, westonruter, ocean90, johnbillion.
See #40854, #39994, #40935.
Fixes #34115.

#43 @westonruter
6 years ago

In 41913:

Widgets: Fix previewing embeds in Text widget by allowing parse-embed admin ajax requests with an empty post_ID just as WP_oEmbed_Controller::get_proxy_item_permissions_check() allows.

As of #34115 if there is no post context the oEmbed will be cached in an oembed_cache custom post type, so having a post as context is no longer a requirement for caching.

Props biskobe, westonruter.
See #34115, #40450.
Fixes #40854.

#44 @westonruter
6 years ago

In 41985:

Editor: Specify maxwidth in parse-embed requests based on width of editor iframe so that TinyMCE view embeds fit, particularly in Text widgets.

See #40854, #34115.

#45 @westonruter
6 years ago

In 42009:

Embeds: Improve consistency of update and refresh logic for oEmbed caching between oembed_cache and post meta.

  • Allow updating oEmbed cache during parse-embed requests for non-post editors (such as widgets).
  • Update any existing oembed_cache post when usecache and TTL has passed.
  • Do not overwrite a previously valid cache with {{unknown}}.

Props dlh.
See #34115.
Fixes #42310.

Note: See TracTickets for help on using tickets.