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
        • Pull Master Diff URL:

          Gliffy Diagrams

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            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
            Permalink
            Ankit Agarwal added a comment -

            works as described.
            passing
            Thanks

            Show
            Ankit Agarwal added a comment - works as described. passing Thanks
            Hide
            Permalink
            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: