Moodle
  1. Moodle
  2. MDL-36641

External SCORM manifest URL checks only for syntax

    Details

    • Testing Instructions:
      Hide
      1. Get a SCORM Package (zip file)
      2. Unzip it and place it in a folder reachable by HTTP (you can leave the zip itself within that folder)
      3. Take note of the URL pointing to the imsmanifest.xml file , supposing here to be http://hostname/path/to/resource.imsmanifest.xml
      4. Enable allowtypeexternal* in the SCORM Activity settings (/admin/settings.php?section=modsettingscorm)
      5. Create a SCORM activity selecting the Type among the External ones (AICC included)
      6. Paste the URL above in URL field
        • Add a suffix to the URL e.g. .external and click on Save and display: expect Invalid URL specified
        • Change something in the URL appending e.g. .external to /path (=>/path.external/} and click on Save and display: expect Invalid URL specified. Debug message: followed by a preformatted text telling what's going on
      7. Change the Type with the other External and try again the previous step
      8. Select Type equal to External SCORM manifest
      9. Paste the URL, the right one
      10. Click on Save and display: expect the package having been published, play with it just to see the TOC and click on one of the items
      Show
      Get a SCORM Package (zip file) Unzip it and place it in a folder reachable by HTTP (you can leave the zip itself within that folder) Take note of the URL pointing to the imsmanifest.xml file , supposing here to be http://hostname/path/to/resource.imsmanifest.xml Enable allowtypeexternal* in the SCORM Activity settings ( /admin/settings.php?section=modsettingscorm ) Create a SCORM activity selecting the Type among the External ones (AICC included) Paste the URL above in URL field Add a suffix to the URL e.g. .external and click on Save and display : expect Invalid URL specified Change something in the URL appending e.g. .external to /path (=> /path.external/ } and click on Save and display : expect Invalid URL specified. Debug message: followed by a preformatted text telling what's going on Change the Type with the other External and try again the previous step Select Type equal to External SCORM manifest Paste the URL, the right one Click on Save and display : expect the package having been published, play with it just to see the TOC and click on one of the items
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m24_MDL-36641_Missing_external_package_HTTP_check

      Gliffy Diagrams

        Activity

        Hide
        Matteo Scaramuccia added a comment -

        Here is my patch proposal: it makes usage of an HTTP HEAD call which is used in few other place so it's safe to adopt it here too. The debug message cannot be simplified into a set of predefined error messages, to help the troubleshooting.

        Now, I'll look at the issue in the forum (where everything started): it could be this issue not related with the original post but with Dan's reply.

        Show
        Matteo Scaramuccia added a comment - Here is my patch proposal: it makes usage of an HTTP HEAD call which is used in few other place so it's safe to adopt it here too. The debug message cannot be simplified into a set of predefined error messages, to help the troubleshooting. Now, I'll look at the issue in the forum (where everything started): it could be this issue not related with the original post but with Dan's reply.
        Hide
        Matteo Scaramuccia added a comment -

        Fixed a small typo (use => used) in a comment inside scorm_check_url().

        Show
        Matteo Scaramuccia added a comment - Fixed a small typo ( use => used ) in a comment inside scorm_check_url() .
        Hide
        Matteo Scaramuccia added a comment -

        Damn' flu! Just realized that:

        1. the branch name was incorrect
        2. more important the coding style for the nested if was tremendous :

          +            } else {
          +                // Availability check.
          +                if ($errormsg = scorm_check_url($reference)) {
          +                    $errors['packageurl'] = $errormsg;
          +                }
                       }
          

        Show
        Matteo Scaramuccia added a comment - Damn' flu! Just realized that: the branch name was incorrect more important the coding style for the nested if was tremendous : + } else { + // Availability check. + if ($errormsg = scorm_check_url($reference)) { + $errors['packageurl'] = $errormsg; + } }
        Hide
        Dan Marsden added a comment -

        thanks Matteo - took a quick look and it looks ok - couple of extra carriage returns in the patch that might not be needed but I haven't actually looked at the full code so it might make sense to have those in the patch? - I'll try to do a proper peer review on this later this week - if I haven't done it by Thurs ping me again to take a look!

        Show
        Dan Marsden added a comment - thanks Matteo - took a quick look and it looks ok - couple of extra carriage returns in the patch that might not be needed but I haven't actually looked at the full code so it might make sense to have those in the patch? - I'll try to do a proper peer review on this later this week - if I haven't done it by Thurs ping me again to take a look!
        Hide
        Matteo Scaramuccia added a comment -

        Ping!
        L2R419 and L2R429 are there according to the style used for the previous check "blocks" i.e added intentionally.

        Show
        Matteo Scaramuccia added a comment - Ping! L2R419 and L2R429 are there according to the style used for the previous check "blocks" i.e added intentionally.
        Hide
        Dan Marsden added a comment -

        looking again..

        not sure what I think about returning false on success? - my personal preference for functions would be to return true when no error is found - although I'm not sure what the integrators think about that.

        Show
        Dan Marsden added a comment - looking again.. not sure what I think about returning false on success? - my personal preference for functions would be to return true when no error is found - although I'm not sure what the integrators think about that.
        Hide
        Dan Marsden added a comment -

        yeah - just had a chat with Sam H - returning true or error string would be more consistent with the rest of Moodle rather than false or error string - would you mind changing that? - thanks!

        Show
        Dan Marsden added a comment - yeah - just had a chat with Sam H - returning true or error string would be more consistent with the rest of Moodle rather than false or error string - would you mind changing that? - thanks!
        Hide
        Matteo Scaramuccia added a comment -

        OK
        false|string was selected to let the code consuming be compact enough: BTW, I'll do the required changes.

        Show
        Matteo Scaramuccia added a comment - OK false|string was selected to let the code consuming be compact enough: BTW, I'll do the required changes.
        Hide
        Matteo Scaramuccia added a comment -

        Hi Dan,
        here is the code using true|string, rebased against 2.4beta+ (Build: 20121123).

        Show
        Matteo Scaramuccia added a comment - Hi Dan, here is the code using true|string , rebased against 2.4beta+ (Build: 20121123) .
        Hide
        Dan Marsden added a comment -

        cool - looks ok to me - bouncing up for integration. not completely sure about the is_string() stuff but will leave the integrator to make a call on that. Thanks Matteo!

        Show
        Dan Marsden added a comment - cool - looks ok to me - bouncing up for integration. not completely sure about the is_string() stuff but will leave the integrator to make a call on that. Thanks Matteo!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (24 and master).

        I don't think we should be backporting this to older stables, as far as may be considered an improvement.

        Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (24 and master). I don't think we should be backporting this to older stables, as far as may be considered an improvement. Thanks!
        Hide
        Ankit Agarwal added a comment -

        works as described.
        passing
        Thanks

        Show
        Ankit Agarwal added a comment - works as described. passing Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: