Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#28639 closed defect (bug) (fixed)

WP_Embed::cache_oembed() only works for posts and pages by default

Reported by: helen Owned by: SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version:
Component: Embeds Keywords: has-patch commit
Focuses: Cc:


WP_Embed::cache_oembed() runs an embed_cache_oembed_types filter on an array of post types that just contains post and page by default. If you're in a CPT, for instance, the async call for oEmbed caching will bail early. This is pretty non-intuitive, and it's unclear to me what the benefit of limiting the post types is.

Attachments (4)

28639.1.diff (1.2 KB) - added by bordoni 7 years ago.
Solution number 1
28639.2.diff (1.9 KB) - added by bordoni 7 years ago.
Solution number 2
28639.diff (1.9 KB) - added by helen 7 years ago.
28639.3.diff (1.2 KB) - added by SergeyBiryukov 7 years ago.

Download all attachments as: .zip

Change History (11)

#1 follow-up: @helen
7 years ago

I would recommend just killing that check entirely - a quick search of plugins turned up no results for the hook. If we want to keep the hook, then we should pass all registered post types and let it become an unset instead. Since this is only called in core by wp_ajax_oembed_cache(), it would only happen for post types with an edit screen by default. If somebody is calling this manually, I hope they are passing the appropriate post ID.

If we're concerned about revisions, we should check if it's a revision rather than artificially limiting the post types, and update_post_meta() already handles that anyway.

Interested in any recollection anyone has over why this was done this way. Asked Viper007Bond, who doesn't remember anything specific. He did note that there was this comment in the original commit ([12023]): // post_type check is incase of "save_post" usage

#2 in reply to: ↑ 1 @bordoni
7 years ago

  • Keywords reporter-feedback has-patch dev-feedback added

Replying to helen:
There is 2 ways of doing it:

  1. As you said explicitly excluding the revision post type from the caching
  2. Unsetting revision by applying a filter to the embed_cache_oembed_types by default

Personally I like the second one better because if someone wants to do caching for revisions for some weird reason it could be done.

Anyways both of the possibilities diff are attached.

7 years ago

Solution number 1

7 years ago

Solution number 2

#3 @johnbillion
7 years ago

  • Keywords reporter-feedback dev-feedback removed
  • Milestone changed from Awaiting Review to 4.0

Just to clarify, this only affects the asynchronous pre-caching that's triggered on the post editing screen.

Let's go with bordoni's second approach but limit the post types to those with show_ui set to true.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

7 years ago

7 years ago

#5 @helen
7 years ago

  • Keywords commit added

Addressed show_ui => true in 28639.diff.

#6 @SergeyBiryukov
7 years ago

If we're going with show_ui => true, we don't need _wp_exclude_revision_oembed_cache().

#7 @SergeyBiryukov
7 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 29557:

Don't limit WP_Embed::cache_oembed() to posts and pages.

props bordoni, helen.
fixes #28639.

Note: See TracTickets for help on using tickets.