Moodle
  1. Moodle
  2. MDL-33116

new media embedding seems incompatible with slasharguments off

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Filters
    • Labels:
    • Testing Instructions:
      Hide

      0. To begin with, in admin settings, set/leave option 'slasharguments' to on.
      1. If necessary, restore the test course from MDL-29624.
      2. In the test course, click on 'Page (filter testing)'

      • Various items should display embedded.

      3. In admin settings, change slasharguments to off.
      4. Load the same page from test course again (e.g. in another tab) and compare the results.

      • The same items should be embedded.

      5. Run unit tests like 'phpunit x lib/tests/medialib_test.php' (or full tests).

      Actual behaviour before fixing bug: in step 4, only the YouTube item is now embeddded, everything else has degraded to a download link. The unit tests pass, but don't cover this situation.

      Show
      0. To begin with, in admin settings, set/leave option 'slasharguments' to on. 1. If necessary, restore the test course from MDL-29624 . 2. In the test course, click on 'Page (filter testing)' Various items should display embedded. 3. In admin settings, change slasharguments to off. 4. Load the same page from test course again (e.g. in another tab) and compare the results. The same items should be embedded. 5. Run unit tests like 'phpunit x lib/tests/medialib_test.php' (or full tests). Actual behaviour before fixing bug: in step 4, only the YouTube item is now embeddded, everything else has degraded to a download link. The unit tests pass, but don't cover this situation.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-33116-master
    • Rank:
      40379

      Description

      If you disable slasharguments the media embedding stops working in most places.

      To integrators: This needs to be integrated before MDL-33145.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Yes, it would be good to have this fixed before release.

          Show
          Michael de Raadt added a comment - Yes, it would be good to have this fixed before release.
          Hide
          Sam Marshall added a comment -

          OK, looking at it.

          Show
          Sam Marshall added a comment - OK, looking at it.
          Hide
          Sam Marshall added a comment -

          This was intended to work at some point (I can see things I did specifically to take it into account), but obviously I forgot about it before testing.

          I have now written additional unit tests that cover basic use of slash arguments, and fixed the bug. Added new get_param function to moodle_url to do this, also improved documentation of existing get_path function to add a warning about slash arguments there.

          Show
          Sam Marshall added a comment - This was intended to work at some point (I can see things I did specifically to take it into account), but obviously I forgot about it before testing. I have now written additional unit tests that cover basic use of slash arguments, and fixed the bug. Added new get_param function to moodle_url to do this, also improved documentation of existing get_path function to add a warning about slash arguments there.
          Hide
          Sam Marshall added a comment -

          Submitting for review.

          Show
          Sam Marshall added a comment - Submitting for review.
          Hide
          Petr Škoda added a comment -

          Hmm, what happens with external files? And what if somebody still has legacy file.php links and switches the slasharguments? Previously it used to work for both types at the same time just ignoring (?file=). I am also a bit confused by the exclusive use of moodle_url, I would be a bit afraid that it is not fully compatible with general URLs that might be typed manually in the html editor.

          Show
          Petr Škoda added a comment - Hmm, what happens with external files? And what if somebody still has legacy file.php links and switches the slasharguments? Previously it used to work for both types at the same time just ignoring (?file=). I am also a bit confused by the exclusive use of moodle_url, I would be a bit afraid that it is not fully compatible with general URLs that might be typed manually in the html editor.
          Hide
          Sam Marshall added a comment -

          1) Re legacy file.php links and working for both at once regardless of setting - very good point, I will fix this and then request review again.

          2) You can embed external files, afaik (test script uses example.org for everything rather than bothering to put wwwroot on the front). I thought this was supposed to work. If I'm wrong and it's not, please file another issue and I'll fix it separately, as it's not related to this one.

          3) There isn't any comment in moodle_url (class comment or constructor comment) to suggest you can't use it with general URLs. Afaik it works fine with any URL, just has shortcuts for creating ones to current Moodle, and I prefer using 'typed' parameters so moodle_url seemed best. That said, I guess what you're pointing out is the way it throws exception if PHP parse_url returns false? Maybe there would be a way to make this crash the filter - do you happen to know how to do this? I tried to google for things that make parse_url fail but didn't find it. (Again this is a separate issue.)

          Show
          Sam Marshall added a comment - 1) Re legacy file.php links and working for both at once regardless of setting - very good point, I will fix this and then request review again. 2) You can embed external files, afaik (test script uses example.org for everything rather than bothering to put wwwroot on the front). I thought this was supposed to work. If I'm wrong and it's not, please file another issue and I'll fix it separately, as it's not related to this one. 3) There isn't any comment in moodle_url (class comment or constructor comment) to suggest you can't use it with general URLs. Afaik it works fine with any URL, just has shortcuts for creating ones to current Moodle, and I prefer using 'typed' parameters so moodle_url seemed best. That said, I guess what you're pointing out is the way it throws exception if PHP parse_url returns false? Maybe there would be a way to make this crash the filter - do you happen to know how to do this? I tried to google for things that make parse_url fail but didn't find it. (Again this is a separate issue.)
          Hide
          Sam Marshall added a comment -

          Fixed, ready for review again. Matching now does not depend at all on current state of $CFG->slasharguments.

          Show
          Sam Marshall added a comment - Fixed, ready for review again. Matching now does not depend at all on current state of $CFG->slasharguments.
          Hide
          Petr Škoda added a comment -

          moodle_url has the 'moodle' word in it because it was intended mainly for wwwroot urls, there is no guarantee it works for all standard compliant URLs - especially passing of URL string to constructor is problematic, it may work for simple alphanumeric parameters but it breaks for non php slasharguments, encoded characters in params and paths, arrays, etc.

          In short: if you specify $params and explicitly set slashargument part it works perfectly, but constructing moodle_url from string is not fully implemented. My problem here is that if you expect only moodle_url how do you create them properly from html in filters?

          Show
          Petr Škoda added a comment - moodle_url has the 'moodle' word in it because it was intended mainly for wwwroot urls, there is no guarantee it works for all standard compliant URLs - especially passing of URL string to constructor is problematic, it may work for simple alphanumeric parameters but it breaks for non php slasharguments, encoded characters in params and paths, arrays, etc. In short: if you specify $params and explicitly set slashargument part it works perfectly, but constructing moodle_url from string is not fully implemented. My problem here is that if you expect only moodle_url how do you create them properly from html in filters?
          Hide
          Sam Marshall added a comment -

          Yes the filter constructs from string. OK I have filed MDL-33145 about that.

          Show
          Sam Marshall added a comment - Yes the filter constructs from string. OK I have filed MDL-33145 about that.
          Hide
          Dan Poltawski added a comment -

          I'm pushing the integration button on this because it looks like Petr has been reviewing it. Also we need to get these media embeding regressions under control.

          Show
          Dan Poltawski added a comment - I'm pushing the integration button on this because it looks like Petr has been reviewing it. Also we need to get these media embeding regressions under control.
          Hide
          Dan Poltawski added a comment -

          I've integrated this. I'm going to test it.

          Show
          Dan Poltawski added a comment - I've integrated this. I'm going to test it.
          Hide
          Dan Poltawski added a comment -

          Tested and passed looks good.

          Thanks Petr for the quick spotting and Sam for the quick fix!

          Show
          Dan Poltawski added a comment - Tested and passed looks good. Thanks Petr for the quick spotting and Sam for the quick fix!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: