Unable to add YouTube video to Video Widget

[taijacg]

When I attempt to use the Video Widget to display a video from YouTube, I get an error.

Steps to Reproduce

• Current WordPress version: 4.8-beta1-40668
• Add Video Widget to widget area
• Click 'Add Video'
• Click 'Insert from URL'
• Paste in YouTube URL (for example ​https://www.youtube.com/watch?v=PQQkGm7YAYg )

Error Message
Sorry, we can’t display the video file type selected. Please select a supported video file (.mp4, .m4v, .webm, .ogv, .wmv, .flv) or stream (YouTube or Vimeo) instead.

[taijacg]

Error message

[Presskopp]

confirmed. Looks like a regression between 40663 and 40668. I added the video successfully using 40663, then updated to 40668 and while the video was still showing, deleting the URL completely started showing the message. I then deleted the widget and tried to make it new, and it was not possible anymore.

[timmydcrawford]

Thanks for the report - I just pulled 40670 and unable to reproduce the error myself using various YouTube videos. I tested on Chrome and Firefox - @taijacg which browser did you experience this in?

[taijacg]

@timmydcrawford - I'm running Chrome 58 on MacOS 10.12.4. Note that I experienced the issue on 40668, which seems to be the latest I can get (running 'bleeding edge nightlies' through Beta Tester plugin)

Update
I am also able to reproduce the issue on Safari 10.1 (12603.1.30.0.34) and Firefox 53.0.2 (64-bit)

[timmydcrawford]

Thanks for the details @taijacg - I'll try against that revision too - but can not reproduce against a current checkout of trunk right now.

One way I can reproduce is to add a space on the end of a YouTube URL, like pasting the following in to the URL field will cause the same issue: https://www.youtube.com/watch?v=PQQkGm7YAYg - note the space on the end.

That particular issue is fixed in 40771-1.diff​

[timmydcrawford]

Okay tested in r40668 too, but I was still able to add a video from YouTube, so not entirely sure why I am not able to get the failure to happen.

[Presskopp]

what I said in comment:1 did never happen again, since and now with 40674 it's working fine so far for me.

Only the error message could be improved, if you ask me, see #40774

[westonruter]

@taijacg Hi! Would you please open the network console and look at the XHR HTTP request that is made after you paste in the YouTube URL? It should be making a request off to the oEmbed endpoint in the WP REST API. If that request fails then that might be a reason you are seeing the error. Please share the HTTP response from that request.

[taijacg]

Replying to westonruter:

Interesting; it looks like the XHR request is failing with the status (blocked:mixed-content). Here are the headers- there is no response.

Request URL:http://wp.local/search/wp-json/oembed/1.0/proxy?url=https%3A%2F%2Fwww.youtube.com%2Fwatch%3Fv%3DrEg6oeazTNY&_wpnonce=eea4ecbabd&discover=false
Referrer Policy:no-referrer-when-downgrade
Request Headers
Provisional headers are shown
Accept:application/json, text/javascript, */*; q=0.01
Origin:https://wp.local
Query String Parameters
view source
view URL encoded
url:https://www.youtube.com/watch?v=rEg6oeazTNY
_wpnonce:eea4ecbabd
discover:false

@taijacg Hi! Would you please open the network console and look at the XHR HTTP request that is made after you paste in the YouTube URL? It should be making a request off to the oEmbed endpoint in the WP REST API. If that request fails then that might be a reason you are seeing the error. Please share the HTTP response from that request.

[jnylen0]

40771-1.diff​ looks fine to me, but we should also improve the error messaging here.

[timmydcrawford]

Replying to taijacg:

Looks like the origin.url is https and the destination is http. Seems like we could maybe change wp.media.view.settings to start with a // to prevent this from happening?

[jnylen0]

Good catch @timmydcrawford, indeed under this situation browsers will block API requests as mixed content.

If there is something to fix here, we should do it in the PHP rest_url function instead.

However this does seem like a misconfigured site to me. @taijacg how did you set up HTTPS on this installation?

[westonruter]

I see get_rest_url() is only looking at is_ssl() for whether to use the HTTPS protocol. When there is a proxy in front, it often returns the incorrect value as has been discussed in the context of #31288. I think the best solution is to just strip off the protocol. @taijacg do you have HTTPS termination on a reverse proxy?

In regards to 40771-1.diff​, I think we should use jQuery.trim() instead of String.prototype.trim(), as the latter is only available in IE11. True we're not supporting older IE anymore, but no need to break for such a trivial thing.

[westonruter]

Use jQuery.trim() and protocol-less oEmbed endpoint URL

[westonruter]

@timmydcrawford only question I have about 40771.2.diff​ now is whether or not we really need all of the instances of trim. Is trimming of already-trimmed strings happening?

[timmydcrawford]

@timmydcrawford only question I have about 40771.2.diff​ now is whether or not we really need all of the instances of trim. Is trimming of already-trimmed strings happening?

I believe we still need one in fetch since that is operating directly from the user input, not the model attribute that has been sanitized. I agree that the trims in media-video-widget.js could likely be removed. I will test and rev the diff if that is the case.

[timmydcrawford]

Remove un-needed trim()s

[timmydcrawford]

@westonruter removed the trim logic that is no longer needed in 40771.3.diff​ - testing out well for me, thanks for adding in the jQuery.trim updates as well.

[jnylen0]

I am -1 on the proposed solution for the URL here (modifying the return value of rest_url with a regex) for several reasons.

First, this is the first usage of the REST API in the admin and having the standard function for this fail right off the bat and having to hack around it with a regex is not a great example, this worries me a bit.

Second, and more specifically, it's possible to have all kinds of exotic configurations where wp-admin URLs are different from the URLs of other parts of WP, including the API. These can be different hostnames and protocols, like they are on WP.com. Undoubtedly other multisite installations have done similar things.

If we really do need separate behavior for getting a REST URL in the admin context, then it needs to live in a new, reusable function like wp_admin_rest_url. However, we should try pretty hard to avoid this by just making rest_url() work correctly. For example, we could change the API protocol to HTTPS if the current page is served over HTTPS (is_ssl()) and the REST API is served over HTTP (wp_startswith( strolower( rest_url( ... ) ), 'http:' )).

See #34299 which does something similar. I'm not sure why that logic wouldn't be kicking in here.

See also #36451 which looks like the same underlying issue to me.

cc @johnbillion @rmccue who have looked at such issues in the past.

[johnbillion]

Yeah the underlying issue is #36451. Needs some more eyes.

[westonruter]

@jnylen0 re: #34299, at the risk of restating things, the problem is that is_ssl() simply does not reliably return whether or not the user's browser is connected via HTTPS. As noted in #31288, when a HTTPS termination is done at a proxy then is_ssl() will return false and the only PHP solutions I'm aware of are to start looking at environment vars that the proxy sets, such as $_SERVER['HTTP_X_FORWARDED_PROTO'] or $_SERVER['HTTP_X_SSL']. The reason these headers are not looked at in is_ssl() is explained in #31288. And the cause for why the protocol isn't matching in the admin context is that the rest_url is constructed off of the home_url and not the (admin) site_url, where the latter would always be HTTPS if force_ssl_admin().

So I think we either strip the protocol from the URL returned by rest_url(), or before we switch the browser's actual scheme on the client with JS.

[westonruter]

Re-opening to at least address the issue with accidental whitespace being included in oEmbed URLs.

[westonruter]

Trim at input

[westonruter]

@timmydcrawford I found a problem when the URL field has whitespace at the beginning: the oEmbed preview doesn't show up. What if we trimmed the whitespace out of the underlying url in the model? Like in 40771.4.diff​? This implementation is not ideal because the non-trimmed value still appears in the field and momentarily populates the model. It would be better if we could just strip whitespace on input, preventing whitespace from the field altogether.

[timmydcrawford]

This implementation is not ideal because the non-trimmed value still appears in the field and momentarily populates the model. It would be better if we could just strip whitespace on input, preventing whitespace from the field altogether.

On that note would this be better fixed in the media modal itself? Having spaces in the insert from URL field in the editor media flow is broken too.

[westonruter]

Replying to timmydcrawford:

On that note would this be better fixed in the media modal itself? Having spaces in the insert from URL field in the editor media flow is broken too.

Yes, absolutely. And that's actually what 40771.4.diff​ is an initial stab at doing. I'm not happy with how it is intercepting whitespace from being input into the field, however.

[timmydcrawford]

Add keyup listener on url field and trim()

[timmydcrawford]

Yes, absolutely. And that's actually what 40771.4.diff​ is an initial stab at doing. I'm not happy with how it is intercepting whitespace from being input into the field, however.

:facepalm: totally missed the new diff there. In 40771.5.diff​ I've tried using a keyup listener to trim the value - seems to work reasonably well.

[westonruter]

Use paste event in addition to keyup

[westonruter]

@timmydcrawford that does work much better. One other thing I notice, however, is that if you paste into the field purely with the mouse by right-clicking on the input field and selecting from the context menu, that the whitespace isn't getting trimmed in that case. I thought maybe the paste event would do the trick in 40771.6.diff​ but it doesn't.

[westonruter]

40771.7.diff​ does the trick, but it isn't very elegant.

[westonruter]

There we go. So simple: 40771.8.diff​

[westonruter]

And 40771.9.diff​ accounts for the unexpected case where if you clear out the URL field it shows an error message.

@timmydcrawford 👍?

[timmydcrawford]

Replying to westonruter:

Simple always wins, 👍 LGTM

[westonruter]

In 40772:

Media: Trim whitespace in URLs provided for external embeds.

Also avoid showing error notice in media widget when URL field is empty.

Props timmydcrawford, westonruter.
Fixes #40771.

[rmccue]

Replying to jnylen0:

I am -1 on the proposed solution for the URL here (modifying the return value of rest_url with a regex) for several reasons.

FWIW, agree on @jnylen0 here on the underlying problem; let's fix the URL weirdness, not do string replacements on the result.