Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#40935 closed defect (bug) (fixed)

Facebook Video Works On Preview But Not On Theme

Reported by: emanweb Owned by: westonruter
Milestone: 4.8.1 Priority: normal
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch fixed-major
Focuses: Cc:


I am using a vanila Twenty Seventeen with 4.8RC2 and once I add a Facebook video using widget it shows up in the preview but not after adding it. See screenshots below:

Enviromments tested:
WP: 4.8-RC2-40875
Nginx: 1.10.1
PHP 7.1.4
MySQL 5.6.34

WP: 4.8-RC2-40875
Nginx: 1.10.1
PHP 7.0.3
MySQL 5.6.34

WP: 4.8-RC2-40875
Apache: 2.4.10
PHP 7.0.3
MySQL 5.6.34

Video Preview Works Fine

Once Added Video URL It Shows This

Then in the front end:

p.s.: This is my first time reporting a bug, so please excuse me if I did it the wrong way.

Attachments (1)

40935.1.diff (634 bytes) - added by timmydcrawford 4 years ago.

Download all attachments as: .zip

Change History (14)

#1 @westonruter
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.8.1

This is a bug. Only YouTube and Vimeo are supposed to have been acceptable.

The video widget somehow needs to extend the fetch logic to take into account isHostedVideo, perhaps. There could perhaps be a new MediaWidgetControl#isAllowedUrl() which by default returns true, or rather it should include the specific logic in the fetch method. Then the video widget specifically can extend that isAllowedUrl method to take into account isHostedVideo. Since oEmbed is not supported by the audio widget at all, then it should bail entirely if the URL is not for an audio file.

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

4 years ago

#3 @westonruter
4 years ago

Alternatively, it would be more ideal to allow for arbitrary oEmbed video or audio to be referenced. To do that, we'd need #34115 resolved, and then the widget would need to switch between ME.js rendering and oEmbed rendering, depending on the URL provided.

#4 follow-up: @fierevere
4 years ago

I got same problem with frontend when using direct link to .mp4

( https://myhost/file.mp4 )

It works fine with the backend, all the time, but shows as link on frontend


Clean WP, twenty seventeen theme (updated)

#5 in reply to: ↑ 4 @westonruter
4 years ago

Replying to fierevere:

I got same problem with frontend when using direct link to .mp4

Thanks. That's actually a different issue. See #40977.

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

4 years ago

#7 @timmydcrawford
4 years ago

  • Keywords has-patch added; needs-patch removed

I spent some time exploring the options here - ideally like @westonruter said above, the most ideal solution would be to respect all whitelisted embed providers, like Facebook, in this scenario. As he mentioned, we do need #34115 resolved to proceed there since we can not use any of the embed shortcode logic without a $post. I would go a bit further and say to fully leverage oembeds, we should explore a way to add mime types to the provider list ( https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-oembed.php#L57 ) - so we could properly determine which embeds would work in which context ( Audio vs Video widget ).

Ultimately though, this kind of brings me back to the notion of a generic oembed widget. When we do have the ability to use embeds with a post, perhaps when a user pastes in an approved embed URL, the widget automatically changes from a Video or an Audio widget to an embed widget.

For the time-being though, we are still restricted by the $post_ID requirement ( https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-embed.php#L193 ) in the embed shortcode logic. If this were not there, we could add logic to the widget rendering that detects an embeddable URL, and returns the embed instead of the wp_video_shortcode logic. As such we are still currently limited by what mejs supports for playback which is Vimeo and YouTube.

40935.1.diff extends the existing validation logic in media-widgets.js to ensure the URL inserted in the modal is Vimeo or YouTube before fetching the oembed/proxy. This allows a URL to an external .mp4 file to be used still, but also ensures that only the truly supported external providers can be used.

#8 @westonruter
4 years ago

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

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.

#9 @westonruter
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for consideration in 4.8.1.

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

4 years ago

#11 @westonruter
4 years ago

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

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.

#12 @westonruter
4 years ago

In 41651:

Embeds: Cache oEmbeds in an oembed_cache custom post type instead of postmeta when there is no global $post.

Add processing of embeds to rich Text widget.

Props swissspidy, westonruter, ocean90, johnbillion.
See #40854, #39994, #40935.
Fixes #34115.

#13 @westonruter
3 years ago

In 41933:

Widgets: Harden logic for checking for valid media URLs.

  • Update deprecated isHostedVideo method to always return true since all oEmbeds are now supported.
  • Disable scanImage for non-image media widgets.
  • Ensure embed URL field element is matched from current media frame instance due to bug where media frames are not destroyed upon closing.
  • Update error message for invalid video URLs to ask user to check the URL for validity.

Props gk.loveweb, octalmage, westonruter.
See #42039, #40935.
Fixes #41052.

Note: See TracTickets for help on using tickets.