Moodle
  1. Moodle
  2. MDL-23119

LmsSetValue with cmi.objectives.n.id may return "Incorrect data type" and/or "Invalid argument error"

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.9
    • Fix Version/s: 1.9.10
    • Component/s: SCORM
    • Labels:
      None
    • Database:
      MySQL
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      26891

      Description

      When using 1.2 SCORM packages where a LmsSetValue is issued using cmi.objectives.n.id is sent, under certain circumstances errors are thrown. Up to now we reproduced the following:

      1. "Incorrect data type" (405) is always thrown when cmi.objectives.n.id i set to alphanumeric id containing blank spaces (that could be a limit of the 1.2 scorm API ?)

      2. "Incorrect data type" is also thrown sometimes when N > 10. That is not alway reproducible but it happens for id with and without spaces.

      3. When incorrect data type is thrown using an id label without space, setting the value by the SCORM API debugger yields "Invalid argument error" (201).

      We have a test environment to check it.

        Activity

        Hide
        Dan Marsden added a comment - - edited

        Hi Andrea,

        as far as I can see this is correct as per the ADL spec:

        The SCORM Spec states for CMIIdentifier (type for cmi.objectives.n.id)

        • Value shall contain an alphanumeric group of characters
        • Value shall contain no white space
        • Value shall contain no unprintable characters
        • Value's length shall not exceed 255 characters

        Point 2 could be related to this part of the spec:
        6.3.5 Accept only elements where the list index n is less than or equal to the number of elements currently in the list
        ADL Note: Lists in the SCORM Run-Time Environment Data Model are 0-based.
        Therefore, if the list contains 4 items the last position in the list has the index of 3. The
        total size of the list is therefore the index of the first/next available empty position.

        If you are able to provide more specific information about point 2/3 and it is not the expected behaviour as mentioned in the spec, please provide more information and we can reopen this bug.

        thanks.

        Show
        Dan Marsden added a comment - - edited Hi Andrea, as far as I can see this is correct as per the ADL spec: The SCORM Spec states for CMIIdentifier (type for cmi.objectives.n.id) Value shall contain an alphanumeric group of characters Value shall contain no white space Value shall contain no unprintable characters Value's length shall not exceed 255 characters Point 2 could be related to this part of the spec: 6.3.5 Accept only elements where the list index n is less than or equal to the number of elements currently in the list ADL Note: Lists in the SCORM Run-Time Environment Data Model are 0-based. Therefore, if the list contains 4 items the last position in the list has the index of 3. The total size of the list is therefore the index of the first/next available empty position. If you are able to provide more specific information about point 2/3 and it is not the expected behaviour as mentioned in the spec, please provide more information and we can reopen this bug. thanks.
        Hide
        Andrea Bicciolo added a comment -

        Dan,

        thanks for confirming me point 1. Just wondering if you are aware of a workaround different from repackagng SCORM?

        Show
        Andrea Bicciolo added a comment - Dan, thanks for confirming me point 1. Just wondering if you are aware of a workaround different from repackagng SCORM?
        Hide
        Dan Marsden added a comment -

        No sorry - The LMS SCORM 1.2 tests would probably fail if we relaxed the requirement and we would no longer be able to gain certification.

        SCORM is pretty complex and we could constantly be chasing our tail to provide workarounds for different buggy packages (we've already added some new settings to 2.0 to help with buggy grading calls in packages) We could "potentially" create a "compatibility mode" option in scorm that ignored this sort of thing but I'd only add this if someone else created the patch - I'd want it set it up as an Admin option - "allow compatibility mode for buggy packages" (disabled by detault) - and then allow an extra option on the scorm settings page "enable compatibility mode" - turned off by default.

        This new "option" would need to be well documented in the wiki somewhere stating exactly what changes it made to the running of SCORM objects.

        Show
        Dan Marsden added a comment - No sorry - The LMS SCORM 1.2 tests would probably fail if we relaxed the requirement and we would no longer be able to gain certification. SCORM is pretty complex and we could constantly be chasing our tail to provide workarounds for different buggy packages (we've already added some new settings to 2.0 to help with buggy grading calls in packages) We could "potentially" create a "compatibility mode" option in scorm that ignored this sort of thing but I'd only add this if someone else created the patch - I'd want it set it up as an Admin option - "allow compatibility mode for buggy packages" (disabled by detault) - and then allow an extra option on the scorm settings page "enable compatibility mode" - turned off by default. This new "option" would need to be well documented in the wiki somewhere stating exactly what changes it made to the running of SCORM objects.
        Hide
        Andrea Bicciolo added a comment -

        Thans Dan. Agree with you, better avoid to create workaround to buggy packages. However a compatibility mode would be something to think at in the future, as there are plenty of buggy packages around.

        Show
        Andrea Bicciolo added a comment - Thans Dan. Agree with you, better avoid to create workaround to buggy packages. However a compatibility mode would be something to think at in the future, as there are plenty of buggy packages around.
        Hide
        Andrea Bicciolo added a comment -

        Dan,

        the SCORM package we are testing and where the problem described is experienced, claims to send a " " (non breaking space) rather than a standard blank space. Do you think   should be not considered a white space 1.2 SCORM API or it considered the same as a blank space ?

        Show
        Andrea Bicciolo added a comment - Dan, the SCORM package we are testing and where the problem described is experienced, claims to send a " " (non breaking space) rather than a standard blank space. Do you think   should be not considered a white space 1.2 SCORM API or it considered the same as a blank space ?
        Hide
        Andrea Bicciolo added a comment -

        Well, the blank spaces not visible on the message are actually non-breaking space. "& nbsp;"

        Show
        Andrea Bicciolo added a comment - Well, the blank spaces not visible on the message are actually non-breaking space. "& nbsp;"
        Hide
        Andrea Bicciolo added a comment -

        Dan,

        we tested against scorm test track from Rustici. Id are accepted when using spaces (& nbsp). Probably it should worth a check?

        Show
        Andrea Bicciolo added a comment - Dan, we tested against scorm test track from Rustici. Id are accepted when using spaces (& nbsp). Probably it should worth a check?
        Hide
        Dan Marsden added a comment -

        well - I'd interpret "Value shall contain no white space" to include using nbsp - seems like a way to try to "get-around" the spec rather than a valid use of cmi.objectives.n.id - I'll think on this a bit more and get back to you with a "decision"

        Show
        Dan Marsden added a comment - well - I'd interpret "Value shall contain no white space" to include using nbsp - seems like a way to try to "get-around" the spec rather than a valid use of cmi.objectives.n.id - I'll think on this a bit more and get back to you with a "decision"
        Hide
        Dan Marsden added a comment -

        I've had another look at how we validate CMIIdentifier values and had a quick e-mail conversation with the Rustici guys..

        Moodle currently uses this regex:
        ^
        w

        {1,255}$

        Which is basically equivalent to: [a-zA-Z0-9_] and 1 to 255 chars long

        note - that expression doesn't include & or ; and the SCORM Api docs don't specifically state what they mean by "Alphanumeric" - most of the technical uses of the term Alphanumeric appear to me to be A-Z 0-9 and _ excluding special characters.

        I suppose we could relax this restriction to include & and ; but I don't think we should be adding support for !@#$%^*() as The SCORM API docs do differentiate between Alphanumeric and ASCII - if the intention of ADL was to allow all these chars then I would think they would have specified "ASCII" characters excluding white space and non-printable characters. Instead they have specified "Alphanumeric"

        here's a possible replacement regex to include & and ;
        ^[a-zA-Z0-9_&;]{1,255}

        $

        thoughts?

        Show
        Dan Marsden added a comment - I've had another look at how we validate CMIIdentifier values and had a quick e-mail conversation with the Rustici guys.. Moodle currently uses this regex: ^ w {1,255}$ Which is basically equivalent to: [a-zA-Z0-9_] and 1 to 255 chars long note - that expression doesn't include & or ; and the SCORM Api docs don't specifically state what they mean by "Alphanumeric" - most of the technical uses of the term Alphanumeric appear to me to be A-Z 0-9 and _ excluding special characters. I suppose we could relax this restriction to include & and ; but I don't think we should be adding support for !@#$%^*() as The SCORM API docs do differentiate between Alphanumeric and ASCII - if the intention of ADL was to allow all these chars then I would think they would have specified "ASCII" characters excluding white space and non-printable characters. Instead they have specified "Alphanumeric" here's a possible replacement regex to include & and ; ^ [a-zA-Z0-9_&;] {1,255} $ thoughts?
        Hide
        Dan Marsden added a comment - - edited

        I suppose (if it still passes the ADL tests) we could extend the regex to this, which is pretty much all characters on a keyboard excluding the non-printable ones.....

         
        CMIIdentifier = '^[\\u0021-\\u007E]{0,255}$';
        
        
        Show
        Dan Marsden added a comment - - edited I suppose (if it still passes the ADL tests) we could extend the regex to this, which is pretty much all characters on a keyboard excluding the non-printable ones..... CMIIdentifier = '^[\\u0021-\\u007E]{0,255}$';
        Hide
        Andrea Bicciolo added a comment -

        Dan,

        thanks for looking at this. If the new regex will allow for nbsp, it could be a good improvement. However, only if it also pass ADL test: in my opinion it should pass, as the Rustici is a certified ADL SCORM engine and it is accepting nbsp.

        Show
        Andrea Bicciolo added a comment - Dan, thanks for looking at this. If the new regex will allow for nbsp, it could be a good improvement. However, only if it also pass ADL test: in my opinion it should pass, as the Rustici is a certified ADL SCORM engine and it is accepting nbsp.
        Hide
        Dan Marsden added a comment -

        Andrea - can you test the new Regex with your SCORM object to make sure it works with the content being passed? - it goes in mod/scorm/datamodels/scorm_12.js.php

        Show
        Dan Marsden added a comment - Andrea - can you test the new Regex with your SCORM object to make sure it works with the content being passed? - it goes in mod/scorm/datamodels/scorm_12.js.php
        Hide
        Andrea Bicciolo added a comment -

        Dan,

        testing the new regexp now, no "Incorrect data type"so far. SCORM API has a nice all-black log with plenty of succesfull LmsSetValue.

        Show
        Andrea Bicciolo added a comment - Dan, testing the new regexp now, no "Incorrect data type"so far. SCORM API has a nice all-black log with plenty of succesfull LmsSetValue.
        Hide
        Dan Marsden added a comment -

        Hi Andrea,

        I've pushed this fix into 1.9Stable and HEAD.

        It's important to note here that this patch extends the ADL Spec for CMIIdentifier - this usage is outside the SCORM Specification for this type of var - people should NOT rely on this behaviour in all SCORM players as it is likely that some will directly follow the spec and not be as flexible as we are to making changes to support extended characters.

        Show
        Dan Marsden added a comment - Hi Andrea, I've pushed this fix into 1.9Stable and HEAD. It's important to note here that this patch extends the ADL Spec for CMIIdentifier - this usage is outside the SCORM Specification for this type of var - people should NOT rely on this behaviour in all SCORM players as it is likely that some will directly follow the spec and not be as flexible as we are to making changes to support extended characters.
        Hide
        Andrea Bicciolo added a comment -

        Thanks DAN. I think you had clever approach.

        Show
        Andrea Bicciolo added a comment - Thanks DAN. I think you had clever approach.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: