Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#40808 closed defect (bug) (fixed)

Media Widgets: VideoPress Uploads Not Functioning Correctly

Reported by: timmydcrawford's profile timmydcrawford Owned by: westonruter's profile westonruter
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch has-unit-tests commit
Focuses: javascript, administration Cc:

Description

With the media widgets now merged in for 4.8, I'm opting to open this issue here and close out the corresponding legacy issue at https://github.com/xwp/wp-core-media-widgets/issues/160

Original Issue Report


Steps
1- Install Jetpack and a plan that includes VideoPress
2- Via the customizer, create a new Video Widget
3- Upload a new video to the library, note the preview in the modal will show a Jetpack Video Processing image
4- It seems after clicking Add to Widget, the media items post URL is returned and set on the widget instance, as such the preview in the widget and in the customizer will not work

Alternatively, uploading a video via the Media Library directly, then selecting it within a Video Widget from the library works fine.


Essentially what is happening is the attachment page URL is being returned from the media modal after an upload has been initiated on video files via VideoPress. The Video Widget attempts to use the attachment URL as a source for a video tag and in the end the URL is rendered in the widget instead of a video player.

Two issues need to be addressed here to allow for plugins that handle transcoding off-site to work properly:

1- A way, or "best practice" at least to over-ride the URL used within the widget preview.
2- Determine the best way to grab the correct "final" URL to be used in the front-end widget. It seems like this should be working since the render_media for the Video Widget uses wp_get_attachment_url which JP hooks into, but it currently is not working properly.

Attachments (3)

Redemption_via_JSXHint_–_haiku2.png (15.8 KB) - added by timmydcrawford 7 years ago.
Video Widget Output From VideoPress Upload
40808.1.diff (2.3 KB) - added by timmydcrawford 7 years ago.
40808.2.diff (2.6 KB) - added by westonruter 7 years ago.
Fix jshint issue (remove unused var) and fix jshint config in grunt to check file

Download all attachments as: .zip

Change History (16)

#1 follow-up: @westonruter
7 years ago

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

I think perhaps a more underlying issue is adding support for more oEmbeds than just YouTube and Vimeo, as VideoPress is also an oEmbed. So if at the point of media selection we can then make sure that the widget instance has a url that is the oEmbed URL as opposed to the faux file URL, then we can enter into the is_hosted_video condition. If the URL provided doesn't look like a file URL, then we could at that point we could assume that the URL is for an oEmbed fire off an oEmbed proxy request to obtain the HTML for rendering the player and write it into an iframe.

Another piece to consider is the error message when providing a embed URL. Let's say you try to add a video by supplying the oEmbed URL itself: https://videopress.com/v/OAVHZIvv This then should not be rejected. In order to do that we should be exporting all of the \WP_oEmbed::$providers to the client so that we can check against them instead of statically only looking for YouTube or Vimeo as we do with hard-coding right now.

See https://github.com/xwp/wp-core-media-widgets/issues/160#issuecomment-301678111

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

Replying to westonruter:

Another piece to consider is the error message when providing a embed URL. Let's say you try to add a video by supplying the oEmbed URL itself: https://videopress.com/v/OAVHZIvv This then should not be rejected. In order to do that we should be exporting all of the \WP_oEmbed::$providers to the client so that we can check against them instead of statically only looking for YouTube or Vimeo as we do with hard-coding right now.

Could we instead rely upon oembed/proxy to validate this for us? If the url doesn't contain an extension, just use the new endpoint and see if it gets a valid response back - which utilizes the whitelist itself.

#3 @westonruter
7 years ago

@timmydcrawford excellent point. This would just mean the error message would be shown asynchronously after the response returns, but I think that is fine.

@timmydcrawford
7 years ago

Video Widget Output From VideoPress Upload

#4 @timmydcrawford
7 years ago

This would just mean the error message would be shown asynchronously after the response returns, but I think that is fine.

I agree seems okay to me as well.

So back to the original issue here - I have tracked down the current failure point in the flow of the OP, as to why the widget is rendering the attachment URL. In the video widget's render_media, we are calling the base class method is_attachment_with_mime_type https://core.trac.wordpress.org/browser/trunk/src/wp-includes/widgets/class-wp-widget-media-video.php#L112 which subsequently calls wp_attachment_is on the attachment and the mime type. That function returns false because there is no attached file for the VideoPress attachment post.

Back in the render_media logic - no $attachment is set, so wp_get_attachment_url is not called and the original attachment url returned from the media modal is used in the video shortcode, and the above screenshot is the result.

Solutions? Perhaps the VideoPress plugin can extend the media widget JS to ensure the proper oembed url is set based upon the videopress_guid returned from the WordPress.com API - this would likely also fix the render_preview issue as the oembed/proxy response would return a proper thumbnail_url for the video as well.

#5 follow-up: @westonruter
7 years ago

@timmydcrawford what about if VideoPress did this:

wp.mediaWidgets.controlConstructors.media_video.prototype.mapMediaToModelProps = (function( originalMapMediaToModelProps ) {
        return function( mediaFrameProps ) {
                var props = mediaFrameProps;
                if ( mediaFrameProps.videopress_guid ) {
                        props = _.extend( {}, props, {
                                url: 'https://videopress.com/v/' + mediaFrameProps.videopress_guid,
                                attachment_id: 0,
                                mp4: '' // @todo Would there be other file types returned?
                        });
                }
                return originalMapMediaToModelProps.call( this, props );
        };
}( wp.mediaWidgets.controlConstructors.media_video.prototype.mapMediaToModelProps ));

This would essentially force the selection of VideoPress attachment to be rewritten to be an external oEmbed.

It's a somewhat interesting situation we have here because the video is simultaneously both an attachment in the media library and an external hosted video oEmbed. To me it seems the primary value of the video being in the media library is that it is available for easy reference, but that it doesn't really have any value beyond that.

#6 in reply to: ↑ 5 @timmydcrawford
7 years ago

I thought that might be the best route to go also - but I think where it ends up not working is mediaelement not handling the oembed url for videopress properly. I tested this URL structure out and it did not work for me. Alternatively, the path to the video file on the VideoPress CDN works great.

I have a diff on the WPCOM API that would do just that, but need to see if it could potentially cause other issues. If changing the URL in the API doesn't work, then yes the approach you detailed above could work with constructing the video file URL.

FWIW - the flow of selecting an existing VideoPress upload from the media library in the Video Widget works great as-is, and this is because the URL returned there is that of the video file on the CDN.

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


7 years ago

#8 @timmydcrawford
7 years ago

As discussed in slack ( link above ) 40808.1.diff introduces a new method on the video widget for isHostedVideo which will allow plugins like Jetpack/VideoPress to add custom logic to the renderPreview flow and ensure their embed urls can be whitelisted for use with the oembed/proxy endpoint.

@westonruter
7 years ago

Fix jshint issue (remove unused var) and fix jshint config in grunt to check file

#9 @westonruter
7 years ago

  • Keywords has-patch added; needs-patch removed

@timmydcrawford great, so then the VideoPress plugin would need to enqueue JS that extends media-video-widget with something like:

wp.mediaWidgets.controlConstructors.media_video.prototype.isHostedVideo = (function( isHostedVideo ) {
        return function( url ) {
                var parsedUrl = document.createElement( 'a' );
                parsedUrl.href = url;
                if ( 'videos.files.wordpress.com' === parsedUrl.hostname || 'videopress.com' === parsedUrl.hostname ) {
                        return true;
                }
                return isHostedVideo.call( this, url );
        };
}( wp.mediaWidgets.controlConstructors.media_video.prototype.isHostedVideo ));

Right?

Last edited 7 years ago by westonruter (previous) (diff)

#10 @timmydcrawford
7 years ago

  • Keywords has-unit-tests added

Yep that looks about right... I was thinking of flipping the logic and seeing if the isHostedVideo returns true first, but 6 one way half dozen the other way :) - I'll get a Jetpack PR up soon and drop a link here when it is ready.

#12 @westonruter
7 years ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to accepted
  • Version set to trunk

#13 @westonruter
7 years ago

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

In 40810:

Widgets: Introduce isHostedVideo method on VideoWidgetControl to allow plugins to extend for recognizing services beyond YouTube and Vimeo.

Also update jshint configuration in Gruntfile to include the widget scripts among the JS files linted.

Props timmydcrawford.
See #39994.
Fixes #40808.

Note: See TracTickets for help on using tickets.