WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#32161 closed enhancement (fixed)

support youtube search friendly URL usualy used for sharing http://www.youtube.com/v/dr4HHepGxxM

Reported by: ritter.aaron Owned by: johnbillion
Milestone: 4.3 Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch
Focuses: Cc:

Description

youtube offers "search friendly" URLs for reposting/sharing e.g. http://www.youtube.com/v/dr4HHepGxxM

it would be nice if they could be added to the automated embeding

Attachments (4)

32161-1.patch (721 bytes) - added by dmchale 5 years ago.
32161-2.patch (718 bytes) - added by dmchale 5 years ago.
32161-3.patch (3.4 KB) - added by kjpro27 5 years ago.
changes to wp-oembed media test file
32161-4.patch (2.1 KB) - added by dmchale 5 years ago.
Creating test_youtube_com_friendly_url_embed() to test new handler for /v/ urls as well as the pre-existing /embed/ functionality. Minor changes to test_youtube_com_secure_embed() to fix pre-existing failure (first assertion always failed since return is always https.)

Download all attachments as: .zip

Change History (20)

#1 @dmchale
5 years ago

The issue with adding this is that already-supported URLs contain link tags with paths to their oembed data. The youtube.com/v/ and youtube.com/embed/ paths do not contain an oembed link tag.

That said, they DO contain canonical link tags which could probably be used to hijack the process and make "https://www.youtube.com/v/12345678" end up looking at "https://www.youtube.com/watch?v=12345678" for its oembed data. While technically you could try to cheat the system another way, I think relying on the canonical urls makes more sense (and is certainly more future-proof) than us making assumptions about what it "should" be. It's two curl posts instead of one but... *shrug*

It's also a hack for only these urls, so it would be up to the Core team if that is "allowed" since it's a fairly specific case.

I have some code done already, will try to post a patch soon.

#2 @MikeNGarrett
5 years ago

Wouldn't this be a matter of running some regex to grab the video ID from the url and then doing your curl?

#3 @dmchale
5 years ago

If I understand what you're saying @MikeNGarrett, you're suggesting what I cautioned against. If you simply strip the video ID off the url and curl to some other URL, you're making an assumption of what you should curl to.

Plus you don't inherently know if something besides the video ID is on the end of the url. I think it's much safer to first curl to the provided url and look there for the canonical url to youtube's "watch" page.

#4 @MikeNGarrett
5 years ago

Sorry for the misunderstanding. Thanks for clarifying.

@dmchale
5 years ago

#5 @dmchale
5 years ago

  • Keywords has-patch added

No worries at all @MikeNGarrett!

Turns out that I was grossly overthinking this anyway. It seems WordPress already had an override in wp-includes/media.php for handling youtube.com/embed/ paths... I simply added 1 line of code to mimic the functionality for youtube.com/v/ paths and it works like a champ. Patch attached.

#6 @MikeNGarrett
5 years ago

Looks good to me. Doing some research it looks like this url pattern was intended for older flash videos, but it looks like it's still being supported. I can't find anywhere that says youtube will drop support for this pattern anytime soon.

@dmchale
5 years ago

#7 @dmchale
5 years ago

Patch updated for coding standards... spaces snuck into the beginning of the line instead of a tab like it should be.

#8 @ritter.aaron
5 years ago

works :) cool stuff.

#9 @johnbillion
5 years ago

@dmchale WordPress doesn't fetch the embed URL, so whether or not its page contains a <link> tag with the oEmbed endpoint is moot. WordPress just passes the URL to the known oEmbed endpoint for that provider and embeds the response.

As you've noted, we already have wp_embed_handler_youtube(). Your patch should be sufficient.

It'd be nice if we had some unit tests for these.

#10 @johnbillion
5 years ago

  • Version changed from 4.2.1 to 2.9

#11 @kjpro27
5 years ago

I would be willing jump on the unit testing.

#12 @kjpro27
5 years ago

wp_oembed_get is returning false for the https://www.youtube.com/v/ url structure. I'm still looking into the issue but I'll update once I have more info.

@kjpro27
5 years ago

changes to wp-oembed media test file

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


5 years ago

#14 @johnbillion
5 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Owner set to johnbillion
  • Status changed from new to assigned

@dmchale
5 years ago

Creating test_youtube_com_friendly_url_embed() to test new handler for /v/ urls as well as the pre-existing /embed/ functionality. Minor changes to test_youtube_com_secure_embed() to fix pre-existing failure (first assertion always failed since return is always https.)

#15 @dmchale
5 years ago

This is my first time writing unit tests, so please be gentle :)

Patch 4 added for testing youtube.com/embed/ URLs as well as the new youtube.com/v/ URLs. Mimicking the functionality of the handlers, we regex the given URL to pull the video id from it, run it through wp_embed_handler_youtube() to get a youtube.com?v=xxxxxxxx formatted url, and then take the final step of passing that through wp_oembed_get() to get a full https://www.youtube.com/embed/oHg5SJYRHA0?feature=oembed result.

Not sure if that's too far, not enough, or done the wrong way - so input is more than welcome.

#16 @wonderboymusic
5 years ago

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

In 32787:

In the youtube_embed_url embed handler, make embed a non-capturing group that alternately matches for v - YouTube supports both URL paths.

Add unit test cases.

Props dmchale for some patch work.
Fixes #32161.

Note: See TracTickets for help on using tickets.