#493 ✓ staged
Bobby Richter

Youtube plugin misses videos with underscores in their id

Reported by Bobby Richter | April 28th, 2011 @ 04:04 PM | in 0.6

Videos such as this one are missing by the youtube player plugin: http://www.youtube.com/embed/GP53b__h4ew

This is why:

var matches = url.match( /((http:\/\/)?www\.)?youtube\.[a-z]+\/watch\?v\=[a-z0-9]+/i );

Should be:

var matches = url.match( /((http:\/\/)?www\.)?youtube\.[a-z]+\/watch\?v\=[a-z0-9_]+/i );

Lines 62 & 73.

Comments and changes to this ticket

  • Bobby Richter

    Bobby Richter April 28th, 2011 @ 04:08 PM

    Fix: https://github.com/secretrobotron/popcorn-js/commit/b6ea59fa98989bd...

    Should this be "\w" instead of "[a-z0-9]"? [a-z0-9] also doesn't include capitals.

  • Rick

    Rick April 28th, 2011 @ 04:29 PM

    • State changed from “new” to “assigned”
    • Milestone set to 0.6
    • Assigned user set to “Bobby Richter”
    • Milestone order changed from “55” to “0”

    Your changes don't work for the given url example:

    
    "http://www.youtube.com/embed/GP53b__h4ew".match( /((http:\/\/)?www\.)?youtube\.[a-z]+\/watch\?v\=[a-z0-9_]+/i );
    
    > null
    

    But does work for traditional youtube urls... (sortof?)

    
    "http://www.youtube.com/watch?v=hZCtw8o4OfQ".match( /((http:\/\/)?www\.)?youtube\.[a-z]+\/watch\?v\=[a-z0-9_]+/i );
    
    > ["http://www.youtube.com/watch?v=hZCtw8o4OfQ", "http://www.", "http://"]
    
  • Rick

    Rick April 28th, 2011 @ 04:32 PM

    This works for most types...

    
    "http://www.youtube.com/watch?v=hZCtw8o4OfQ".match( /^[^v]+v.(.{11}).*/ );
    
    >["http://www.youtube.com/watch?v=hZCtw8o4OfQ", "hZCtw8o4OfQ"]
    
    "http://www.youtube.com/watch?v=hZCtw8o4OfQ&feature=related".match( /^[^v]+v.(.{11}).*/ );
    
    >["http://www.youtube.com/watch?v=hZCtw8o4OfQ&feature=related", "hZCtw8o4OfQ"]
    
    
    A slight modification works for the embed urls...
    
    "http://www.youtube.com/embed/GP53b__h4ew".match( /^[^d]+d.(.{11}).*/ );
    
    >["http://www.youtube.com/embed/GP53b__h4ew", "GP53b__h4ew"]
    
  • Rick

    Rick April 28th, 2011 @ 04:35 PM

    The regex should really be cached and used like...

    
    var ryoutube = /^[^d]+d.(.{11}).*/;
    
    ryoutube.exec( "http://www.youtube.com/embed/GP53b__h4ew" );
    
    > ["http://www.youtube.com/embed/GP53b__h4ew", "GP53b__h4ew"]
    
  • Bobby Richter

    Bobby Richter May 4th, 2011 @ 09:23 PM

    • State changed from “assigned” to “peer-review-requested”

    How's this look? Used both of Rick's suggestions.

    https://github.com/secretrobotron/popcorn-js/commits/493

    Seems to work with underscores and without.

  • David Humphrey

    David Humphrey May 5th, 2011 @ 10:43 AM

    Bobby, one thing we've found is that using branch names that are simply integers can lead to problems when they overlap with the first digits of commit SHAs. I'd recommend t493 or bug493 or something vs. 493.

  • annasob

    annasob May 5th, 2011 @ 10:56 AM

    • Assigned user changed from “Bobby Richter” to “Scott Downe”

    Scott can u review this is't your expertise. Pass it to Rick for a second look once you are done :)

  • annasob

    annasob May 5th, 2011 @ 11:18 AM

    Sorry Scott i meant to say reg-ex is ur expertise

  • dseif

    dseif May 6th, 2011 @ 01:32 PM

    • State changed from “peer-review-requested” to “under-review”
  • Scott Downe

    Scott Downe May 6th, 2011 @ 02:07 PM

    • State changed from “under-review” to “super-review-requested”

    Yeah, very cool regex.

    It also works on links like this:

    http://www.youtube.com/v/hZCtw8o4OfQ

  • Scott Downe

    Scott Downe May 6th, 2011 @ 02:09 PM

    • Assigned user changed from “Scott Downe” to “Bobby Richter”
  • Rick

    Rick May 6th, 2011 @ 02:44 PM

    This is not a blocking review, but more an academic question... I'm wondering if we can make those two regex patterns into one? Any thoughts?

  • Scott Downe

    Scott Downe May 6th, 2011 @ 03:03 PM

    • State changed from “super-review-requested” to “review-needs-work”

    Yeah, I agree with merging both regexs into one.

    It also occurred to me we should be making tests of some sort for all the urls and uris tested.

    I am going to r- this for those two reasons.

  • Rick

    Rick May 6th, 2011 @ 03:07 PM

    Scott +9001!

    Good call on the tests.

  • Bobby Richter

    Bobby Richter May 6th, 2011 @ 11:10 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    https://github.com/secretrobotron/popcorn-js/commit/3c61e384ac799af...

    I'm not terribly proud of the regex I came up with, but it does the job, and it's relatively easy to add to if youtube adds/changes something. I opted for something readable and explicit, but I'm certainly open to something more elegant.

    The unit test file has one more test in it at the bottom. It runs through each kind of youtube link for which the regex matches.

    Two points of concern:

    • I actually snip the swf object from the dom tree after each equals() completes just to keep the dom simple. I'm not sure that it really matters though. I wasn't sure if the fact that consecutive calls to Popcorn.youtube() on the same div each append a new swf object is a bug, a feature, or something we should simply be aware of in the future.
    • 35/43 checks in the first test fail for me on 64bit Linux, but it's silly of me to expect that to be anything but regular old Linux behaviour, considering it's a miracle when I can watch any two flash videos in sequence on 64bit Linux right now. I'm going to head to a mac when I get a chance to try it out, but if a reviewer wouldn't mind double-checking to make sure it's still in working order, I would be much obliged.
  • Scott Downe

    Scott Downe May 8th, 2011 @ 09:59 AM

    Yeah, youtube's tests are not in the best state if I remember correctly. So I wouldn't worry about failures on tests you're not creating tests for + are there in a fresh 0.6.

    About the regex, I see why you changed what you did, because of this: 'http://www.youtube.com/e/9oar9glUCL0'

    You also need to have a test for this url: http://www.youtube.com/v/hZCtw8o4OfQ

    I'll play around with the regex and see what I can come up with.

  • Bobby Richter

    Bobby Richter May 8th, 2011 @ 10:06 AM

    • State changed from “peer-review-requested” to “review-needs-work”

    @Scott: I'm not terribly concerned with the regex not being pretty. If we all agree that it's

    1. readable for future change, and
    2. operating as desired,

    let's leave it, and just expand it when necessary. That is, of course, unless you have an elegant solution that also covers those two requisites.

    I'll get a test ready for /v/ as well.

  • Rick

    Rick May 8th, 2011 @ 12:21 PM

    I'm really concerned that a lot of attention is being paid to this ticket and not enough attention is being paid to the ticket that renders the youtube plugin completely unusable in production: #500

  • Scott Downe

    Scott Downe May 8th, 2011 @ 03:12 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    Rick, agreed. 500 is way more important. I'm just filling in smaller gaps of free time.

    I also enjoy playing with regex ;)

    This will work in all the cases we've found so far.

    /^.*[\/=](.{11})/
    

    I'm just greedily looking for the 11 characters after the / or =. Also works with /v/

  • Rick

    Rick May 8th, 2011 @ 07:04 PM

    Scott, that looks great. If we can make sure to add extensive tests to the youtube player's unit test file, I feel like we'll be in great shape.

  • Bobby Richter

    Bobby Richter May 9th, 2011 @ 11:16 AM

    Implemented Scott's beautiful regex, added /v/ to tests, and made test titles a little more explicit.

    https://github.com/secretrobotron/popcorn-js/commit/ae4cf70139646aa...

  • annasob

    annasob May 10th, 2011 @ 05:28 PM

    • Assigned user changed from “Bobby Richter” to “Scott Downe”
  • Scott Downe

    Scott Downe May 11th, 2011 @ 11:29 AM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user cleared.

    Awesome.

    tests pass.

  • Rick

    Rick May 16th, 2011 @ 04:38 PM

    • Assigned user set to “annasob”
  • annasob

    annasob May 16th, 2011 @ 05:07 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “annasob” to “Bobby Richter”

    Bobby, i dont see any tests that actually test the underscore :( please add and put back to super-review-requested

  • Bobby Richter

    Bobby Richter May 17th, 2011 @ 11:52 AM

    • State changed from “review-needs-work” to “super-review-requested”
  • cadecairos

    cadecairos May 17th, 2011 @ 11:54 AM

    • Assigned user changed from “Bobby Richter” to “annasob”

    assigning to Anna

  • annasob

    annasob May 17th, 2011 @ 12:16 PM

    • State changed from “super-review-requested” to “review-looks-good”

    Thanks bobby,

    • youtube unit tests pass

    • lint passes

    • core unit test pass

    Ran on Fedors Chrome, firefox 3.6 and 4.0

  • annasob

    annasob May 17th, 2011 @ 12:17 PM

    • State changed from “review-looks-good” to “staged”

    Staged in annasob/popcorn-js commit

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages