Permalink
Browse files

MDL-34841 error importing questions with long names.

The problem was with the non-UTF-8-safe way that a question name
was being constructed from the question text.

I have done a proper fix with methods in the base class to
carefully construct a question name that is reasonable, and
which will fit in the database column. Then I have changed all
importers to use the new methods.

I also remembered not to break the lesson in the process.
  • Loading branch information...
1 parent 7413ce9 commit 5df0a6b47e246ce0a5de46bfe3fdf4d6974a2242 @timhunt timhunt committed Sep 11, 2012
View
@@ -541,6 +541,37 @@ function readquestion($lines) {
return NULL;
}
+ /**
+ * Construct a reasonable default question name, based on the start of the question text.
+ * @param string $questiontext the question text.
+ * @param string $default default question name to use if the constructed one comes out blank.
+ * @return string a reasonable question name.
+ */
+ public function create_default_question_name($questiontext, $default) {
+ $name = $this->clean_question_name(shorten_text($questiontext, 80));
+ if ($name) {
+ return $name;
+ } else {
+ return $default;
+ }
+ }
+
+ /**
+ * Ensure that a question name does not contain anything nasty, and will fit in the DB field.
+ * @param string $name the raw question name.
+ * @return string a safe question name.
+ */
+ public function clean_question_name($name) {
+ $name = clean_param($name, PARAM_TEXT); // Matches what the question editing form does.
+ $name = trim($name);
+ $trimlength = 251;
+ while (textlib::strlen($name) > 255 && $trimlength > 0) {
+ $name = shorten_text($name, $trimlength);
+ $trimlength -= 10;
+ }
+ return $name;
+ }
+
function defaultquestion() {
// returns an "empty" question
// Somewhere to specify question parameters that are not handled
View
@@ -661,6 +661,37 @@ protected function defaultquestion() {
}
/**
+ * Construct a reasonable default question name, based on the start of the question text.
+ * @param string $questiontext the question text.
+ * @param string $default default question name to use if the constructed one comes out blank.
+ * @return string a reasonable question name.
+ */
+ public function create_default_question_name($questiontext, $default) {
+ $name = $this->clean_question_name(shorten_text($questiontext, 80));
+ if ($name) {
+ return $name;
+ } else {
+ return $default;
+ }
+ }
+
+ /**
+ * Ensure that a question name does not contain anything nasty, and will fit in the DB field.
+ * @param string $name the raw question name.
+ * @return string a safe question name.
+ */
+ public function clean_question_name($name) {
+ $name = clean_param($name, PARAM_TEXT); // Matches what the question editing form does.
+ $name = trim($name);
+ $trimlength = 251;
+ while (textlib::strlen($name) > 255 && $trimlength > 0) {
+ $name = shorten_text($name, $trimlength);
+ $trimlength -= 10;
+ }
+ return $name;
+ }
+
+ /**
* Add a blank combined feedback to a question object.
* @param object question
* @return object question
@@ -95,7 +95,7 @@ public function readquestions($lines) {
} else {
// Must be the first line of a new question, since no recognised prefix.
$question->qtype = MULTICHOICE;
- $question->name = shorten_text(s($nowline), 50);
+ $question->name = $this->create_default_question_name($nowline, get_string('questionname', 'question'));
$question->questiontext = htmlspecialchars(trim($nowline), ENT_NOQUOTES);
$question->questiontextformat = FORMAT_HTML;
$question->generalfeedback = '';
@@ -123,13 +123,9 @@ public function process_common($questiondata) {
$question->questiontext = $text;
}
// Put name in question object. We must ensure it is not empty and it is less than 250 chars.
- $question->name = shorten_text(strip_tags($question->questiontext), 200);
- $question->name = substr($question->name, 0, 250);
- if (!$question->name) {
- $id = $this->getpath($questiondata,
- array('@', 'id'), '', true);
- $question->name = get_string('defaultname', 'qformat_blackboard' , $id);
- }
+ $id = $this->getpath($questiondata, array('@', 'id'), '', true);
+ $question->name = $this->create_default_question_name($question->questiontext,
+ get_string('defaultname', 'qformat_blackboard', $id));
$question->generalfeedback = '';
$question->generalfeedbackformat = FORMAT_HTML;
@@ -92,13 +92,9 @@ public function process_common($questiondata) {
$question->questiontextformat = FORMAT_HTML; // Needed because add_blank_combined_feedback uses it.
// Put name in question object. We must ensure it is not empty and it is less than 250 chars.
- $question->name = shorten_text(strip_tags($question->questiontext['text']), 200);
- $question->name = substr($question->name, 0, 250);
- if (!$question->name) {
- $id = $this->getpath($questiondata,
- array('@', 'id'), '', true);
- $question->name = get_string('defaultname', 'qformat_blackboard_six' , $id);
- }
+ $id = $this->getpath($questiondata, array('@', 'id'), '', true);
+ $question->name = $this->create_default_question_name($question->questiontext['text'],
+ get_string('defaultname', 'qformat_blackboard_six' , $id));
$question->generalfeedback = '';
$question->generalfeedbackformat = FORMAT_HTML;
@@ -510,11 +510,8 @@ public function process_common($quest) {
$question->questiontext = $this->cleaned_text_field($text);
$question->questiontextformat = FORMAT_HTML; // Needed because add_blank_combined_feedback uses it.
- $question->name = shorten_text(strip_tags($question->questiontext['text']), 200);
- $question->name = substr($question->name, 0, 250);
- if (!$question->name) {
- $question->name = get_string('defaultname', 'qformat_blackboard_six' , $quest->id);
- }
+ $question->name = $this->create_default_question_name($question->questiontext['text'],
+ get_string('defaultname', 'qformat_blackboard_six' , $quest->id));
$question->generalfeedback = '';
$question->generalfeedbackformat = FORMAT_HTML;
$question->generalfeedbackfiles = array();
@@ -131,7 +131,7 @@ protected function process_matches(&$questions) {
$question->questiontext = $htmltext;
$question->questiontextformat = FORMAT_HTML;
$question->questiontextfiles = array();
- $question->name = shorten_text( $question->questiontext, 250 );
+ $question->name = $this->create_default_question_name($question->questiontext, get_string('questionname', 'question'));
$question->qtype = MATCH;
$question = $this->add_blank_combined_feedback($question);
$question->subquestions = array();
@@ -200,7 +200,7 @@ public function readquestion($qrec) {
$question->questiontext = $this->cleaninput($htmltext);
$question->questiontextformat = FORMAT_HTML;
$question->questiontextfiles = array();
- $question->name = shorten_text( $question->questiontext, 250 );
+ $question->name = $this->create_default_question_name($question->questiontext, get_string('questionname', 'question'));
switch ($question->qtype) {
case MULTICHOICE:
@@ -206,7 +206,7 @@ public function readquestion($lines) {
// name will be assigned after processing question text below
} else {
$questionname = substr($text, 0, $namefinish);
- $question->name = trim($this->escapedchar_post($questionname));
+ $question->name = $this->clean_question_name($this->escapedchar_post($questionname));
$text = trim(substr($text, $namefinish+2)); // Remove name from text
}
} else {
@@ -251,13 +251,9 @@ public function readquestion($lines) {
// set question name if not already set
if ($question->name === false) {
- $question->name = $question->questiontext;
+ $question->name = $this->create_default_question_name($question->questiontext, get_string('questionname', 'question'));
}
- // ensure name is not longer than 250 characters
- $question->name = shorten_text($question->name, 200);
- $question->name = strip_tags(substr($question->name, 0, 250));
-
// determine QUESTION TYPE
$question->qtype = NULL;
@@ -126,10 +126,7 @@ function readquestion($lines) {
$question = $this->defaultquestion();
$question->qtype = MULTICHOICE;
- $question->name = substr($questiontext, 0, 30);
- if (strlen($questiontext) > 30) {
- $question->name .= '...';
- }
+ $question->name = $this->create_default_question_name($questiontext, get_string('questionname', 'question'));
$question->questiontext = $questiontext;
$question->single = ($type == 'multichoice') ? 1 : 0;
@@ -89,7 +89,7 @@ function readquestion($lines) {
/// Save the new question text
$question->questiontext = substr_replace($text, "_____", $answerstart, $answerlength+1);
- $question->name = $question->questiontext;
+ $question->name = $this->create_default_question_name($question->questiontext, get_string('questionname', 'question'));
/// Parse the answers
@@ -62,18 +62,7 @@ public function readquestions($lines) {
$question->penalty = 0.3333333;
if (!empty($question)) {
- $name = html_to_text(implode(' ', $lines));
- $name = preg_replace('/{[^}]*}/', '', $name);
- $name = trim($name);
-
- if ($name) {
- $question->name = shorten_text($name, 45);
- } else {
- // We need some name, so use the current time, since that will be
- // reasonably unique.
- $question->name = userdate(time());
- }
-
+ $question->name = $this->create_default_question_name($question->questiontext, get_string('questionname', 'question'));
$questions[] = $question;
}
@@ -47,7 +47,9 @@ public function test_import() {
$qs = $importer->readquestions($lines);
$expectedq = (object) array(
- 'name' => 'Match the following cities with the ...',
+ 'name' => 'Match the following cities with the correct state:
+* San Francisco: {#1}
+* ...',
'questiontext' => 'Match the following cities with the correct state:
* San Francisco: {#1}
* Tucson: {#2}
@@ -259,11 +259,8 @@ protected function readquestions($lines) {
// Setup default value of missing fields
if (!isset($question->name)) {
- $question->name = $question->questiontext;
- }
- if (strlen($question->name) > 255) {
- $question->name = substr($question->name,0,250)."...";
- $warnings[] = get_string("questionnametoolong", "qformat_webct", $nQuestionStartLine);
+ $question->name = $this->create_default_question_name(
+ $question->questiontext, get_string('questionname', 'question'));
}
if (!isset($question->defaultmark)) {
$question->defaultmark = 1;
@@ -466,11 +463,7 @@ protected function readquestions($lines) {
if (preg_match("~^:TITLE:(.*)~i",$line,$webct_options)) {
$name = trim($webct_options[1]);
- if (strlen($name) > 255) {
- $name = substr($name,0,250)."...";
- $warnings[] = get_string("questionnametoolong", "qformat_webct", $nLineCounter);
- }
- $question->name = $name;
+ $question->name = $this->clean_question_name($name);
continue;
}
@@ -166,9 +166,9 @@ public function import_headers($question) {
$qo = $this->defaultquestion();
// Question name
- $qo->name = $this->getpath($question,
+ $qo->name = $this->clean_question_name($this->getpath($question,
array('#', 'name', 0, '#', 'text', 0, '#'), '', true,
- get_string('xmlimportnoname', 'qformat_xml'));
+ get_string('xmlimportnoname', 'qformat_xml')));
$qo->questiontext = $this->getpath($question,
array('#', 'questiontext', 0, '#', 'text', 0, '#'), '', true);
$qo->questiontextformat = $this->trans_format($this->getpath(
@@ -437,7 +437,7 @@ public function import_multianswer($question) {
$qo->course = $this->course;
$qo->generalfeedback = '';
- $qo->name = $this->import_text($question['#']['name'][0]['#']['text']);
+ $qo->name = $this->clean_question_name($this->import_text($question['#']['name'][0]['#']['text']));
$qo->questiontextformat = $questiontext['format'];
$qo->questiontext = $qo->questiontext['text'];
$qo->questiontextfiles = array();
@@ -87,4 +87,71 @@ public function test_split_category_path_cleans() {
$path = '<evil>Nasty <virus //> thing<//evil>';
$this->assertEquals(array('Nasty thing'), $format->split_category_path($path));
}
+
+ public function test_clean_question_name() {
+ $format = new testable_qformat();
+
+ $name = 'Nice simple name';
+ $this->assertEquals($name, $format->clean_question_name($name));
+
+ $name = 'Question in <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">French</span>';
+ $this->assertEquals($name, $format->clean_question_name($name));
+
+ $name = 'Evil <script type="text/javascrip">alert("You have been hacked!");</script>';
+ $this->assertEquals('Evil alert("You have been hacked!");', $format->clean_question_name($name));
+
+ $name = 'This is a very long question name. It goes on and on and on. ' .
+ 'I wonder if it will ever stop. The quetsion name field in the database is only ' .
+ 'two hundred and fifty five characters wide, so if the import file contains a ' .
+ 'name longer than that, the code had better truncate it!';
+ $this->assertEquals(shorten_text($name, 251), $format->clean_question_name($name));
+
+ // The worst case scenario is a whole lot of single charaters in separate multilang tags.
+ $name = '<span lang="en" class="multilang">A</span>' .
+ '<span lang="fr" class="multilang">B</span>' .
+ '<span lang="fr_ca" class="multilang">C</span>' .
+ '<span lang="en_us" class="multilang">D</span>' .
+ '<span lang="de" class="multilang">E</span>' .
+ '<span lang="cz" class="multilang">F</span>' .
+ '<span lang="it" class="multilang">G</span>' .
+ '<span lang="es" class="multilang">H</span>' .
+ '<span lang="pt" class="multilang">I</span>' .
+ '<span lang="ch" class="multilang">J</span>';
+ $this->assertEquals(shorten_text($name, 1), $format->clean_question_name($name));
+ }
+
+ public function test_create_default_question_name() {
+ $format = new testable_qformat();
+
+ $text = 'Nice simple name';
+ $this->assertEquals($text, $format->create_default_question_name($text, 'Default'));
+
+ $this->assertEquals('Default', $format->create_default_question_name('', 'Default'));
+
+ $text = 'Question in <span lang="en" class="multilang">English</span><span lang="fr" class="multilang">French</span>';
+ $this->assertEquals($text, $format->create_default_question_name($text, 'Default'));
+
+ $text = 'Evil <script type="text/javascrip">alert("You have been hacked!");</script>';
+ $this->assertEquals('Evil alert("You have been hacked!");',
+ $format->create_default_question_name($text, 'Default'));
+
+ $text = 'This is a very long question text. It goes on and on and on. ' .
+ 'I wonder if it will ever stop. The question name field in the database is only ' .
+ 'two hundred and fifty five characters wide, so if the import file contains a ' .
+ 'name longer than that, the code had better truncate it!';
+ $this->assertEquals(shorten_text($text, 80), $format->create_default_question_name($text, 'Default'));
+
+ // The worst case scenario is a whole lot of single charaters in separate multilang tags.
+ $text = '<span lang="en" class="multilang">A</span>' .
+ '<span lang="fr" class="multilang">B</span>' .
+ '<span lang="fr_ca" class="multilang">C</span>' .
+ '<span lang="en_us" class="multilang">D</span>' .
+ '<span lang="de" class="multilang">E</span>' .
+ '<span lang="cz" class="multilang">F</span>' .
+ '<span lang="it" class="multilang">G</span>' .
+ '<span lang="es" class="multilang">H</span>' .
+ '<span lang="pt" class="multilang">I</span>' .
+ '<span lang="ch" class="multilang">J</span>';
+ $this->assertEquals(shorten_text($text, 1), $format->create_default_question_name($text, 'Default'));
+ }
}

0 comments on commit 5df0a6b

Please sign in to comment.