Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      29738

      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"; }

      ?>

        Activity

        Hide
        Dan Marsden added a comment -

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

        Dan

        Show
        Dan Marsden added a comment - assigning to me - will test fix and look at patching it. Dan
        Hide
        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
        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
        Dan Poltawski added a comment -

        Many thanks Peter, i've fixed this in CVS

        Show
        Dan Poltawski added a comment - Many thanks Peter, i've fixed this in CVS
        Hide
        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
        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
        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
        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
        Dan Poltawski added a comment -

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

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

        Peeerfect Dan!

        Closing this now. Ciao

        Show
        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: