MDL-46148 qtype_calculated: validate formulas everywhere.
authorTim Hunt <T.J.Hunt@open.ac.uk>
Thu, 26 Jun 2014 14:37:33 +0000 (15:37 +0100)
committerDan Poltawski <dan@moodle.com>
Mon, 7 Jul 2014 13:33:34 +0000 (14:33 +0100)
question/type/calculated/edit_calculated_form.php
question/type/calculated/questiontype.php
question/type/calculatedmulti/edit_calculatedmulti_form.php
question/type/calculatedmulti/questiontype.php

index 853030d..532f056 100644 (file)
@@ -216,36 +216,39 @@ class qtype_calculated_edit_form extends qtype_numerical_edit_form {
         return 'calculated';
     }
 
-    public function validation($data, $files) {
-
-        // Verifying for errors in {=...} in question text.
-        $qtext = "";
-        $qtextremaining = $data['questiontext']['text'];
-        $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
-        foreach ($possibledatasets as $name => $value) {
-            $qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining);
-        }
-        while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) {
-            $qtextsplits = explode($regs1[0], $qtextremaining, 2);
-            $qtext = $qtext.$qtextsplits[0];
-            $qtextremaining = $qtextsplits[1];
-            if (!empty($regs1[1]) && $formulaerrors =
-                    qtype_calculated_find_formula_errors($regs1[1])) {
-                if (!isset($errors['questiontext'])) {
-                    $errors['questiontext'] = $formulaerrors.':'.$regs1[1];
-                } else {
-                    $errors['questiontext'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
-                }
-            }
+    /**
+     * Validate the equations in the some question content.
+     * @param array $errors where errors are being accumulated.
+     * @param string $field the field being validated.
+     * @param string $text the content of that field.
+     * @return array the updated $errors array.
+     */
+    protected function validate_text($errors, $field, $text) {
+        $problems = qtype_calculated_find_formula_errors_in_text($text);
+        if ($problems) {
+            $errors[$field] = $problems;
         }
+        return $errors;
+    }
 
+    public function validation($data, $files) {
         $errors = parent::validation($data, $files);
 
+        // Verifying for errors in {=...} in question text.
+        $errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']);
+        $errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']);
+
         // Check that the answers use datasets.
         $answers = $data['answer'];
         $mandatorydatasets = array();
         foreach ($answers as $key => $answer) {
+            $problems = qtype_calculated_find_formula_errors($answer);
+            if ($problems) {
+                $errors['answeroptions['.$key.']'] = $problems;
+            }
             $mandatorydatasets += $this->qtypeobj->find_dataset_names($answer);
+            $errors = $this->validate_text($errors, 'feedback[' . $key . ']',
+                    $data['feedback'][$key]['text']);
         }
         if (empty($mandatorydatasets)) {
             foreach ($answers as $key => $answer) {
index 1ce68e8..414470d 100644 (file)
@@ -484,6 +484,36 @@ class qtype_calculated extends question_type {
         $mform->display();
     }
 
+    /**
+     * Verify that the equations in part of the question are OK.
+     * We throw an exception here because this should have already been validated
+     * by the form. This is just a last line of defence to prevent a question
+     * being stored in the database if it has bad formulas. This saves us from,
+     * for example, malicious imports.
+     * @param string $text containing equations.
+     */
+    protected function validate_text($text) {
+        $error = qtype_calculated_find_formula_errors_in_text($text);
+        if ($error) {
+            throw new coding_exception($error);
+        }
+    }
+
+    /**
+     * Verify that an answer is OK.
+     * We throw an exception here because this should have already been validated
+     * by the form. This is just a last line of defence to prevent a question
+     * being stored in the database if it has bad formulas. This saves us from,
+     * for example, malicious imports.
+     * @param string $text containing equations.
+     */
+    protected function validate_answer($answer) {
+        $error = qtype_calculated_find_formula_errors($answer);
+        if ($error) {
+            throw new coding_exception($error);
+        }
+    }
+
     /**
      * This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
      * so that they can be saved
@@ -497,11 +527,12 @@ class qtype_calculated extends question_type {
      * @param int $questionfromid default = '0'
      */
     public function preparedatasets($form , $questionfromid = '0') {
+
         // The dataset names present in the edit_question_form and edit_calculated_form
         // are retrieved.
         $possibledatasets = $this->find_dataset_names($form->questiontext);
         $mandatorydatasets = array();
-        foreach ($form->answers as $answer) {
+        foreach ($form->answers as $key => $answer) {
             $mandatorydatasets += $this->find_dataset_names($answer);
         }
         // If there are identical datasetdefs already saved in the original question
@@ -590,8 +621,15 @@ class qtype_calculated extends question_type {
      */
     public function save_question($question, $form) {
         global $DB;
+
+        if (isset($form->correctfeedback)) {
+            $this->validate_text($form->correctfeedback['text']);
+            $this->validate_text($form->partiallycorrectfeedback['text']);
+            $this->validate_text($form->incorrectfeedback['text']);
+        }
+
         if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
-                $question = parent::save_question($question, $form);
+            $question = parent::save_question($question, $form);
             return $question;
         }
 
@@ -610,6 +648,14 @@ class qtype_calculated extends question_type {
             case '' :
             case 'question': // Coming from the first page, creating the second.
                 if (empty($form->id)) { // or a new question $form->id is empty.
+                    // Make it impossible to save bad formulas anywhere.
+                    $this->validate_text($form->questiontext['text']);
+                    $this->validate_text($form->generalfeedback['text']);
+                    foreach ($form->answer as $key => $answer) {
+                        $this->validate_answer($answer);
+                        $this->validate_text($form->feedback[$key]['text']);
+                    }
+
                     $question = parent::save_question($question, $form);
                     // Prepare the datasets using default $questionfromid.
                     $this->preparedatasets($form);
index 400a1df..bd0af82 100644 (file)
@@ -232,30 +232,31 @@ class qtype_calculatedmulti_edit_form extends question_edit_form {
         return $question;
     }
 
+    /**
+     * Validate the equations in the some question content.
+     * @param array $errors where errors are being accumulated.
+     * @param string $field the field being validated.
+     * @param string $text the content of that field.
+     * @return array the updated $errors array.
+     */
+    protected function validate_text($errors, $field, $text) {
+        $problems = qtype_calculated_find_formula_errors_in_text($text);
+        if ($problems) {
+            $errors[$field] = $problems;
+        }
+        return $errors;
+    }
+
     public function validation($data, $files) {
         $errors = parent::validation($data, $files);
 
         // Verifying for errors in {=...} in question text.
-        $qtext = '';
-        $qtextremaining = $data['questiontext']['text'];
-        $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
-        foreach ($possibledatasets as $name => $value) {
-            $qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining);
-        }
+        $errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']);
+        $errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']);
+        $errors = $this->validate_text($errors, 'correctfeedback', $data['correctfeedback']['text']);
+        $errors = $this->validate_text($errors, 'partiallycorrectfeedback', $data['partiallycorrectfeedback']['text']);
+        $errors = $this->validate_text($errors, 'incorrectfeedback', $data['incorrectfeedback']['text']);
 
-        while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) {
-            $qtextsplits = explode($regs1[0], $qtextremaining, 2);
-            $qtext = $qtext.$qtextsplits[0];
-            $qtextremaining = $qtextsplits[1];
-            if (!empty($regs1[1]) && $formulaerrors =
-                    qtype_calculated_find_formula_errors($regs1[1])) {
-                if (!isset($errors['questiontext'])) {
-                    $errors['questiontext'] = $formulaerrors.':'.$regs1[1];
-                } else {
-                    $errors['questiontext'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
-                }
-            }
-        }
         $answers = $data['answer'];
         $answercount = 0;
         $maxgrade = false;
@@ -270,6 +271,7 @@ class qtype_calculatedmulti_edit_form extends question_edit_form {
                         get_string('atleastonewildcard', 'qtype_calculated');
             }
         }
+
         $totalfraction = 0;
         $maxfraction = -1;
         foreach ($answers as $key => $answer) {
@@ -283,27 +285,11 @@ class qtype_calculatedmulti_edit_form extends question_edit_form {
             }
             if ($trimmedanswer != '' || $answercount == 0) {
                 // Verifying for errors in {=...} in answer text.
-                $qanswer = '';
-                $qanswerremaining =  $trimmedanswer;
-                $possibledatasets = $this->qtypeobj->find_dataset_names($trimmedanswer);
-                foreach ($possibledatasets as $name => $value) {
-                    $qanswerremaining = str_replace('{'.$name.'}', '1', $qanswerremaining);
-                }
-
-                while (preg_match('~\{=([^[:space:]}]*)}~', $qanswerremaining, $regs1)) {
-                    $qanswersplits = explode($regs1[0], $qanswerremaining, 2);
-                    $qanswer = $qanswer . $qanswersplits[0];
-                    $qanswerremaining = $qanswersplits[1];
-                    if (!empty($regs1[1]) && $formulaerrors =
-                            qtype_calculated_find_formula_errors($regs1[1])) {
-                        if (!isset($errors['answeroptions['.$key.']'])) {
-                            $errors['answeroptions['.$key.']'] = $formulaerrors.':'.$regs1[1];
-                        } else {
-                            $errors['answeroptions['.$key.']'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
-                        }
-                    }
-                }
+                $errors = $this->validate_text($errors, 'answeroptions[' . $key . ']', $answer);
+                $errors = $this->validate_text($errors, 'feedback[' . $key . ']',
+                        $data['feedback'][$key]['text']);
             }
+
             if ($trimmedanswer != '') {
                 if ('2' == $data['correctanswerformat'][$key] &&
                         '0' == $data['correctanswerlength'][$key]) {
index 3522e0d..283ccd4 100644 (file)
@@ -156,6 +156,13 @@ class qtype_calculatedmulti extends qtype_calculated {
         return true;
     }
 
+    protected function validate_answer($answer) {
+        $error = qtype_calculated_find_formula_errors_in_text($answer);
+        if ($error) {
+            throw new coding_exception($error);
+        }
+    }
+
     protected function make_question_instance($questiondata) {
         question_bank::load_question_definition_classes($this->name());
         if ($questiondata->options->single) {