question upgrade MDL-16094 fix up earlier mistakes in the text format…

… upgrade.

There was a mistake in the text format upgrade in the question bank. The wrong conversions were performed, and the wrong arguments were passed to text_to_html in the conversions that were done.

Also, not all the calls to format_text had been updated to use the values in the new format columns.

I think this change fixes everything, but I have only had very limited time to test it. I am committing it anyway, because that seems to me to be the best way to maximise testing. I think that the new code is certainly better than the old code was.

lib/db/upgrade.php

@@ -4927,6 +4927,19 @@ function xmldb_main_upgrade($oldversion) {
             $dbman->add_field($table, $field);
         }
 
+    /// Upgrading the text formats in some question types depends on the
+    /// questiontextformat field, but the question type upgrade only runs
+    /// after the code below has messed around with the questiontextformat
+    /// value. Therefore, we need to create a new column to store the old value.
+    /// The column should be dropped in Moodle 2.1.
+    /// Define field oldquestiontextformat to be added to question
+        $field = new xmldb_field('oldquestiontextformat', XMLDB_TYPE_INTEGER, '2', null, XMLDB_NOTNULL, null, '0', 'generalfeedback');
+
+    /// Conditionally launch add field oldquestiontextformat
+        if (!$dbman->field_exists($table, $field)) {
+            $dbman->add_field($table, $field);
+        }
+
     /// Define field infoformat to be added to question_categories
         $table = new xmldb_table('question_categories');
         $field = new xmldb_field('infoformat', XMLDB_TYPE_INTEGER, '2', null, XMLDB_NOTNULL, null, '0', 'info');
@@ -5039,44 +5052,72 @@ function xmldb_main_upgrade($oldversion) {
             $dbman->drop_field($table, $field);
         }
 
-        // fix fieldformat
-        $sql = 'SELECT a.*, q.qtype FROM {question_answers} a, {question} q WHERE a.question = q.id';
-        $rs = $DB->get_recordset_sql($sql);
+        // Update question_answers.
+        // In question_answers.feedback was previously always treated as
+        // FORMAT_HTML in calculated, multianswer, multichoice, numerical,
+        // shortanswer and truefalse; and
+        // FORMAT_MOODLE in essay (despite being edited using the HTML editor)
+        // So essay feedback needs to be converted to HTML unless $CFG->texteditors == 'textarea'.
+        // For all question types except multichoice,
+        // question_answers.answer is FORMAT_PLAIN and does not need to be changed.
+        // For multichoice, question_answers.answer is FORMAT_MOODLE, and should
+        // stay that way, at least for now.
+        $rs = $DB->get_recordset_sql('
+                SELECT qa.*, q.qtype
+                FROM {question_answers} qa
+                JOIN {question} q ON a.question = q.id');
         foreach ($rs as $record) {
-            // generalfeedback should use questiontext format
+            // Convert question_answers.answer
+            if ($record->qtype !== 'multichoice') {
+                $record->answerformat = FORMAT_PLAIN;
+            } else {
+                $record->answerformat = FORMAT_MOODLE;
+            }
+
+            // Convert question_answers.feedback
             if ($CFG->texteditors !== 'textarea') {
-                if (!empty($record->feedback)) {
-                    $record->feedback = text_to_html($record->feedback);
+                if ($record->qtype == 'essay') {
+                    $record->feedback = text_to_html($record->feedback, false, false, true);
                 }
                 $record->feedbackformat = FORMAT_HTML;
             } else {
                 $record->feedbackformat = FORMAT_MOODLE;
-                $record->answerformat = FORMAT_MOODLE;
             }
-            unset($record->qtype);
+
             $DB->update_record('question_answers', $record);
         }
         $rs->close();
 
-        $rs = $DB->get_recordset('question');
-        foreach ($rs as $record) {
-            if ($CFG->texteditors !== 'textarea') {
-                if (!empty($record->questiontext)) {
-                    $record->questiontext = text_to_html($record->questiontext);
-                }
+        // In the question table, the code previously used questiontextformat
+        // for both question text and general feedback. We need to copy the
+        // values into the new column.
+        // Then we need to convert FORMAT_MOODLE to FORMAT_HTML (depending on
+        // $CFG->texteditors).
+        $DB->execute('
+                UPDATE {question}
+                SET generalfeedbackformat = questiontextformat');
+        // Also save the old questiontextformat, so that plugins that need it
+        // can access it.
+        $DB->execute('
+                UPDATE {question}
+                SET oldquestiontextformat = questiontextformat');
+        // Now covert FORMAT_MOODLE content, if necssary.
+        if ($CFG->texteditors !== 'textarea') {
+            $rs = $DB->get_recordset('question', 'questiontextformat', FORMAT_MOODLE);
+            foreach ($rs as $record) {
+                $record->questiontext = text_to_html($record->questiontext, false, false, true);
                 $record->questiontextformat = FORMAT_HTML;
-                // conver generalfeedback text to html
-                if (!empty($record->generalfeedback)) {
-                    $record->generalfeedback = text_to_html($record->generalfeedback);
-                }
-            } else {
-                $record->questiontextformat = FORMAT_MOODLE;
+                $record->generalfeedback = text_to_html($record->generalfeedback, false, false, true);
+                $record->generalfeedbackformat = FORMAT_HTML;
+                $DB->update_record('question', $record);
             }
-            // generalfeedbackformat should be the save as questiontext format
-            $record->generalfeedbackformat = $record->questiontextformat;
-            $DB->update_record('question', $record);
+            $rs->close();
         }
-        $rs->close();
+
+        // In the past, question_sessions.manualcommentformat was always treated
+        // as FORMAT_HTML.
+        $DB->set_field('question_sessions', 'manualcommentformat', FORMAT_HTML);
+
         // Main savepoint reached
         upgrade_main_savepoint(true, 2010080901);
     }

lib/questionlib.php

@@ -1027,7 +1027,8 @@ function question_preload_states($attemptid) {
     // array index in the array returned by $DB->get_records_sql
     $statefields = 'n.questionid as question, s.id, s.attempt, ' .
             's.seq_number, s.answer, s.timestamp, s.event, s.grade, s.raw_grade, ' .
-            's.penalty, n.sumpenalty, n.manualcomment, n.flagged, n.id as questionsessionid';
+            's.penalty, n.sumpenalty, n.manualcomment, n.manualcommentformat, ' .
+            'n.flagged, n.id as questionsessionid';
 
     // Load the newest states for the questions
     $sql = "SELECT $statefields
@@ -1131,6 +1132,7 @@ function question_load_states(&$questions, &$states, $cmoptions, $attempt, $last
             $states[$qid]->penalty = 0;
             $states[$qid]->sumpenalty = 0;
             $states[$qid]->manualcomment = '';
+            $states[$qid]->manualcommentformat = FORMAT_HTML;
             $states[$qid]->flagged = 0;
 
             // Prevent further changes to the session from incrementing the
@@ -1217,7 +1219,8 @@ function question_load_specific_state($question, $cmoptions, $attempt, $stateid)
     global $DB;
     // Load specified states for the question.
     // sess.sumpenalty is probably wrong here shoul really be a sum of penalties from before the one we are asking for.
-    $sql = 'SELECT st.*, sess.sumpenalty, sess.manualcomment, sess.flagged, sess.id as questionsessionid
+    $sql = 'SELECT st.*, sess.sumpenalty, sess.manualcomment, sess.manualcommentformat,
+                        sess.flagged, sess.id as questionsessionid
               FROM {question_states} st, {question_sessions} sess
              WHERE st.id = ?
                AND st.attempt = ?
@@ -1231,7 +1234,8 @@ function question_load_specific_state($question, $cmoptions, $attempt, $stateid)
     restore_question_state($question, $state);
 
     // Load the most recent graded states for the questions before the specified one.
-    $sql = 'SELECT st.*, sess.sumpenalty, sess.manualcomment, sess.flagged, sess.id as questionsessionid
+    $sql = 'SELECT st.*, sess.sumpenalty, sess.manualcomment, sess.manualcommentformat,
+                        sess.flagged, sess.id as questionsessionid
               FROM {question_states} st, {question_sessions} sess
              WHERE st.seq_number <= ?
                AND st.attempt = ?
@@ -1267,8 +1271,6 @@ function restore_question_state(&$question, &$state) {
 
     // initialise response to the value in the answer field
     $state->responses = array('' => $state->answer);
-    unset($state->answer);
-    $state->manualcomment = isset($state->manualcomment) ? $state->manualcomment : '';
 
     // Set the changed field to false; any code which changes the
     // question session must set this to true and must increment
@@ -1278,8 +1280,7 @@ function restore_question_state(&$question, &$state) {
     $state->changed = false;
 
     // Load the question type specific data
-    return $QTYPES[$question->qtype]
-            ->restore_session_and_responses($question, $state);
+    return $QTYPES[$question->qtype]->restore_session_and_responses($question, $state);
 
 }
 
@@ -1332,6 +1333,7 @@ function save_question_session($question, $state) {
         $session->newgraded = $state->id;
         $session->sumpenalty = $state->sumpenalty;
         $session->manualcomment = $state->manualcomment;
+        $session->manualcommentformat = $state->manualcommentformat;
         $session->flagged = !empty($state->newflaggedstate);
         $DB->insert_record('question_sessions', $session);
     } else {
@@ -1341,8 +1343,7 @@ function save_question_session($question, $state) {
             $session->newgraded = $state->id;
             $session->sumpenalty = $state->sumpenalty;
             $session->manualcomment = $state->manualcomment;
-        } else {
-            $session->manualcomment = $session->manualcomment;
+            $session->manualcommentformat = $state->manualcommentformat;
         }
         $session->flagged = !empty($state->newflaggedstate);
         $DB->update_record('question_sessions', $session);
@@ -1554,7 +1555,7 @@ function regrade_question_in_attempt($question, $attempt, $cmoptions, $verbose=f
                 }
                 if (!$dryrun){
                     $error = question_process_comment($question, $replaystate, $attempt,
-                            $replaystate->manualcomment, $states[$j]->grade);
+                            $replaystate->manualcomment, $replaystate->manualcommentformat, $states[$j]->grade);
                     if (is_string($error)) {
                          echo $OUTPUT->notification($error);
                     }
@@ -1939,7 +1940,7 @@ function question_print_comment_fields($question, $state, $prefix, $cmoptions, $
  * @return mixed true on success, a string error message if a problem is detected
  *         (for example score out of range).
  */
-function question_process_comment($question, &$state, &$attempt, $comment, $grade) {
+function question_process_comment($question, &$state, &$attempt, $comment, $commentformat, $grade) {
     global $DB;
 
     $grade = trim($grade);
@@ -1954,6 +1955,7 @@ function question_process_comment($question, &$state, &$attempt, $comment, $grad
     // Update the comment and save it in the database
     $comment = trim($comment);
     $state->manualcomment = $comment;
+    $state->manualcommentformat = $commentformat;
     $state->newflaggedstate = $state->flagged;
     $DB->set_field('question_sessions', 'manualcomment', $comment, array('attemptid'=>$attempt->uniqueid, 'questionid'=>$question->id));
 

mod/quiz/attemptlib.php

@@ -895,7 +895,7 @@ public function links_to_other_attempts($url) {
      * @return mixed true on success, a string error message if a problem is detected
      *         (for example score out of range).
      */
-    public function process_comment($questionid, $comment, $grade) {
+    public function process_comment($questionid, $comment, $commentformat, $grade) {
         // I am not sure it is a good idea to have update methods here - this
         // class is only about getting data out of the question engine, and
         // helping to display it, apart from this.
@@ -904,7 +904,7 @@ public function process_comment($questionid, $comment, $grade) {
         $state = $this->states[$questionid];
 
         $error = question_process_comment($this->questions[$questionid],
-                $state, $this->attempt, $comment, $grade);
+                $state, $this->attempt, $comment, $commentformat, $grade);
 
         // If the state was update (successfully), save the changes.
         if (!is_string($error) && $state->changed) {

mod/quiz/comment.php

@@ -44,7 +44,7 @@
 /// Process any data that was submitted.
     if ($data = data_submitted() and confirm_sesskey()) {
         $error = $attemptobj->process_comment($questionid,
-                $data->response['comment'], $data->response['grade']);
+                $data->response['comment'], FORMAT_HTML, $data->response['grade']);
 
     /// If success, notify and print a close button.
         if (!is_string($error)) {

mod/quiz/db/upgrade.php

@@ -282,7 +282,7 @@ function xmldb_quiz_upgrade($oldversion) {
 
         // conditionally migrate to html format in intro
         if ($CFG->texteditors !== 'textarea') {
-            $rs = $DB->get_recordset('quiz', array('introformat'=>FORMAT_MOODLE), '', 'id,intro,introformat');
+            $rs = $DB->get_recordset('quiz', array('introformat' => FORMAT_MOODLE), '', 'id,intro,introformat');
             foreach ($rs as $q) {
                 $q->intro       = text_to_html($q->intro, false, false, true);
                 $q->introformat = FORMAT_HTML;
@@ -352,6 +352,8 @@ function xmldb_quiz_upgrade($oldversion) {
             $dbman->add_field($table, $field);
         }
 
+        // This column defaults to FORMAT_MOODLE, which is correct.
+
         // quiz savepoint reached
         upgrade_mod_savepoint(true, 2010080600, 'quiz');
     }

mod/quiz/report/grading/report.php

@@ -130,7 +130,8 @@ function display($quiz, $cm, $course) {
                     $state = &$states[$question->id];
 
                     // the following will update the state and attempt
-                    $error = question_process_comment($question, $state, $attempt, $response['comment'], $response['grade']);
+                    $error = question_process_comment($question, $state, $attempt,
+                            $response['comment'], FORMAT_HTML, $response['grade']);
                     if (is_string($error)) {
                         echo $OUTPUT->notification($error);
                         $allok = false;

question/type/calculated/db/upgrade.php

@@ -165,26 +165,26 @@ function xmldb_qtype_calculated_upgrade($oldversion) {
             $dbman->add_field($table, $field);
         }
 
-        // fix fieldformat
-        $rs = $DB->get_recordset('question_calculated_options');
+        // In the past, the correctfeedback, partiallycorrectfeedback,
+        // incorrectfeedback columns were assumed to contain content of the same
+        // form as questiontextformat. If we are using the HTML editor, then
+        // convert FORMAT_MOODLE content to FORMAT_HTML.
+        $rs = $DB->get_recordset_sql('
+                SELECT qco.*, q.oldquestiontextformat
+                FROM {question_calculated_options} qco
+                JOIN {question} q ON qco.question = q.id');
         foreach ($rs as $record) {
-            if ($CFG->texteditors !== 'textarea') {
-                if (!empty($record->correctfeedback)) {
-                    $record->correctfeedback = text_to_html($record->correctfeedback);
-                }
+            if ($CFG->texteditors !== 'textarea' && $record->oldquestiontextformat == FORMAT_MOODLE) {
+                $record->correctfeedback = text_to_html($record->correctfeedback, false, false, true);
                 $record->correctfeedbackformat = FORMAT_HTML;
-                if (!empty($record->partiallycorrectfeedback)) {
-                    $record->partiallycorrectfeedback = text_to_html($record->partiallycorrectfeedback);
-                }
+                $record->partiallycorrectfeedback = text_to_html($record->partiallycorrectfeedback, false, false, true);
                 $record->partiallycorrectfeedbackformat = FORMAT_HTML;
-                if (!empty($record->incorrectfeedback)) {
-                    $record->incorrectfeedback = text_to_html($record->incorrectfeedback);
-                }
+                $record->incorrectfeedback = text_to_html($record->incorrectfeedback, false, false, true);
                 $record->incorrectfeedbackformat = FORMAT_HTML;
             } else {
-                $record->correctfeedbackformat = FORMAT_MOODLE;
-                $record->partiallycorrectfeedbackformat = FORMAT_MOODLE;
-                $record->incorrectfeedbackformat = FORMAT_MOODLE;
+                $record->correctfeedbackformat = $record->oldquestiontextformat;
+                $record->partiallycorrectfeedback = $record->oldquestiontextformat;
+                $record->incorrectfeedbackformat = $record->oldquestiontextformat;
             }
             $DB->update_record('question_calculated_options', $record);
         }

question/type/description/questiontype.php

@@ -72,7 +72,7 @@ function print_question(&$question, &$state, $number, $cmoptions, $options) {
         $generalfeedback = '';
         if ($isfinished && $options->generalfeedback) {
             $generalfeedback = $this->format_text($question->generalfeedback,
-                    $question->questiontextformat, $cmoptions);
+                    $question->generalfeedbackformat, $cmoptions);
         }
 
         include "$CFG->dirroot/question/type/description/question.html";

question/type/essay/questiontype.php

@@ -105,7 +105,7 @@ function print_question_formulation_and_controls(&$question, &$state, $cmoptions
         if (isset($state->responses[''])) {
             $value = $state->responses[''];
         } else {
-            $value = "";
+            $value = '';
         }
 
         // answer

question/type/match/db/upgrade.php

@@ -36,15 +36,19 @@ function xmldb_qtype_match_upgrade($oldversion) {
             $dbman->add_field($table, $field);
         }
 
-        $rs = $DB->get_recordset('question_match_sub');
+        // In the past, question_match_sub.questiontext assumed to contain
+        // content of the same form as question.questiontextformat. If we are
+        // using the HTML editor, then convert FORMAT_MOODLE content to FORMAT_HTML.
+        $rs = $DB->get_recordset_sql('
+                SELECT qms.*, q.oldquestiontextformat
+                FROM {question_match_sub} qms
+                JOIN {question} q ON qms.question = q.id');
         foreach ($rs as $record) {
-            if ($CFG->texteditors !== 'textarea') {
-                if (!empty($record->questiontext)) {
-                    $record->questiontext = text_to_html($record->questiontext);
-                }
+            if ($CFG->texteditors !== 'textarea' && $record->oldquestiontextformat == FORMAT_MOODLE) {
+                $record->questiontext = text_to_html($record->questiontext, false, false, true);
                 $record->questiontextformat = FORMAT_HTML;
             } else {
-                $record->questiontextformat = FORMAT_MOODLE;
+                $record->questiontextformat = $record->oldquestiontextformat;
             }
             $DB->update_record('question_match_sub', $record);
         }

question/type/missingtype/questiontype.php

@@ -48,7 +48,7 @@ function print_question_formulation_and_controls(&$question, &$state, $cmoptions
         if ($answers) {
             foreach ($answers as $answer) {
                 $a = new stdClass;
-                $a->text = format_text("$answer->answer", FORMAT_MOODLE, $formatoptions, $cmoptions->course);
+                $a->text = format_text($answer->answer, $answer->answerformat, $formatoptions, $cmoptions->course);
 
                 $anss[] = clone($a);
             }