Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#42310 closed defect (bug) (fixed)

Inconsistent update and refresh logic between `oembed_cache` and post meta

Reported by: dlh's profile dlh Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Embeds Keywords: has-patch needs-testing
Focuses: Cc:

Description

The logic for caching oEmbeds in the oembed_cache post type seems to have a couple of inconsistencies with the existing logic for caching oEmbeds in post meta.

First, there isn't logic in WP_Embed::shortcode() to update to an existing oembed_cache post. If WP_Embed::usecache is false and the embed's TTL has passed, then WP_Embed::shortcode() will insert a new oembed_cache post even when a $cached_post_id exists. By contrast, for post meta, any existing _oembed_ and _oembed_time_ values are updated.

One part of the attached patch adds logic to use wp_update_post() when there is a $cached_post_id. Per [28972], one of the goals of the patch is also to not overwrite a previously valid cache with {{unknown}}.

Second, there isn't logic to refresh caches of oEmbed in widgets after saving the widget. By contrast, when a post is saved in the admin, WP_Embed::maybe_run_ajax_cache() will print JavaScript on the next pageload that refreshes the embeds in the post content via the oembed-cache Ajax action.

This inconsistency seems a little tougher to rectify because widgets are already saved over Ajax. They're also saved using different mechanisms on widgets.php and customize.php. However, it's arguably more important to refresh oembed_cache embeds because they're saved "globally" instead of specifically to a single post. If an oEmbed fails in one widget, it will fail for all future widgets.

The attached patch would address this issue in wp_ajax_parse_embed(). It would set WP_Embed::usecache to false when there is no post ID (which is the case with embeds in widgets). Effectively, an embed whose TTL had passed would be refreshed as part of fetching it to display in the widget form.

This approach has the advantage of occurring automatically for users once they open a widget form on either widgets.php or customize.php. But as @adamsilverstein pointed out to me at WordCamp NYC today, it breaks with existing user expectations in that saving the widget wouldn't itself cause anything to refresh, unlike the current behavior with posts.

In that sense, methods for refreshing oembed_cache posts after saving changes on widgets.php or customize.php might be preferable.

Attachments (1)

42310.diff (3.6 KB) - added by dlh 7 years ago.

Download all attachments as: .zip

Change History (7)

@dlh
7 years ago

#1 @johnbillion
7 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.9

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 melchoyce. View the logs.


7 years ago

#4 @westonruter
7 years ago

@dlh:

In that sense, methods for refreshing oembed_cache posts after saving changes on widgets.php or customize.php might be preferable.

This is a challenge. While this could maybe align with user expectations for embeds in widgets specifically, it wouldn't then help with using embeds in TinyMCE outside of the widget context. For example, if I add a TinyMCE editor to the user profile screen and I want to drop an embed in there, we wouldn't be able to reliably know when to refresh the oEmbed other than there also attempt to manually add a refresh when user updated action fires. So I think it is useful to piggyback on the parse-embed request to also refresh the cache.

#5 @westonruter
7 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

#6 @westonruter
7 years ago

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

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.