Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-25646

html_writer is used in mediaplugin but is not defined

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 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
            stronk7 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
            rwijaya Rossiani Wijaya added a comment -

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

            Show
            rwijaya Rossiani Wijaya added a comment - fixed issue and submitted pull request (PULL-11).
            Hide
            rwijaya 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
            rwijaya 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
            rwijaya 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
            rwijaya 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
            stronk7 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
            stronk7 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
            rwijaya 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
            rwijaya 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
            stronk7 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
            stronk7 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
            rwijaya 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
            rwijaya 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
            stronk7 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
            stronk7 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
            rwijaya 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
            rwijaya 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
            skodak Petr Skoda added a comment -

            committed, thanks

            Show
            skodak Petr Skoda added a comment - committed, thanks
            Hide
            timhunt 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
            timhunt 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
            timhunt Tim Hunt added a comment -

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

            Show
            timhunt 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:
                  Fix Release Date:
                  21/Feb/11