Moodle
  1. Moodle
  2. MDL-34997

Youtube filter : Allow shortened url youtu.be and y2u.be

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3, 2.4
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Filters
    • Labels:
    • Rank:
      43586

      Description

      Allow youtube filter to integrate shortened url youtu.be and y2u.be.

      This funcitonnality become more important since youtube made "youtu.be" the default url when sharing a video (Share button on the page of a video).

        Issue Links

          Activity

          Jean-Philippe Gaudreau created issue -
          Jean-Philippe Gaudreau made changes -
          Field Original Value New Value
          Priority Minor [ 4 ] Major [ 3 ]
          Workaround Use "www.youtube.com" url to make the filter work.
          Jean-Philippe Gaudreau made changes -
          Assignee moodle.com [ moodle.com ] Jean-Philippe Gaudreau [ gaudreaj ]
          Jean-Philippe Gaudreau made changes -
          Assignee Jean-Philippe Gaudreau [ gaudreaj ] moodle.com [ moodle.com ]
          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this.

          If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Michael de Raadt made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Labels triaged
          Affects Version/s DEV backlog [ 10464 ]
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Michael,

          I've just submitted a patch for 2.2, 2.3 and master.

          I'm willing to self-assign the task but please tell me who should I put as peer reviewer?

          Thx!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Michael, I've just submitted a patch for 2.2, 2.3 and master. I'm willing to self-assign the task but please tell me who should I put as peer reviewer? Thx!
          Jean-Philippe Gaudreau made changes -
          Pull Master Diff URL https://github.com/StudiUM/moodle/compare/master...MDL-34997_youtube_filter_shortened_url
          Pull Master Branch MDL-34997_youtube_filter_shortened_url
          Pull 2.3 Diff URL https://github.com/StudiUM/moodle/compare/MOODLE_23_STABLE...MDL-34997_youtube_filter_shortened_url_moodle23
          Pull 2.2 Diff URL https://github.com/StudiUM/moodle/compare/MOODLE_22_STABLE...MDL-34997_youtube_filter_shortened_url_moodle22
          Pull 2.2 Branch MDL-34997_youtube_filter_shortened_url_moodle22
          Labels triaged patch triaged
          Pull 2.3 Branch MDL-34997_youtube_filter_shortened_url_moodle23
          Jean-Philippe Gaudreau made changes -
          Assignee moodle.com [ moodle.com ] Jean-Philippe Gaudreau [ gaudreaj ]
          Jean-Philippe Gaudreau made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Jean-Philippe Gaudreau made changes -
          Testing Instructions * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 3 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY
          * *You should see the three embedded Youtube videos in the labels of the course*
          Jean-Philippe Gaudreau made changes -
          Status Development in progress [ 3 ] Open [ 1 ]
          Jean-Philippe Gaudreau made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          Frédéric Massart made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer fred
          Hide
          Frédéric Massart added a comment -

          Hi Jean-Philippe, thanks for working on this issue!

          Here are my comments about your patch:

          • The regex is getting really hard to read, perhaps that would be a good idea to split the different domains in variables and include them in the global regular expression.
          • The domains y2u.be and youtu.be do not support -nocookie, so you can remove that part
          • Even it is unlikely to happen, that would be nice to support www.youtu.be and www.y2u.be
          • The comment on line 551 is not accurate any more
          • In get_embeddable_markers() the . should be escaped as those are considered as regular expressions
          • Again in get_embeddable_markers() the domain youtube-nocookie is not supported any more.

          Those are only minor changes but your patch will then be perfect! Thank you

          Show
          Frédéric Massart added a comment - Hi Jean-Philippe, thanks for working on this issue! Here are my comments about your patch: The regex is getting really hard to read, perhaps that would be a good idea to split the different domains in variables and include them in the global regular expression. The domains y2u.be and youtu.be do not support -nocookie, so you can remove that part Even it is unlikely to happen, that would be nice to support www.youtu.be and www.y2u.be The comment on line 551 is not accurate any more In get_embeddable_markers() the . should be escaped as those are considered as regular expressions Again in get_embeddable_markers() the domain youtube-nocookie is not supported any more. Those are only minor changes but your patch will then be perfect! Thank you
          Frédéric Massart made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Hide
          Jean-Philippe Gaudreau added a comment - - edited

          Hi Frédéric,

          I don't know why but it seems the regex are failing for "youtu\.be/" and "y2u\.be/" when I escape "." in get_embeddable_markers()... Can you help me on this one?
          Here's the code :

          return array('youtube\.com/','youtu\.be/','y2u\.be/');
          

          Would it be ok to set the markers without the extensions ".com" and ".be"?

          return array('youtube','youtu','y2u');
          

          This way everything seems to be fine.

          I'll put the new patches with your comments soon.

          Thx for the review!

          Show
          Jean-Philippe Gaudreau added a comment - - edited Hi Frédéric, I don't know why but it seems the regex are failing for "youtu\.be/" and "y2u\.be/" when I escape "." in get_embeddable_markers()... Can you help me on this one? Here's the code : return array('youtube\.com/','youtu\.be/','y2u\.be/'); Would it be ok to set the markers without the extensions ".com" and ".be"? return array('youtube','youtu','y2u'); This way everything seems to be fine. I'll put the new patches with your comments soon. Thx for the review!
          Jean-Philippe Gaudreau made changes -
          Testing Instructions * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 3 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY
          * *You should see the three embedded Youtube videos in the labels of the course*
          * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 5 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          * *You should see the three embedded Youtube videos in the labels of the course*
          Jean-Philippe Gaudreau made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Hide
          Jean-Philippe Gaudreau added a comment -

          New patches in github for the 3 branches.

          Bye!

          Show
          Jean-Philippe Gaudreau added a comment - New patches in github for the 3 branches. Bye!
          Hide
          Frédéric Massart added a comment - - edited

          Hi Jean-Philippe,

          I was wrong about to suggest to escape the . in get_embeddable_markers(), the regex is actually escaped further in the logic. I still like the idea of having the tld in the regex, especially for small domains like y2u. Not that it wouldn't work the way you did it, but I guess we would save a bit of CPU by doing so.

          Here is patch which applies the tld, fixes a few white space issues, moves the encapsulating brackets for the domain out of both variables, restores the support for youtube-nocookie and gets the last match by default:

          diff --git a/filter/mediaplugin/tests/filter_test.php b/filter/mediaplugin/tests/filter_test.php
          index 45d00bf..73fd656 100644
          --- a/filter/mediaplugin/tests/filter_test.php
          +++ b/filter/mediaplugin/tests/filter_test.php
          @@ -57,6 +57,7 @@ class filter_mediaplugin_testcase extends advanced_testcase {
                       '<a id="movie player" class="center" href="http://moodle.org/testfile/test.mpg">test mpg</a>',
                       '<a href="http://moodle.org/testfile/test.ram">test</a>',
                       '<a href="http://www.youtube.com/watch?v=JghQgA2HMX8" class="href=css">test file</a>',
          +            '<a href="http://www.youtube-nocookie.com/watch?v=JghQgA2HMX8" class="href=css">test file</a>',
                       '<a href="http://youtu.be/JghQgA2HMX8" class="href=css">test file</a>',
                       '<a href="http://y2u.be/JghQgA2HMX8" class="href=css">test file</a>',
                       '<a class="youtube" href="http://www.youtube.com/watch?v=JghQgA2HMX8">test file</a>',
          diff --git a/lib/medialib.php b/lib/medialib.php
          index e1f3807..6985c17 100644
          --- a/lib/medialib.php
          +++ b/lib/medialib.php
          @@ -526,7 +526,7 @@ OET;
            */
           class core_media_player_youtube extends core_media_player_external {
               protected function embed_external(moodle_url $url, $name, $width, $height, $options) {
          -        $videoid = $this->matches[6];
          +        $videoid = end($this->matches);
           
                   $info = trim($name);
                   if (empty($info) or strpos($info, 'http') === 0) {
          @@ -545,14 +545,14 @@ OET;
           
               }
           
          -    protected function get_regex() {     
          +    protected function get_regex() {
                   // Regex for standard youtube link
          -        $link = '((youtube\.com/(?:watch\?v=|v/))';
          +        $link = '(youtube(-nocookie)?\.com/(?:watch\?v=|v/))';
                   // Regex for shortened youtube link
          -        $shortlink = '((youtu|y2u)\.be/))';
          -         
          +        $shortlink = '((youtu|y2u)\.be/)';
          +
                   // Initial part of link.
          -        $start = '~^https?://(www\.)?' . $link . '|' . $shortlink;
          +        $start = '~^https?://(www\.)?(' . $link . '|' . $shortlink . ')';
                   // Middle bit: Video key value
                   $middle = '([a-z0-9\-_]+)';
                   return $start . $middle . core_media_player_external::END_LINK_REGEX_PART;
          @@ -565,7 +565,7 @@ OET;
               }
           
               public function get_embeddable_markers() {
          -        return array('youtube','youtu','y2u');
          +        return array('youtube.com', 'youtube-nocookie.com', 'youtu.be', 'y2u.be');
               }
           }
          

          Let me know when your branches are updated and I'll push it straight for integration!

          Thank you and sorry for those back and forth!

          Show
          Frédéric Massart added a comment - - edited Hi Jean-Philippe, I was wrong about to suggest to escape the . in get_embeddable_markers(), the regex is actually escaped further in the logic. I still like the idea of having the tld in the regex, especially for small domains like y2u. Not that it wouldn't work the way you did it, but I guess we would save a bit of CPU by doing so. Here is patch which applies the tld, fixes a few white space issues, moves the encapsulating brackets for the domain out of both variables, restores the support for youtube-nocookie and gets the last match by default: diff --git a/filter/mediaplugin/tests/filter_test.php b/filter/mediaplugin/tests/filter_test.php index 45d00bf..73fd656 100644 --- a/filter/mediaplugin/tests/filter_test.php +++ b/filter/mediaplugin/tests/filter_test.php @@ -57,6 +57,7 @@ class filter_mediaplugin_testcase extends advanced_testcase { '<a id="movie player" class="center" href="http://moodle.org/testfile/test.mpg">test mpg</a>', '<a href="http://moodle.org/testfile/test.ram">test</a>', '<a href="http://www.youtube.com/watch?v=JghQgA2HMX8" class="href=css">test file</a>', + '<a href="http://www.youtube-nocookie.com/watch?v=JghQgA2HMX8" class="href=css">test file</a>', '<a href="http://youtu.be/JghQgA2HMX8" class="href=css">test file</a>', '<a href="http://y2u.be/JghQgA2HMX8" class="href=css">test file</a>', '<a class="youtube" href="http://www.youtube.com/watch?v=JghQgA2HMX8">test file</a>', diff --git a/lib/medialib.php b/lib/medialib.php index e1f3807..6985c17 100644 --- a/lib/medialib.php +++ b/lib/medialib.php @@ -526,7 +526,7 @@ OET; */ class core_media_player_youtube extends core_media_player_external { protected function embed_external(moodle_url $url, $name, $width, $height, $options) { - $videoid = $this->matches[6]; + $videoid = end($this->matches); $info = trim($name); if (empty($info) or strpos($info, 'http') === 0) { @@ -545,14 +545,14 @@ OET; } - protected function get_regex() { + protected function get_regex() { // Regex for standard youtube link - $link = '((youtube\.com/(?:watch\?v=|v/))'; + $link = '(youtube(-nocookie)?\.com/(?:watch\?v=|v/))'; // Regex for shortened youtube link - $shortlink = '((youtu|y2u)\.be/))'; - + $shortlink = '((youtu|y2u)\.be/)'; + // Initial part of link. - $start = '~^https?://(www\.)?' . $link . '|' . $shortlink; + $start = '~^https?://(www\.)?(' . $link . '|' . $shortlink . ')'; // Middle bit: Video key value $middle = '([a-z0-9\-_]+)'; return $start . $middle . core_media_player_external::END_LINK_REGEX_PART; @@ -565,7 +565,7 @@ OET; } public function get_embeddable_markers() { - return array('youtube','youtu','y2u'); + return array('youtube.com', 'youtube-nocookie.com', 'youtu.be', 'y2u.be'); } } Let me know when your branches are updated and I'll push it straight for integration! Thank you and sorry for those back and forth!
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Frédéric,

          Branches are fixed. Thx for the patch!

          Show
          Jean-Philippe Gaudreau added a comment - Hi Frédéric, Branches are fixed. Thx for the patch!
          Hide
          Frédéric Massart added a comment -

          Thanks Jean-Philippe, I have created new branches to squash your commits and fix some white space issues but I kept the credit to you. Pushing for integration.

          Show
          Frédéric Massart added a comment - Thanks Jean-Philippe, I have created new branches to squash your commits and fix some white space issues but I kept the credit to you. Pushing for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Sam Hemelryk made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (just for confirmation... testing instructions need amending, correct? They are 5, not 3)

          Show
          Eloy Lafuente (stronk7) added a comment - (just for confirmation... testing instructions need amending, correct? They are 5, not 3)
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator stronk7
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, I'm reopening this:

          0) Tiny: fix the testing instructions.
          1) Patches are different and should be the same if possible.
          2) We cannot use end($matches). All the URLs for multimedia filter can have extra parameter(s) to specify the width and height (END_LINK_REGEX_PART) so we need to check the exact match number X (3?). It would be great to get some tests with those optional dimension params in use.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this: 0) Tiny: fix the testing instructions. 1) Patches are different and should be the same if possible. 2) We cannot use end($matches). All the URLs for multimedia filter can have extra parameter(s) to specify the width and height (END_LINK_REGEX_PART) so we need to check the exact match number X (3?). It would be great to get some tests with those optional dimension params in use. Ciao
          Eloy Lafuente (stronk7) made changes -
          Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
          Hide
          Frédéric Massart added a comment -

          Hi Eloy, thanks for working on this.

          1/ Moodle 2.2 is relatively different regarding the media filtering (lib/medialib.php does not even exist)
          2/ I could be wrong, but I think that the size is handled in another part of the whole process ($width and $height are passed via the arguments). I don't think that $this->matches is meant to contain other information than the media URL information.

          Although, I'd be happy to read Jean-Philippe's feedback as I might have missed something.

          Cheers

          Show
          Frédéric Massart added a comment - Hi Eloy, thanks for working on this. 1/ Moodle 2.2 is relatively different regarding the media filtering (lib/medialib.php does not even exist) 2/ I could be wrong, but I think that the size is handled in another part of the whole process ($width and $height are passed via the arguments). I don't think that $this->matches is meant to contain other information than the media URL information. Although, I'd be happy to read Jean-Philippe's feedback as I might have missed something. Cheers
          CiBoT made changes -
          Status Reopened [ 4 ] Reopened [ 4 ]
          Currently in integration Yes [ 10041 ]
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi guys,

          From what I can see in the code, I think Frédéric is right but maybe I missed something too.

          See from "root" embed_url function in "lib/outputrenderers.php :

              /**
               * Renders a media file (audio or video) using suitable embedded player.
               *
               * See embed_alternatives function for full description of parameters.
               * This function calls through to that one.
               *
               * When using this function you can also specify width and height in the
               * URL by including ?d=100x100 at the end. If specified in the URL, this
               * will override the $width and $height parameters.
               *
               * @param moodle_url $url Full URL of media file
               * @param string $name Optional user-readable name to display in download link
               * @param int $width Width in pixels (optional)
               * @param int $height Height in pixels (optional)
               * @param array $options Array of key/value pairs
               * @return string HTML content of embed
               */
              public function embed_url(moodle_url $url, $name = '', $width = 0, $height = 0,
                      $options = array()) {
          
                  // Get width and height from URL if specified (overrides parameters in
                  // function call).
                  $rawurl = $url->out(false);
                  if (preg_match('/[?#]d=([\d]{1,4}%?)x([\d]{1,4}%?)/', $rawurl, $matches)) {
                      $width = $matches[1];
                      $height = $matches[2];
                      $url = new moodle_url(str_replace($matches[0], '', $rawurl));
                  }
          
                  // Defer to array version of function.
                  return $this->embed_alternatives(array($url), $name, $width, $height, $options);
              }
          

          Eloy, can you explain more in details what could be the problem ?

          Thx!

          Show
          Jean-Philippe Gaudreau added a comment - Hi guys, From what I can see in the code, I think Frédéric is right but maybe I missed something too. See from "root" embed_url function in "lib/outputrenderers.php : /** * Renders a media file (audio or video) using suitable embedded player. * * See embed_alternatives function for full description of parameters. * This function calls through to that one. * * When using this function you can also specify width and height in the * URL by including ?d=100x100 at the end. If specified in the URL, this * will override the $width and $height parameters. * * @param moodle_url $url Full URL of media file * @param string $name Optional user-readable name to display in download link * @param int $width Width in pixels (optional) * @param int $height Height in pixels (optional) * @param array $options Array of key/value pairs * @ return string HTML content of embed */ public function embed_url(moodle_url $url, $name = '', $width = 0, $height = 0, $options = array()) { // Get width and height from URL if specified (overrides parameters in // function call). $rawurl = $url->out( false ); if (preg_match('/[?#]d=([\d]{1,4}%?)x([\d]{1,4}%?)/', $rawurl, $matches)) { $width = $matches[1]; $height = $matches[2]; $url = new moodle_url(str_replace($matches[0], '', $rawurl)); } // Defer to array version of function. return $ this ->embed_alternatives(array($url), $name, $width, $height, $options); } Eloy, can you explain more in details what could be the problem ? Thx!
          Hide
          Jean-Philippe Gaudreau added a comment -

          By the way, it's seems setting dimensions with the complete Youtube link doesn't work with or without the fix of this task.

          Try adding a label with the follwing URL : http://www.youtube.com/watch?v=OMYzsAurxHY&d=200x200

          Show
          Jean-Philippe Gaudreau added a comment - By the way, it's seems setting dimensions with the complete Youtube link doesn't work with or without the fix of this task. Try adding a label with the follwing URL : http://www.youtube.com/watch?v=OMYzsAurxHY&d=200x200
          Jean-Philippe Gaudreau made changes -
          Testing Instructions * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 5 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          * *You should see the three embedded Youtube videos in the labels of the course*
          * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 5 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://www.youtube.com/watch?v=OMYzsAurxHY&d=200x200
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY?d=200x200
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY?d=200x200
          * *You should see the three embedded Youtube videos in the labels of the course*
          Hide
          Eloy Lafuente (stronk7) added a comment -

          What do you get if you test these with your patch:

          http://www.youtube.com/watch?v=OMYzsAurxHY#d=200x200
          http://youtu.be/OMYzsAurxHY#d=200x200

          (note I've not tried, just imagined they will cause the $videoid to be 100% borked, because it causes the match to return one more captured element with the dimensions, so your end($matches) will be getting the dimensions, not the correct videoid, that will be at $matches[3] or wherever).

          Show
          Eloy Lafuente (stronk7) added a comment - What do you get if you test these with your patch: http://www.youtube.com/watch?v=OMYzsAurxHY#d=200x200 http://youtu.be/OMYzsAurxHY#d=200x200 (note I've not tried, just imagined they will cause the $videoid to be 100% borked, because it causes the match to return one more captured element with the dimensions, so your end($matches) will be getting the dimensions, not the correct videoid, that will be at $matches [3] or wherever).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And yes, it seems that dimensions handling is anything but perfect along medialib.

          Show
          Eloy Lafuente (stronk7) added a comment - And yes, it seems that dimensions handling is anything but perfect along medialib.
          Hide
          Jean-Philippe Gaudreau added a comment -

          The 2 above URLs are working (right dimensions) with the current patch.

          Cheers!

          Show
          Jean-Philippe Gaudreau added a comment - The 2 above URLs are working (right dimensions) with the current patch. Cheers!
          Jean-Philippe Gaudreau made changes -
          Testing Instructions * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 5 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://www.youtube.com/watch?v=OMYzsAurxHY&d=200x200
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY?d=200x200
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY?d=200x200
          * *You should see the three embedded Youtube videos in the labels of the course*
          * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 5 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://www.youtube.com/watch?v=OMYzsAurxHY#d=200x200
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY#d=200x200
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY#d=200x200
          * *You should see the three embedded Youtube videos in the labels of the course*
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Then I don't understand what the hell is core_media_player_external::END_LINK_REGEX_PART adding to the regexp if it does not lead to more $matches[] being picked. I'll have to look code in context after all...

          Thanks for your patience...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Then I don't understand what the hell is core_media_player_external::END_LINK_REGEX_PART adding to the regexp if it does not lead to more $matches[] being picked. I'll have to look code in context after all... Thanks for your patience...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, got it. Those dimensions are stripped from the URLs at the beginning by core_media::split_alternatives(), so they never match later with the END_LINK_REGEX_PART regexp (what makes me think why the hell we do have it there).

          So end($matches) is always the correct $videoid apparently... oki re-integrating.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, got it. Those dimensions are stripped from the URLs at the beginning by core_media::split_alternatives(), so they never match later with the END_LINK_REGEX_PART regexp (what makes me think why the hell we do have it there). So end($matches) is always the correct $videoid apparently... oki re-integrating. Ciao
          Eloy Lafuente (stronk7) made changes -
          Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Currently in integration Yes [ 10041 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

          Note: I've added one extra commit to 22_STABLE about to support dimensions for shortened URLs there. Plz, check:

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=70206bbda8b58ce242bba534797eb3570c9463c1

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks! Note: I've added one extra commit to 22_STABLE about to support dimensions for shortened URLs there. Plz, check: http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=70206bbda8b58ce242bba534797eb3570c9463c1 Ciao
          Eloy Lafuente (stronk7) made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Affects Version/s 2.4 [ 12255 ]
          Fix Version/s 2.2.5 [ 12352 ]
          Fix Version/s 2.3.2 [ 12353 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Eloy Lafuente (stronk7) made changes -
          Testing Instructions * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 5 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://www.youtube.com/watch?v=OMYzsAurxHY#d=200x200
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY#d=200x200
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY#d=200x200
          * *You should see the three embedded Youtube videos in the labels of the course*
          NOTE: This needs separated testing on each branch, because all solutions are different.

          * Go to "Site administration > Plugins > Filters > Manage filters" and set to "On" the Mutlimedia plugins filter.
          * Go into a course and add 8 different labels with to following URL in the label text :
          ** http://www.youtube.com/watch?v=OMYzsAurxHY
          ** http://www.youtube.com/watch?v=OMYzsAurxHY#d=200x200
          ** http://youtu.be/OMYzsAurxHY
          ** http://www.youtu.be/OMYzsAurxHY
          ** http://youtu.be/OMYzsAurxHY#d=200x200
          ** http://y2u.be/OMYzsAurxHY
          ** http://www.y2u.be/OMYzsAurxHY
          ** http://y2u.be/OMYzsAurxHY#d=200x200
          * *You should see the 8 embedded Youtube videos in the labels of the course* (with those specifying dimensions "d=200x200" being shown with that size).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amended testing instructions and created MDL-35253 about to improve filter_mediaplugin unit tests.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Amended testing instructions and created MDL-35253 about to improve filter_mediaplugin unit tests. Ciao
          Eloy Lafuente (stronk7) made changes -
          Link This issue testing discovered MDL-35253 [ MDL-35253 ]
          Michael de Raadt made changes -
          Tester ankit_frenz
          Hide
          Frédéric Massart added a comment -

          About the dimensions, I've tried it too, it works with the param ?d=100x100 not &d=100x100. Don't know if it's expected.

          Show
          Frédéric Massart added a comment - About the dimensions, I've tried it too, it works with the param ?d=100x100 not &d=100x100. Don't know if it's expected.
          Ankit Agarwal made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          Ankit Agarwal added a comment -

          works as expected.
          Thanks

          Show
          Ankit Agarwal added a comment - works as expected. Thanks
          Ankit Agarwal made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, I think that, somehow, the &d= is deprecated/not supported in favor of the #d=, after looking how medialib behaves with URLs.

          So when reviewing this I decided to ignore that, as far as the goal for this was to get short youtube urls supported, and that is apparently achieved.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, I think that, somehow, the &d= is deprecated/not supported in favor of the #d=, after looking how medialib behaves with URLs. So when reviewing this I decided to ignore that, as far as the goal for this was to get short youtube urls supported, and that is apparently achieved. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work.

          These changes have been spread upstream and are already available in the git and cvs repositories.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao
          Eloy Lafuente (stronk7) made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 07/Sep/12

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: