Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2, 1.8.3
    • Fix Version/s: 1.8.6, 1.9.1
    • Component/s: SCORM
    • Labels:
      None
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      In the \moodle\mod\scorm\datamodels\scorm_12.js.php file at line 131 there is a PHP block designed to initialise the data model's objectives.

      However, the last check-in of this code introduced a bug whereby $subelement is always empty-string and the for..each block only matches the first objective initialisation. This is due to the regular expression exclusively matching full-stops (now \., previously .), rather than the underscore as captured by the data model from a previous session write-back (cmi.objectives_0.id). Also the replacement is performed after the regex matcher for the objective identifiers, introducing needless complexity. Consequently the following JS code is produced by api.php when revisiting a SCORM 1.2 package making use of the cmi.objectives data model section:

      = new Object();
      .score = new Object();
      .score._children = score_children;
      .score.raw = '';
      .score.min = '';
      .score.max = '';

      Note: There is no actual or intended 'with' block, nor should there be.

      I propose the following code fix:

      <?php
      $count = 0;
      $objectives = '';
      foreach($userdata as $element => $value) {
      if (substr($element, 0, 14) == 'cmi.objectives') {
      $element = preg_replace('/\.(\d+)\./', "_\$1.", $element);
      preg_match('/_(\d+)\./', $element, $matches);
      if ($matches[1] == $count)

      { $count++; $end = strpos($element, $matches[1]) + strlen($matches[1]); $subelement = substr($element, 0, $end); echo ' '.$subelement." = new Object();\n"; echo ' '.$subelement.".score = new Object();\n"; echo ' '.$subelement.".score._children = score_children;\n"; echo ' '.$subelement.".score.raw = '';\n"; echo ' '.$subelement.".score.min = '';\n"; echo ' '.$subelement.".score.max = '';\n"; }

      echo ' '.$element.' = \''.$value."';\n";
      }
      }
      if ($count > 0)

      { echo ' cmi.objectives._count = '.$count.";\n"; }

      ?>

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            assigning to me - will test fix and look at patching it.

            Dan

            Show
            danmarsden Dan Marsden added a comment - assigning to me - will test fix and look at patching it. Dan
            Hide
            poltawski Dan Poltawski added a comment -

            Ok, has taken me a bit of time to understand this issue. Many thanks to Peter for the info.

            For the benefit of others, here is a little code snippet to try and demonstrate what should be happening with the bad lines commented out:

            <?php
            $element = 'cmi.objectives_1.id';

            $count = 1;

            $element = preg_replace('/\.(\d+)\./', "_\$1.", $element);
            preg_match('/_(\d+)\./', $element, $matches);
            if ($matches[1] == $count) {
            #preg_match('/\.(\d+)\./',$element,$matches);
            #$element = preg_replace('/\.(\d+)\./',".\$1.",$element);
            #if (isset($matches[1]) && $matches[1] == $count) {

            echo $matches[1];
            }
            ?>

            Show
            poltawski Dan Poltawski added a comment - Ok, has taken me a bit of time to understand this issue. Many thanks to Peter for the info. For the benefit of others, here is a little code snippet to try and demonstrate what should be happening with the bad lines commented out: <?php $element = 'cmi.objectives_1.id'; $count = 1; $element = preg_replace('/\.(\d+)\./', "_\$1.", $element); preg_match('/_(\d+)\./', $element, $matches); if ($matches [1] == $count) { #preg_match('/\.(\d+)\./',$element,$matches); #$element = preg_replace('/\.(\d+)\./',".\$1.",$element); #if (isset($matches [1] ) && $matches [1] == $count) { echo $matches [1] ; } ?>
            Hide
            poltawski Dan Poltawski added a comment -

            Many thanks Peter, i've fixed this in CVS

            Show
            poltawski Dan Poltawski added a comment - Many thanks Peter, i've fixed this in CVS
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            code looks ok. Not tested.

            Anyway, before closing.... I'm reopening...

            1) Shouldn't we backport this to 18_STABLE? +1 for that.
            2) Looking at scorm_13.js.php ... seems that code is equivalent to the fixed here. Does it need fixing too?

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - code looks ok. Not tested. Anyway, before closing.... I'm reopening... 1) Shouldn't we backport this to 18_STABLE? +1 for that. 2) Looking at scorm_13.js.php ... seems that code is equivalent to the fixed here. Does it need fixing too? Ciao
            Hide
            poltawski Dan Poltawski added a comment -

            1) Grr, I was meaning to do that sorry.

            2) Yep, it looks like that to me too. I can't think of any reason with 1.3 would be any different.

            Going to do both of these now..

            Show
            poltawski Dan Poltawski added a comment - 1) Grr, I was meaning to do that sorry. 2) Yep, it looks like that to me too. I can't think of any reason with 1.3 would be any different. Going to do both of these now..
            Hide
            poltawski Dan Poltawski added a comment -

            Have merged to 1.8 and fixed the other 13 file too.

            Show
            poltawski Dan Poltawski added a comment - Have merged to 1.8 and fixed the other 13 file too.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Peeerfect Dan!

            Closing this now. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Peeerfect Dan! Closing this now. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  15/May/08