WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 5 weeks ago

Last modified 4 weeks ago

#40771 closed defect (bug) (fixed)

Unable to add YouTube video to Video Widget

Reported by: taijacg Owned by: westonruter
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

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

Steps to Reproduce

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.

Attachments (10)

error-message-video-widget.png (30.5 KB) - added by taijacg 5 weeks ago.
Error message
40771-1.diff (4.5 KB) - added by timmydcrawford 5 weeks ago.
40771.2.diff (5.2 KB) - added by westonruter 5 weeks ago.
Use jQuery.trim() and protocol-less oEmbed endpoint URL
40771.3.diff (3.7 KB) - added by timmydcrawford 5 weeks ago.
Remove un-needed trim()s
40771.4.diff (1.8 KB) - added by westonruter 5 weeks ago.
Trim at input
40771.5.diff (1.8 KB) - added by timmydcrawford 5 weeks ago.
Add keyup listener on url field and trim()
40771.6.diff (2.0 KB) - added by westonruter 5 weeks ago.
Use paste event in addition to keyup
40771.7.diff (2.4 KB) - added by westonruter 5 weeks ago.
40771.8.diff (1.3 KB) - added by westonruter 5 weeks ago.
40771.9.diff (1.7 KB) - added by westonruter 5 weeks ago.

Download all attachments as: .zip

Change History (43)

@taijacg
5 weeks ago

Error message

#1 @Presskopp
5 weeks ago

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.

#2 @westonruter
5 weeks ago

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

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


5 weeks ago

#4 @timmydcrawford
5 weeks ago

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?

#5 @taijacg
5 weeks ago

@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)

Last edited 5 weeks ago by taijacg (previous) (diff)

#6 @timmydcrawford
5 weeks ago

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

#7 @timmydcrawford
5 weeks ago

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.

#8 @Presskopp
5 weeks ago

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

#9 follow-up: @westonruter
5 weeks ago

@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.

#10 in reply to: ↑ 9 ; follow-up: @taijacg
5 weeks ago

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.

#11 @jnylen0
5 weeks ago

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

#12 in reply to: ↑ 10 @timmydcrawford
5 weeks ago

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?

#13 @jnylen0
5 weeks ago

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?

#14 @westonruter
5 weeks ago

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
5 weeks ago

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

#15 follow-up: @westonruter
5 weeks ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to westonruter
  • Status changed from new to accepted

@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?

#16 in reply to: ↑ 15 @timmydcrawford
5 weeks ago

@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
5 weeks ago

Remove un-needed trim()s

#17 @timmydcrawford
5 weeks ago

@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.

#18 follow-up: @jnylen0
5 weeks ago

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.

#19 @johnbillion
5 weeks ago

  • Milestone 4.8 deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

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

#20 @westonruter
5 weeks ago

@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.

#21 @westonruter
5 weeks ago

  • Milestone set to 4.8
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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

@westonruter
5 weeks ago

Trim at input

#22 @westonruter
5 weeks ago

@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.

#23 follow-up: @timmydcrawford
5 weeks ago

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.

#24 in reply to: ↑ 23 ; follow-up: @westonruter
5 weeks ago

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
5 weeks ago

Add keyup listener on url field and trim()

#25 in reply to: ↑ 24 @timmydcrawford
5 weeks ago

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
5 weeks ago

Use paste event in addition to keyup

#26 @westonruter
5 weeks ago

@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
5 weeks ago

#27 @westonruter
5 weeks ago

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

@westonruter
5 weeks ago

#28 @westonruter
5 weeks ago

There we go. So simple: 40771.8.diff

@westonruter
5 weeks ago

#29 follow-up: @westonruter
5 weeks ago

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

@timmydcrawford 👍?

#30 in reply to: ↑ 29 @timmydcrawford
5 weeks ago

Replying to westonruter:

Simple always wins, 👍 LGTM

#31 @westonruter
5 weeks ago

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

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.

#32 in reply to: ↑ 18 @rmccue
5 weeks ago

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.

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


4 weeks ago

Note: See TracTickets for help on using tickets.