Skip to content

Commit

Permalink
MDL-29739 question import used a bad default for text formats.
Browse files Browse the repository at this point in the history 
The new logic is that the questiontext defaults to HTML if the format is
not specified, then all other fields default to the same format of the
question text.

However, good practice is that the XML file should specify the format
for each bit of text content.

This code is not 100% backwards-compatible, since some methods have new
arguments, however for question types that have not been updated, it
will just generate a PHP Warning, which is, I think, and OK way to let
qtype developers know that they need to update their question types.
  • Loading branch information
@timhunt
timhunt committed Nov 3, 2011
1 parent f08f22c commit 5eb4001
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 98 deletions.
114 changes: 54 additions & 60 deletions question/format/xml/format.php
View file
Expand Up @@ -81,7 +81,8 @@ protected function trans_format($name) {
} else if ($name == 'markdown') {
return FORMAT_MARKDOWN;
} else {
return 0; // or maybe warning required
debugging("Unrecognised text format '{$name}' in the import file. Assuming 'html'.");
return FORMAT_HTML;
}
}

Expand Down Expand Up @@ -171,7 +172,7 @@ public function import_headers($question) {
$qo->questiontext = $this->getpath($question,
array('#', 'questiontext', 0, '#', 'text', 0, '#'), '', true);
$qo->questiontextformat = $this->trans_format($this->getpath(
$question, array('#', 'questiontext', 0, '@', 'format'), ''));
$question, array('#', 'questiontext', 0, '@', 'format'), 'html'));

$qo->questiontextfiles = $this->import_files($this->getpath($question,
array('#', 'questiontext', 0, '#', 'file'), array(), false));
Expand All @@ -193,7 +194,7 @@ public function import_headers($question) {
array('#', 'generalfeedback', 0, '#', 'text', 0, '#'), $qo->generalfeedback, true);
$qo->generalfeedbackfiles = array();
$qo->generalfeedbackformat = $this->trans_format($this->getpath($question,
array('#', 'generalfeedback', 0, '@', 'format'), 'moodle_auto_format'));
array('#', 'generalfeedback', 0, '@', 'format'), $this->get_format($qo->questiontextformat)));
$qo->generalfeedbackfiles = $this->import_files($this->getpath($question,
array('#', 'generalfeedback', 0, '#', 'file'), array(), false));

Expand Down Expand Up @@ -223,25 +224,31 @@ public function import_headers($question) {
/**
* Import the common parts of a single answer
* @param array answer xml tree for single answer
* @param bool $withanswerfiles if true, the answers are HTML (or $defaultformat)
* and so may contain files, otherwise the answers are plain text.
* @param array Default text format for the feedback, and the answers if $withanswerfiles
* is true.
* @return object answer object
*/
public function import_answer($answer, $withanswerfiles = false) {
public function import_answer($answer, $withanswerfiles = false, $defaultformat = 'html') {
$ans = new stdClass();

$ans->answer = array();
$ans->answer['text'] = $this->getpath($answer, array('#', 'text', 0, '#'), '', true);
$ans->answer['format'] = $this->trans_format($this->getpath($answer,
array('@', 'format'), 'moodle_auto_format'));
array('@', 'format'), $defaultformat));
if ($withanswerfiles) {
$ans->answer['files'] = $this->import_files($this->getpath($answer,
array('#', 'file'), array()));
} else {
$ans->answer['format'] = FORMAT_PLAIN;
}

$ans->feedback = array();
$ans->feedback['text'] = $this->getpath($answer,
array('#', 'feedback', 0, '#', 'text', 0, '#'), '', true);
$ans->feedback['format'] = $this->trans_format($this->getpath($answer,
array('#', 'feedback', 0, '@', 'format'), 'moodle_auto_format'));
array('#', 'feedback', 0, '@', 'format'), $defaultformat));
$ans->feedback['files'] = $this->import_files($this->getpath($answer,
array('#', 'feedback', 0, '#', 'file'), array()));

Expand All @@ -263,7 +270,7 @@ public function import_combined_feedback($qo, $questionxml, $withshownumpartscor
$text['text'] = $this->getpath($questionxml,
array('#', $field, 0, '#', 'text', 0, '#'), '', true);
$text['format'] = $this->trans_format($this->getpath($questionxml,
array('#', $field, 0, '@', 'format'), 'moodle_auto_format'));
array('#', $field, 0, '@', 'format'), $this->get_format($qo->questiontextformat)));
$text['files'] = $this->import_files($this->getpath($questionxml,
array('#', $field, 0, '#', 'file'), array(), false));

Expand All @@ -284,16 +291,19 @@ public function import_combined_feedback($qo, $questionxml, $withshownumpartscor
/**
* Import a question hint
* @param array $hintxml hint xml fragment.
* @param string $defaultformat the text format to assume for hints that do not specify.
* @return object hint for storing in the database.
*/
public function import_hint($hintxml) {
public function import_hint($hintxml, $defaultformat) {
if (array_key_exists('hintcontent', $hintxml['#'])) {
// Backwards compatibility:

$hint = new stdClass();
$hint->hint = array('format' => FORMAT_HTML, 'files' => array());
$hint->hint['text'] = $this->getpath($hintxml,
array('#', 'hintcontent', 0, '#', 'text', 0, '#'), '', true);
$hint->hint['format'] = $this->trans_format($defaultformat);
$hint->hint['files'] = array();
$hint->shownumcorrect = $this->getpath($hintxml,
array('#', 'statenumberofcorrectresponses', 0, '#'), 0);
$hint->clearwrong = $this->getpath($hintxml,
Expand All @@ -308,7 +318,7 @@ public function import_hint($hintxml) {
$hint->hint['text'] = $this->getpath($hintxml,
array('#', 'text', 0, '#'), '', true);
$hint->hint['format'] = $this->trans_format($this->getpath($hintxml,
array('@', 'format'), 'moodle_auto_format'));
array('@', 'format'), $defaultformat));
$hint->hint['files'] = $this->import_files($this->getpath($hintxml,
array('#', 'file'), array(), false));
$hint->shownumcorrect = array_key_exists('shownumcorrect', $hintxml['#']);
Expand All @@ -322,15 +332,20 @@ public function import_hint($hintxml) {
* Import all the question hints
*
* @param object $qo the question data that is being constructed.
* @param array $hintsxml hints xml fragment.
* @param array $questionxml The xml representing the question.
* @param bool $withparts whether the extra fields relating to parts should be imported.
* @param bool $withoptions whether the extra options field should be imported.
* @param string $defaultformat the text format to assume for hints that do not specify.
* @return array of objects representing the hints in the file.
*/
public function import_hints($qo, $questionxml, $withparts = false, $withoptions = false) {
public function import_hints($qo, $questionxml, $withparts = false,
$withoptions = false, $defaultformat = 'html') {
if (!isset($questionxml['#']['hint'])) {
return;
}

foreach ($questionxml['#']['hint'] as $hintxml) {
$hint = $this->import_hint($hintxml);
$hint = $this->import_hint($hintxml, $defaultformat);
$qo->hint[] = $hint->hint;

if ($withparts) {
Expand Down Expand Up @@ -390,15 +405,15 @@ public function import_multichoice($question) {
$answers = $question['#']['answer'];
$acount = 0;
foreach ($answers as $answer) {
$ans = $this->import_answer($answer, true);
$ans = $this->import_answer($answer, true, $this->get_format($qo->questiontextformat));
$qo->answer[$acount] = $ans->answer;
$qo->fraction[$acount] = $ans->fraction;
$qo->feedback[$acount] = $ans->feedback;
++$acount;
}

$this->import_combined_feedback($qo, $question, true);
$this->import_hints($qo, $question, true);
$this->import_hints($qo, $question, true, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand All @@ -412,34 +427,36 @@ public function import_multianswer($question) {
question_bank::get_qtype('multianswer');

$questiontext['text'] = $this->import_text($question['#']['questiontext'][0]['#']['text']);
$questiontext['format'] = '1';
$questiontext['format'] = FORMAT_HTML;
$questiontext['itemid'] = '';
$qo = qtype_multianswer_extract_question($questiontext);

// 'header' parts particular to multianswer
$qo->qtype = 'multianswer';
$qo->course = $this->course;
$qo->generalfeedback = '';

$qo->name = $this->import_text($question['#']['name'][0]['#']['text']);
$qo->questiontextformat = $questiontext['format'];
$qo->questiontext = $qo->questiontext['text'];
$qo->questiontextfiles = array();

// restore files in generalfeedback
$qo->generalfeedback = $this->getpath($question,
array('#', 'generalfeedback', 0, '#', 'text', 0, '#'), $qo->generalfeedback, true);
$qo->generalfeedbackformat = $this->trans_format($this->getpath($question,
array('#', 'generalfeedback', 0, '@', 'format'), 'moodle_auto_format'));
array('#', 'generalfeedback', 0, '@', 'format'), $this->get_format($qo->questiontextformat)));
$qo->generalfeedbackfiles = $this->import_files($this->getpath($question,
array('#', 'generalfeedback', 0, '#', 'file'), array(), false));

$qo->name = $this->import_text($question['#']['name'][0]['#']['text']);
$qo->questiontext = $qo->questiontext['text'];
$qo->questiontextformat = '';

$qo->penalty = $this->getpath($question,
array('#', 'penalty', 0, '#'), $this->defaultquestion()->penalty);
// Fix problematic rounding from old files:
if (abs($qo->penalty - 0.3333333) < 0.005) {
$qo->penalty = 0.3333333;
}

$this->import_hints($qo, $question);
$this->import_hints($qo, $question, false, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand Down Expand Up @@ -469,7 +486,7 @@ public function import_truefalse($question) {
$feedback = $this->getpath($answer,
array('#', 'feedback', 0, '#', 'text', 0, '#'), '', true);
$feedbackformat = $this->getpath($answer,
array('#', 'feedback', 0, '@', 'format'), 'moodle_auto_format');
array('#', 'feedback', 0, '@', 'format'), $this->get_format($qo->questiontextformat));
$feedbackfiles = $this->getpath($answer,
array('#', 'feedback', 0, '#', 'file'), array());
$files = array();
Expand Down Expand Up @@ -515,7 +532,7 @@ public function import_truefalse($question) {
echo $OUTPUT->notification(get_string('truefalseimporterror', 'qformat_xml', $a));
}

$this->import_hints($qo, $question);
$this->import_hints($qo, $question, false, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand All @@ -539,14 +556,14 @@ public function import_shortanswer($question) {
$answers = $question['#']['answer'];
$acount = 0;
foreach ($answers as $answer) {
$ans = $this->import_answer($answer);
$ans = $this->import_answer($answer, false, $this->get_format($qo->questiontextformat));
$qo->answer[$acount] = $ans->answer['text'];
$qo->fraction[$acount] = $ans->fraction;
$qo->feedback[$acount] = $ans->feedback;
++$acount;
}

$this->import_hints($qo, $question);
$this->import_hints($qo, $question, false, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand Down Expand Up @@ -586,7 +603,7 @@ public function import_numerical($question) {
$qo->tolerance = array();
foreach ($answers as $answer) {
// answer outside of <text> is deprecated
$obj = $this->import_answer($answer);
$obj = $this->import_answer($answer, false, $this->get_format($qo->questiontextformat));
$qo->answer[] = $obj->answer['text'];
if (empty($qo->answer)) {
$qo->answer = '*';
Expand Down Expand Up @@ -622,12 +639,12 @@ public function import_numerical($question) {
$qo->instructions['text'] = $this->getpath($instructions,
array('0', '#', 'text', '0', '#'), '', true);
$qo->instructions['format'] = $this->trans_format($this->getpath($instructions,
array('0', '@', 'format'), 'moodle_auto_format'));
array('0', '@', 'format'), $this->get_format($qo->questiontextformat)));
$qo->instructions['files'] = $this->import_files($this->getpath(
$instructions, array('0', '#', 'file'), array()));
}

$this->import_hints($qo, $question);
$this->import_hints($qo, $question, false, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand All @@ -653,7 +670,7 @@ public function import_match($question) {
$subquestion = array();
$subquestion['text'] = $this->getpath($subqxml, array('#', 'text', 0, '#'), '', true);
$subquestion['format'] = $this->trans_format($this->getpath($subqxml,
array('@', 'format'), 'moodle_auto_format'));
array('@', 'format'), $this->get_format($qo->questiontextformat)));
$subquestion['files'] = $this->import_files($this->getpath($subqxml,
array('#', 'file'), array()));

Expand All @@ -664,7 +681,7 @@ public function import_match($question) {
}

$this->import_combined_feedback($qo, $question, true);
$this->import_hints($qo, $question, true);
$this->import_hints($qo, $question, true, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand All @@ -690,7 +707,7 @@ public function import_essay($question) {
$qo->graderinfo['text'] = $this->getpath($question,
array('#', 'graderinfo', 0, '#', 'text', 0, '#'), '', true);
$qo->graderinfo['format'] = $this->trans_format($this->getpath($question,
array('#', 'graderinfo', 0, '@', 'format'), 'moodle_auto_format'));
array('#', 'graderinfo', 0, '@', 'format'), $this->get_format($qo->questiontextformat)));
$qo->graderinfo['files'] = $this->import_files($this->getpath($question,
array('#', 'graderinfo', '0', '#', 'file'), array()));

Expand All @@ -716,30 +733,7 @@ public function import_calculated($question) {
array('#', 'answernumbering', 0, '#'), 'abc');
$qo->shuffleanswers = $this->trans_single($shuffleanswers);

$qo->correctfeedback = array();
$qo->correctfeedback['text'] = $this->getpath(
$question, array('#', 'correctfeedback', 0, '#', 'text', 0, '#'), '', true);
$qo->correctfeedback['format'] = $this->trans_format($this->getpath(
$question, array('#', 'correctfeedback', 0, '@', 'format'), 'moodle_auto_format'));
$qo->correctfeedback['files'] = $this->import_files($this->getpath(
$question, array('#', 'correctfeedback', '0', '#', 'file'), array()));

$qo->partiallycorrectfeedback = array();
$qo->partiallycorrectfeedback['text'] = $this->getpath($question,
array('#', 'partiallycorrectfeedback', 0, '#', 'text', 0, '#'), '', true);
$qo->partiallycorrectfeedback['format'] = $this->trans_format(
$this->getpath($question, array('#', 'partiallycorrectfeedback', 0, '@', 'format'),
'moodle_auto_format'));
$qo->partiallycorrectfeedback['files'] = $this->import_files($this->getpath(
$question, array('#', 'partiallycorrectfeedback', '0', '#', 'file'), array()));

$qo->incorrectfeedback = array();
$qo->incorrectfeedback['text'] = $this->getpath($question,
array('#', 'incorrectfeedback', 0, '#', 'text', 0, '#'), '', true);
$qo->incorrectfeedback['format'] = $this->trans_format($this->getpath($question,
array('#', 'incorrectfeedback', 0, '@', 'format'), 'moodle_auto_format'));
$qo->incorrectfeedback['files'] = $this->import_files($this->getpath($question,
array('#', 'incorrectfeedback', '0', '#', 'file'), array()));
$this->import_combined_feedback($qo, $question);

$qo->unitgradingtype = $this->getpath($question,
array('#', 'unitgradingtype', 0, '#'), 0);
Expand All @@ -753,7 +747,7 @@ public function import_calculated($question) {
$qo->instructions['text'] = $this->getpath($instructions,
array('0', '#', 'text', '0', '#'), '', true);
$qo->instructions['format'] = $this->trans_format($this->getpath($instructions,
array('0', '@', 'format'), 'moodle_auto_format'));
array('0', '@', 'format'), $this->get_format($qo->questiontextformat)));
$qo->instructions['files'] = $this->import_files($this->getpath($instructions,
array('0', '#', 'file'), array()));
}
Expand All @@ -769,7 +763,7 @@ public function import_calculated($question) {
$qo->correctanswerlength = array();
$qo->feedback = array();
foreach ($answers as $answer) {
$ans = $this->import_answer($answer, true);
$ans = $this->import_answer($answer, true, $this->get_format($qo->questiontextformat));
// answer outside of <text> is deprecated
if (empty($ans->answer['text'])) {
$ans->answer['text'] = '*';
Expand Down Expand Up @@ -803,7 +797,7 @@ public function import_calculated($question) {
$qo->instructions['text'] = $this->getpath($instructions,
array('0', '#', 'text', '0', '#'), '', true);
$qo->instructions['format'] = $this->trans_format($this->getpath($instructions,
array('0', '@', 'format'), 'moodle_auto_format'));
array('0', '@', 'format'), $this->get_format($qo->questiontextformat)));
$qo->instructions['files'] = $this->import_files($this->getpath($instructions,
array('0', '#', 'file'), array()));
}
Expand Down Expand Up @@ -848,7 +842,7 @@ public function import_calculated($question) {
}
}

$this->import_hints($qo, $question);
$this->import_hints($qo, $question, false, false, $this->get_format($qo->questiontextformat));

return $qo;
}
Expand Down Expand Up @@ -970,7 +964,7 @@ protected function get_qtype($qtype) {
* @param int id internal code
* @return string format text
*/
protected function get_format($id) {
public function get_format($id) {
switch($id) {
case FORMAT_MOODLE:
return 'moodle_auto_format';
Expand Down

0 comments on commit 5eb4001

Please sign in to comment.