Bug fix: if a combinator template grader returned a JSON result in which the resulttable attribute was not a list of lists, a Moodle exception was thrown (Exception - count(): Argument #1 ($value) must be of type Countable|array, stdClass given).

Author: trampgeek

classes/combinator_grader_outcome.php

@@ -187,7 +187,7 @@ public function set_mark_and_feedback($markfraction, $feedback) {
             }
         }
 
-        if ($this->valid_table_formats($testresults, $this->columnformats)) {
+        if ($testresults && $this->valid_table($testresults) && $this->valid_table_formats($testresults, $this->columnformats)) {
             $this->format_results_table($testresults, $this->columnformats, $urls);
         }
     }
@@ -438,6 +438,31 @@ public function get_grader_state() {
         return empty($this->graderstate) ? '' : $this->graderstate;
     }
 
+    /**
+     * Check if the given testresults table is a list of lists.
+     * @param array $testresults  The testresults attribute from the grader.
+     * @return bool True iff $testresults is an array of arrays. If false,
+     * an appropriate status error message is set.
+     */
+    private function valid_table($testresults) {
+        $ok = is_array($testresults);
+        if ($ok) {
+            foreach ($testresults as $row) {
+                if (!is_array($row)) {
+                    $ok = false;
+                }
+            }
+        }
+        if (!$ok) {
+            $error = get_string(
+                'badresultstable',
+                'qtype_coderunner'
+            );
+            $this->set_status(self::STATUS_BAD_COMBINATOR, $error);
+        }
+        return $ok;
+    }
+
 
     /**
      * Check if the given values of column formats and test results are

edit_coderunner_form.php

@@ -1284,11 +1284,6 @@ public function validation($data, $files) {
             }
         }
 
-        $penaltyregimeerror = $this->validate_penalty_regime($data);
-        if ($penaltyregimeerror) {
-             $errors['markinggroup'] = $penaltyregimeerror;
-        }
-
         $resultcolumnsjson = trim($data['resultcolumns'] ?? '');
         if ($resultcolumnsjson !== '') {
             $resultcolumns = json_decode($resultcolumnsjson);
@@ -1324,6 +1319,15 @@ public function validation($data, $files) {
             $errors = $this->validate_twigables();
         }
 
+        if (count($errors) == 0) {
+            // Penalty regime is Twiggable, so we have postponed validating
+            // until after validate_twigables.
+            $penaltyregimeerror = $this->validate_penalty_regime($data);
+            if ($penaltyregimeerror) {
+                $errors['markinggroup'] = $penaltyregimeerror;
+            }
+        }
+
         if (count($errors) == 0 && !empty($data['validateonsave'])) {
             $testresult = $this->validate_sample_answer($data);
             if ($testresult) {
@@ -1461,9 +1465,16 @@ private function validate_penalty_regime($data) {
         $errorstring = '';
         $expectedpr = '/[0-9]+(\.[0-9]*)?%?([, ] *[0-9]+(\.[0-9]*)?%?)*([, ] *...)?/';
         $penaltyregime = trim($data['penaltyregime'] ?? '');
-        if (preg_match('/{{.*}}/', $penaltyregime)) {
-            return ''; // Looks like it's a Twig expression. Hope for the best.
+
+        // If twigall on, we should Twig expand the penalty regime
+        // before attempting to validate it.
+        if ($penaltyregime && $data['twigall']) {
+            $question = $this->formquestion;
+            $jsonparams = $question->templateparamsevald;
+            $parameters = json_decode($jsonparams, true);
+            $penaltyregime = $this->twig_render($penaltyregime, $parameters, true);
         }
+
         if ($penaltyregime == '') {
             $errorstring = get_string('emptypenaltyregime', 'qtype_coderunner');
         } else if (!preg_match($expectedpr, $penaltyregime)) {
@@ -1505,10 +1516,11 @@ private function validate_twigables() {
         $question = $this->formquestion;
         $jsonparams = $question->templateparamsevald;
         $parameters = json_decode($jsonparams, true);
-        $parameters['QUESTION'] = $question;
+        $parameters['QUESTION'] = $question; // Shouldn't really include this but removing it might break existing questions.
 
         // Try twig expanding everything (see question::twig_all), with strict_variables true.
-        foreach (['questiontext', 'answer', 'answerpreload', 'globalextra', 'prototypeextra', 'penaltyregime'] as $field) {
+        $twigablefields = qtype_coderunner::twigablefields();
+        foreach ($twigablefields as $field) {
             $text = $question->$field;
             if (is_array($text)) {
                 $text = $text['text'];

lang/en/qtype_coderunner.php

@@ -87,6 +87,7 @@
 $string['badquestion'] = 'Error in question';
 $string['badrandomintarg'] = 'Bad argument to JSON @randomint function';
 $string['badrandompickarg'] = 'Bad argument to JSON @randompic function';
+$string['badresultstable'] = 'Bad test results table. Should be a list of lists.';
 $string['badsandboxparams'] = '\'Other\' field (sandbox params) must be either blank or a valid JSON record';
 $string['badtemplateparams'] = 'Template parameters must evaluate to blank or a valid JSON record. Got: <pre class="templateparamserror">{$a}</pre>';
 $string['baduiparams'] = 'UI parameters must be blank or a valid JSON record.';

question.php

@@ -1020,10 +1020,9 @@ public function example_testcases() {
     private function twig_all() {
         // Twig expand everything in a context that includes the template
         // parameters and the STUDENT and QUESTION objects.
-        $twigables = ['questiontext', 'generalfeedback', 'answer', 'answerpreload',
-            'globalextra', 'prototypeextra', 'penaltyregime', 'uiparameters'];
+        $twigables = qtype_coderunner::twigablefields();
         foreach ($twigables as $twigable) {
-            if ($empty($this->$twigable)) {
+            if (!empty($this->$twigable)) {
                 $this->$twigable = $this->twig_expand($this->$twigable);
             }
         }

questiontype.php

@@ -173,6 +173,25 @@ public static function noninherited_fields() {
         ];
     }
 
+    /** A list of all fields in a question that are to be Twig expanded
+     * when TwigAll is set. It excludes the template itself, which is
+     * Twig expanded only when the question is run (as it needs a
+     * student answer).
+     * @return array A list of all fields to be Twig-expanded prior to rendering.
+     */
+    public static function twigablefields() {
+        return [
+            'questiontext',
+            'generalfeedback',
+            'answer',
+            'answerpreload',
+            'globalextra',
+            'prototypeextra',
+            'penaltyregime',
+            'uiparameters',
+        ];
+    }
+
     public function response_file_areas() {
         return ['attachments'];
     }