Moodle
  1. Moodle
  2. MDL-25646

html_writer is used in mediaplugin but is not defined

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.10
    • Fix Version/s: 1.9.11
    • Component/s: Filters
    • Labels:
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      15256

      Description

      Issue details:

      The filter_mediaplugin_ogg_callback(...) and filter_mediaplugin_ogv_callback(...) methods in filter/mediaplugin/filter.php reference the html_writer class, which is not defined in Moodle 1.9.10+ (build 20101103). This causes a fatal PHP error when those methods are called.

      Solution details:

      We added this custom class:

      /usr/share/php/html_writer.php:
      <?php
      class html_writer {
      function link($url, $text)

      { return '<a href="' . $url . '">' . $text . '</a>'; }

      }
      ?>

      (/usr/share/php is in our installation's include_path.)

      Then we added this to our Moodle installation's config.php:

      require_once 'html_writer.php';

      We chose to write this code instead of fully backporting the html_writer class and other required functions from Moodle 2.0 because only this code appears to be needed at this time.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Confirmed. One quick look to filter/mediaplugin/filter.php reveals a lot of 2.0 stuff into the 1.9 version ($PAGE, $OUTPUT, html_writer...).

          Looking logs it seems that MDL-19927 was the issue were this wrong backport happened.

          Raising to Blocker and moving to the STABLE backlog to be fixed ASAP.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Confirmed. One quick look to filter/mediaplugin/filter.php reveals a lot of 2.0 stuff into the 1.9 version ($PAGE, $OUTPUT, html_writer...). Looking logs it seems that MDL-19927 was the issue were this wrong backport happened. Raising to Blocker and moving to the STABLE backlog to be fixed ASAP. Ciao
          Hide
          Rossiani Wijaya added a comment -

          fixed issue and submitted pull request (PULL-11).

          Show
          Rossiani Wijaya added a comment - fixed issue and submitted pull request (PULL-11).
          Hide
          Rossiani Wijaya added a comment -

          Reopen.

          Eloy's comments on Pull request:

          the get_string() calls are incorrect for 1.9

          it seems that, with the patch, something has changed: originally the $url used to build the $printlink was "addslashed" and, after the patch, it isn't any more. I don't know if needs to be or no but, in any case, is one change in behavior of original code.
          Easy to fix! Thanks!

          Show
          Rossiani Wijaya added a comment - Reopen. Eloy's comments on Pull request: the get_string() calls are incorrect for 1.9 it seems that, with the patch, something has changed: originally the $url used to build the $printlink was "addslashed" and, after the patch, it isn't any more. I don't know if needs to be or no but, in any case, is one change in behavior of original code. Easy to fix! Thanks!
          Hide
          Rossiani Wijaya added a comment - - edited

          Creating patch to address Eloy's comments.

          add Eloy to watcher list

          Hi Eloy,
          1. fixed get_string parameter issue.
          2. The value of $url is the url of the file.
          The value of $printlink contains link to the file. eg <a href='somepage.com/sample.ogv'>sample's file</a>
          If addslashes is used, the hyperlink will appear as sample\'s file.
          Thats why only $url is set to use addslashes.
          Both values are generated from mediaplugin_filter function.

          Patch is available at: https://github.com/rwijaya/moodle/commit/ddd0eab19e971c5c29f06fa2c96864e11961f1df

          Please take a look the patch and leave me any comments.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - - edited Creating patch to address Eloy's comments. add Eloy to watcher list Hi Eloy, 1. fixed get_string parameter issue. 2. The value of $url is the url of the file. The value of $printlink contains link to the file. eg <a href='somepage.com/sample.ogv'>sample's file</a> If addslashes is used, the hyperlink will appear as sample\'s file. Thats why only $url is set to use addslashes. Both values are generated from mediaplugin_filter function. Patch is available at: https://github.com/rwijaya/moodle/commit/ddd0eab19e971c5c29f06fa2c96864e11961f1df Please take a look the patch and leave me any comments. Thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          just for reference: we were discussing about the addslashes_js() thingy and also about to wrap the html5 audio/video tags with spans (like other formats in the same filter do).

          Show
          Eloy Lafuente (stronk7) added a comment - just for reference: we were discussing about the addslashes_js() thingy and also about to wrap the html5 audio/video tags with spans (like other formats in the same filter do).
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy,

          I made the changes per our discussion. Please take a look the patch and let me know if you want to change anything.

          patch: https://github.com/rwijaya/moodle/commit/2835bc8f516786b08ac9e988a3641e162ba0bdf5

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Eloy, I made the changes per our discussion. Please take a look the patch and let me know if you want to change anything. patch: https://github.com/rwijaya/moodle/commit/2835bc8f516786b08ac9e988a3641e162ba0bdf5 Thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks ok IMO:

          • container spans added where possible
          • better urls for all types.

          But with one concern, when building $printlink for all types, i.e. this line:

          $printlink = '<a href="'.$url.'">'.get_string('realaudio', 'mediaplugin').'</a>';
          

          THAT $url, needs to be addslashed or no? I think we agreed that the $url needed to be addslahed if they were going to be used in javascript, but not in other contexts (like normal link tags).

          So perhaps, unless there were some reason that I've forgotten... we should calculate $printlink BEFORE addslashes_js() the $url. Then, when necessary (going to print javascript), addslashes_js() the $url and build the "safe" javascript.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Looks ok IMO: container spans added where possible better urls for all types. But with one concern, when building $printlink for all types, i.e. this line: $printlink = '<a href= "'.$url.'" >'.get_string('realaudio', 'mediaplugin').'</a>'; THAT $url, needs to be addslashed or no? I think we agreed that the $url needed to be addslahed if they were going to be used in javascript, but not in other contexts (like normal link tags). So perhaps, unless there were some reason that I've forgotten... we should calculate $printlink BEFORE addslashes_js() the $url. Then, when necessary (going to print javascript), addslashes_js() the $url and build the "safe" javascript. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Eloy,

          Based on our conversation you wrote:
          (17:37:33) stronk7@contiento.com: bah, don't worry. just addslashes to link[1], build the <a> tag manually and done.

          Addslashes is also used in 2.0. Therefore we are agreed to keep addslashes to link[1].

          Please let me know if $printlink url should not use addslashes.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Eloy, Based on our conversation you wrote: (17:37:33) stronk7@contiento.com: bah, don't worry. just addslashes to link [1] , build the <a> tag manually and done. Addslashes is also used in 2.0. Therefore we are agreed to keep addslashes to link [1] . Please let me know if $printlink url should not use addslashes. Thanks Rosie
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rosie,

          if $printlink is being outputted as HTML I think there is NO reason for addslash-ing anything on it at all. So the URL must not be slashed there IMO.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rosie, if $printlink is being outputted as HTML I think there is NO reason for addslash-ing anything on it at all. So the URL must not be slashed there IMO. Ciao
          Hide
          Rossiani Wijaya added a comment -

          Patch to fixed url addslashes is available at: https://github.com/rwijaya/moodle/compare/MOODLE_19_STABLE...MDL-25646_m19

          Created pull request: PULL-117

          Show
          Rossiani Wijaya added a comment - Patch to fixed url addslashes is available at: https://github.com/rwijaya/moodle/compare/MOODLE_19_STABLE...MDL-25646_m19 Created pull request: PULL-117
          Hide
          Petr Škoda added a comment -

          committed, thanks

          Show
          Petr Škoda added a comment - committed, thanks
          Hide
          Tim Hunt added a comment -

          Was it intentional that this change added the words 'MP3 Audio' visible to the users? It is annoying people. We reverted it at the OU, and see http://moodle.org/mod/forum/discuss.php?d=172476 for someone else who does not like it.

          Show
          Tim Hunt added a comment - Was it intentional that this change added the words 'MP3 Audio' visible to the users? It is annoying people. We reverted it at the OU, and see http://moodle.org/mod/forum/discuss.php?d=172476 for someone else who does not like it.
          Hide
          Tim Hunt added a comment -

          I see. MDL-26605 is where this is being discussed.

          Show
          Tim Hunt added a comment - I see. MDL-26605 is where this is being discussed.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: