WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 29 hours ago

#34115 assigned enhancement

oEmbed not working on author page without posts

Reported by: dannydehaan Owned by: swissspidy
Milestone: 4.9 Priority: high
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch has-unit-tests needs-refresh
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 2 years ago.
Work in Progress Patch file
34115.diff (17.2 KB) - added by swissspidy 19 months ago.
34115.2.diff (16.2 KB) - added by swissspidy 15 months ago.

Download all attachments as: .zip

Change History (42)

#1 @johnbillion
2 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 2 years ago by johnbillion (previous) (diff)

#3 @dannydehaan
2 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
2 years ago

Work in Progress Patch file

#4 @johnbillion
2 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
2 years ago

#33917 was marked as a duplicate.

@swissspidy
19 months ago

#6 @swissspidy
19 months 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
17 months ago

  • Milestone changed from Future Release to 4.6

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


16 months ago

#9 @chriscct7
16 months ago

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

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


16 months ago

#11 @swissspidy
16 months 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
15 months ago

#12 @swissspidy
15 months ago

34115.2.diff makes the patch apply cleanly again.

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

#13 @ocean90
15 months 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
15 months 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.


15 months ago

#16 @swissspidy
15 months ago

In 37892:

Embeds: Add tests for the WP_Embed class.

Fixes #37214. See #34115.

#17 @ocean90
15 months ago

  • Milestone changed from 4.6 to Future Release

#18 @swissspidy
12 months ago

  • Keywords needs-refresh added

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

#19 @westonruter
5 months 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.


5 months ago

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


4 months ago

#22 @westonruter
4 months ago

  • Milestone changed from Future Release to 4.8.1

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

#23 @swissspidy
4 months ago

#40926 was marked as a duplicate.

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


3 months ago

#25 @westonruter
3 months 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
3 months 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.


2 months ago

#28 @westonruter
2 months 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.


2 months ago

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


2 months ago

#31 @westonruter
2 months ago

  • Milestone changed from 4.8.1 to 4.9

#32 @westonruter
2 months 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
12 days 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
12 days 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.


10 days ago

#36 @westonruter
8 days ago

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

#37 @westonruter
6 days 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.


31 hours ago

#39 @westonruter
29 hours 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.

Note: See TracTickets for help on using tickets.