#40771 closed defect (bug) (fixed)
Unable to add YouTube video to Video Widget
Reported by: |
|
Owned by: |
|
---|---|---|---|
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
- 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.
Attachments (10)
Change History (43)
#1
@
8 years 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.
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
8 years ago
#4
@
8 years 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
@
8 years 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)
#6
@
8 years 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
@
8 years 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.
#9
follow-up:
↓ 10
@
8 years 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:
↓ 12
@
8 years 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
@
8 years ago
40771-1.diff looks fine to me, but we should also improve the error messaging here.
#12
in reply to:
↑ 10
@
8 years 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
@
8 years 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
@
8 years 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.
#15
follow-up:
↓ 16
@
8 years 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
@
8 years 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 trim
s in media-video-widget.js could likely be removed. I will test and rev the diff if that is the case.
#17
@
8 years 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:
↓ 32
@
8 years 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
@
8 years 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
@
8 years 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
@
8 years 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.
#22
@
8 years 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:
↓ 24
@
8 years 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:
↓ 25
@
8 years 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.
#25
in reply to:
↑ 24
@
8 years 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.
#26
@
8 years 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.
#27
@
8 years ago
40771.7.diff does the trick, but it isn't very elegant.
#28
@
8 years ago
There we go. So simple: 40771.8.diff
#29
follow-up:
↓ 30
@
8 years 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
@
8 years ago
Replying to westonruter:
Simple always wins, 👍 LGTM
#32
in reply to:
↑ 18
@
8 years 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.
Error message