WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 22 months ago

Last modified 20 months ago

#21719 closed task (blessed) (fixed)

Remove autoembed_urls, embed_size_w, embed_size_h

Reported by: nacin Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch
Focuses: Cc:

Description

Per UI discussion on Tuesday, the oEmbed settings should all disappear:

  • autoembed_urls, the on-off checkbox, goes way, and oEmbed is always assumed to be on. The only reason to have a UI on/off for oEmbed is if it was easy to accidentally embed items. But it doesn't parse every link in the post, just those on their own line or inside an [embed] code.

add_filter( 'default_option_autoembed_urls', '__return_true' ); could handle anyone trying to fetch autoembed_urls in a plugin to try to follow core.

  • Width is now determined solely by $content_width, and otherwise is assumed to be 500. See wp_embed_defaults(), where there is already a filter.
  • Height is assumed to be something, though not sure what yet. Not sure what yet. wp_embed_defaults() thinks it should be 700, while populate_options() thinks it should be 600. I'm thinking we do something more like min( $content_width * x, y ), where x is somewhere between 2 and 3, and y is north of 600, probably 1000.

A few reminders for height and width. One, these are defaults only: the [embed] shortcode accepts 'height' and 'width' manually. Two, while the provider is "required" to follow the spec, it is based on trust. I brought up Storify as a good example of an oEmbed endpoint (which, hey, they don't actually have) that should deliberately ignore maxheight, because of how it works.

Clearly, height is not a slam-dunk, but we should remove it anyway. Why? Because you likely got confused when trying to understand the inheritance and manual overrides I outlined above (I did). Now imagine a user trying to understand "Maximum embed size" and "If the width value is left blank, embeds will default to the max width of your theme." They're edge enough to be best served under the hood.

Attachments (1)

remove-auto-embeds.diff (8.9 KB) - added by wonderboymusic 22 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 obenland23 months ago

Is this different to #21508?

comment:2 wonderboymusic22 months ago

I withdraw my objection - 'embed_defaults' is a more obvious way to accomplish the same thing

Last edited 22 months ago by wonderboymusic (previous) (diff)

comment:3 wonderboymusic22 months ago

  • Keywords needs-patch added

comment:4 wonderboymusic22 months ago

  • Keywords has-patch added; needs-patch removed

Brute force removal and literal interpretation of Nacin's comments - may need caucus on default values.

comment:5 wonderboymusic22 months ago

#21508 was marked as a duplicate.

comment:6 nacin22 months ago

Okay, not saying we shouldn't remove the the width/height fields as well, but here's a first take that only removes autoembed_urls: http://cl.ly/image/1b1I2O0g1f31.

comment:7 nacin22 months ago

remove-auto-embeds.diff looks good.

I am debating as to whether we should still listen to existing options, even while hiding the UI (and thus preventing those options from being changed); or not hiding the UI if the existing options are set to a non-default value; or ignoring all options and clearing them out, as remove-auto-embeds.diff does.

I think for 3.5 beta 1, we should implement this minus the schema change to actually clear them out of the DB, in case we find the desire to roll them back. We could also consider leaving them in the DB in case anyone is using them directly for some esoteric reason, though the default filters are a nice touch. (We could also return wp_embed_defaults() through those option calls, too.)

comment:8 nacin22 months ago

Oh, also, add_filter( 'default_option_embed_size_w', 500 ); wouldn't work.

comment:9 nacin22 months ago

In [21998]:

Always attempt to embed URLs in content, removing the Auto-embeds (autoembed_urls) option.

Remove the UI for setting the default width and height for embeds. Width was confusing as it
was blank by default (inheriting the content width from the theme, or 500px). The height is
now calculated as 1.5x the content width, or 1000px, whichever is smaller.

The [embed] shortcode can still receive manual height and width attributes. This just removes
the global settings.

props wonderboymusic. see #21719.

comment:10 nacin22 months ago

  • Type changed from enhancement to task (blessed)

Okay. Let's see how people react to [21998].

comment:11 nacin22 months ago

In [22070]:

Fix option name. props ocean90, see #21719.

comment:12 nacin22 months ago

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

comment:13 dangayle20 months ago

  • Cc dangayle@… added

I don't like this at all, although I am in favor of smart defaults in general. Why even HAVE a media settings page, if you're not going to have any settings to set?

I think it is a mistake to remove max width settings from the media settings page. If you're going to remove that, you should also remove the thumbnail settings, which are WAY more problematic to themes.

It also breaks the sites of anyone who WAS using the setting when they upgrade.

Note: See TracTickets for help on using tickets.