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
    • Rank:
      46136

      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: