Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#28639 closed defect (bug) (fixed)

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

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

Description

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 10 years ago.
Solution number 1
28639.2.diff (1.9 KB) - added by bordoni 10 years ago.
Solution number 2
28639.diff (1.9 KB) - added by helen 10 years ago.
28639.3.diff (1.2 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (11)

#1 follow-up: @helen
10 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
10 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.

@bordoni
10 years ago

Solution number 1

@bordoni
10 years ago

Solution number 2

#3 @johnbillion
10 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.


10 years ago

@helen
10 years ago

#5 @helen
10 years ago

  • Keywords commit added

Addressed show_ui => true in 28639.diff.

#6 @SergeyBiryukov
10 years ago

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

#7 @SergeyBiryukov
10 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.