If there are parameters in the url before the v parameter the url won't save. I included a patch file I have that changes that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guschilds’s picture

Status: Active » Needs review
FileSize
1.29 KB

@argiope Thanks for bringing this up and supplying a patch. The patch included a lot of irrelevant white space removal. This needed addressed regardless so I removed it from all files and committed it (see 122086a).

I then created a patch that uses your code to address only the issue at hand. Hopefully we can get a few people to test it.

To test:

  1. Install a clean copy of the 7.x-1.0 release. Create a custom YouTube field on any content type.
  2. Try adding a piece of content with a YouTube field value where "v" (the video id) is not the first parameter, such as http://www.youtube.com/watch?autoplay=1&v=AJuZ8x_ETn0. It should fail validation.
  3. Apply the patch by saving the attached patch in the youtube module's directory and running git apply -v youtube-video_id-not-first-param-1865284-1.patch
  4. Try saving the new piece of content again with the value supplied above. It should save appropriately and properly play the YouTube video represented by that video id. Please note that the autoplay parameter (and any others) will/should be ignored.

Thanks again,
Gus

ttkaminski’s picture

I've included a patch that aggressively tries to parse out the youtube id from the url. I've used some online references to put together a regex that tries to cover every case. I also added support for raw mode, where you can enter the youtube video id directly. Upon save, it will convert the raw youtube id to a valid url. It turns out that my patch simplifies the code a bit :)

misthero’s picture

this
if (strstr($input, 'youtube.com/watch?') && preg_match('/v=[^&]*(?=&|$)/', $input, $matches)) {

will fail when video id starts with "v"

for example this video
http://www.youtube.com/watch?v=vM6asX55k1Y

will return the following "wrong" video id:
M6asX55k1Y

stripping the first "v"

what about something like this:

preg_match("/^(?:http(?:s)?:\/\/)?(?:www\.)?(?:youtu\.be\/|youtube\.com\/(?:(?:watch)?\?(?:.*&)?v(?:i)?=|(?:embed|v|vi|user)\/))([^\?&\"'>]+)/", $url, $matches);

$matches[1] contains the video_id

it should covers the following cases:

from: http://stackoverflow.com/questions/3392993/php-regex-to-get-youtube-vide...

the final function would become:

function youtube_get_video_id($input) {
  preg_match("/^(?:http(?:s)?:\/\/)?(?:www\.)?(?:youtu\.be\/|youtube\.com\/(?:(?:watch)?\?(?:.*&)?v(?:i)?=|(?:embed|v|vi|user)\/))([^\?&\"'>]+)/", $input, $matches);
	
  $video_id = $matches[1];
  if (!empty($video_id)) {
    return $video_id;
  }
  return FALSE;
}
guschilds’s picture

Yikes! Thanks for bringing this up. We'll surely want to get it fixed quickly.

I've attached a patch that uses the regex you've suggested. This allows formats not previously supported (updated in README) and, more importantly, doesn't break the embedded video if its ID starts with 'v'.

The patch should apply to both 7.x-1.x branch and the 7.x-1.1 release. I'd like to get this tested asap.

Thanks again!

ttkaminski’s picture

My patch in #2 fixed this a year ago. It also supports raw youtube id input format, and also simplifies the code. Why was it ignored?

guschilds’s picture

ttkaminski,

I'd originally left it as 'Needs review' because I was hoping folks with more regex knowledge would have a look at it. To be honest, I'd then forgotten about it until I got this thread's alert this morning.

With the past introduction of youtube_get_video_id() in youtube.inc, your patch doesn't simplify things any more than #4. I was also unable to quickly tell which new formats, aside from raw, your patch supported. If interested, could you re-roll your patch to apply to the 7.x-1.1 release and specify what formats it supports?

Thanks,
Gus

guschilds’s picture

Status: Needs review » Fixed

Hey all,

Due to video_ids that begin with 'v' failing, I wanted to get this fix committed sooner than later.

After thorough testing, I've committed the patch from #4 to the 7.x-1.x branch and an equivalent to the 8.x-1.x branch.

Thanks for all of your help with this!

Gus

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Vensdale’s picture

After applying patch #4 to 7.x-1.1 release I got an error Notice: Undefined offset: 1 in function youtube_get_video_id() (string 20 in ...\sites\all\modules\youtube\youtube.inc). every time when I'm saving changes to nodes with youtube IDs starting with "v" . Refreshing the page removes that error and everything works fine as far as I see (including "v" IDs).

guschilds’s picture

Thanks for reporting this, Vensdale.

The warning was actually being thrown when no value was supplied to a Video field.

I've corrected this in both the 7.x-1.x and 8.x-1.x branches.

If you'd rather not use the 7.x-1.x branch, I've fixed the patch from #4. It applies directly to the 7.x-1.1 release.