From cacb8fa08aa81effc22223feb89015a68cf74b48 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 11 Sep 2012 15:04:00 +0100 Subject: [PATCH] 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. --- mod/lesson/format.php | 31 ++++++++++ question/format.php | 31 ++++++++++ question/format/aiken/format.php | 2 +- question/format/blackboard/format.php | 10 +--- question/format/blackboard_six/formatpool.php | 10 +--- question/format/blackboard_six/formatqti.php | 7 +-- question/format/examview/format.php | 4 +- question/format/gift/format.php | 8 +-- question/format/learnwise/format.php | 5 +- question/format/missingword/format.php | 2 +- question/format/multianswer/format.php | 13 +---- .../multianswer/tests/multianswerformat_test.php | 4 +- question/format/webct/format.php | 13 +---- question/format/xml/format.php | 6 +- question/tests/importexport_test.php | 67 ++++++++++++++++++++++ 15 files changed, 154 insertions(+), 59 deletions(-) diff --git a/mod/lesson/format.php b/mod/lesson/format.php index a7c7307624b..f1027ba700a 100644 --- a/mod/lesson/format.php +++ b/mod/lesson/format.php @@ -520,6 +520,37 @@ class qformat_default { 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 diff --git a/question/format.php b/question/format.php index 980f20973b7..10f093e8a7d 100644 --- a/question/format.php +++ b/question/format.php @@ -638,6 +638,37 @@ class qformat_default { } /** + * 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 diff --git a/question/format/aiken/format.php b/question/format/aiken/format.php index 8c3cbc99883..35d5afc0ea1 100644 --- a/question/format/aiken/format.php +++ b/question/format/aiken/format.php @@ -95,7 +95,7 @@ class qformat_aiken extends qformat_default { } 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 = ''; diff --git a/question/format/blackboard/format.php b/question/format/blackboard/format.php index 1e7fa0dd779..9d30323f773 100644 --- a/question/format/blackboard/format.php +++ b/question/format/blackboard/format.php @@ -123,13 +123,9 @@ class qformat_blackboard extends qformat_based_on_xml { $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; diff --git a/question/format/blackboard_six/formatpool.php b/question/format/blackboard_six/formatpool.php index 80867f34b85..a6c942c5212 100644 --- a/question/format/blackboard_six/formatpool.php +++ b/question/format/blackboard_six/formatpool.php @@ -92,13 +92,9 @@ class qformat_blackboard_six_pool extends qformat_blackboard_six_base { $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; diff --git a/question/format/blackboard_six/formatqti.php b/question/format/blackboard_six/formatqti.php index 223d9f608ca..37545fd3020 100644 --- a/question/format/blackboard_six/formatqti.php +++ b/question/format/blackboard_six/formatqti.php @@ -510,11 +510,8 @@ class qformat_blackboard_six_qti extends qformat_blackboard_six_base { $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(); diff --git a/question/format/examview/format.php b/question/format/examview/format.php index d13126619f6..21554012c4c 100644 --- a/question/format/examview/format.php +++ b/question/format/examview/format.php @@ -131,7 +131,7 @@ class qformat_examview extends qformat_based_on_xml { $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 @@ class qformat_examview extends qformat_based_on_xml { $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': diff --git a/question/format/gift/format.php b/question/format/gift/format.php index eb608393b1a..0bfc0381ea5 100644 --- a/question/format/gift/format.php +++ b/question/format/gift/format.php @@ -206,7 +206,7 @@ class qformat_gift extends qformat_default { // 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 { @@ -265,13 +265,9 @@ class qformat_gift extends qformat_default { // 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; diff --git a/question/format/learnwise/format.php b/question/format/learnwise/format.php index 3bd31b418e2..0fbc4508cdd 100644 --- a/question/format/learnwise/format.php +++ b/question/format/learnwise/format.php @@ -126,10 +126,7 @@ class qformat_learnwise extends qformat_default { $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; diff --git a/question/format/missingword/format.php b/question/format/missingword/format.php index a1b3cea580d..c79998df2ee 100644 --- a/question/format/missingword/format.php +++ b/question/format/missingword/format.php @@ -89,7 +89,7 @@ class qformat_missingword extends qformat_default { /// 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 diff --git a/question/format/multianswer/format.php b/question/format/multianswer/format.php index d7a7ac788a4..b7c412ea056 100644 --- a/question/format/multianswer/format.php +++ b/question/format/multianswer/format.php @@ -62,18 +62,7 @@ class qformat_multianswer extends qformat_default { $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; } diff --git a/question/format/multianswer/tests/multianswerformat_test.php b/question/format/multianswer/tests/multianswerformat_test.php index 29c32241cb1..8e1d301f8bf 100644 --- a/question/format/multianswer/tests/multianswerformat_test.php +++ b/question/format/multianswer/tests/multianswerformat_test.php @@ -47,7 +47,9 @@ class qformat_multianswer_test extends question_testcase { $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} diff --git a/question/format/webct/format.php b/question/format/webct/format.php index 6facb80fce8..8692542e13b 100644 --- a/question/format/webct/format.php +++ b/question/format/webct/format.php @@ -259,11 +259,8 @@ class qformat_webct extends qformat_default { // 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 @@ class qformat_webct extends qformat_default { 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; } diff --git a/question/format/xml/format.php b/question/format/xml/format.php index 7cdb23c5a8e..9b372154558 100644 --- a/question/format/xml/format.php +++ b/question/format/xml/format.php @@ -166,9 +166,9 @@ class qformat_xml extends qformat_default { $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 @@ class qformat_xml extends qformat_default { $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(); diff --git a/question/tests/importexport_test.php b/question/tests/importexport_test.php index 0ffe4358f52..96d0704782b 100644 --- a/question/tests/importexport_test.php +++ b/question/tests/importexport_test.php @@ -87,4 +87,71 @@ class qformat_default_test extends advanced_testcase { $path = 'Nasty thing'; $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 EnglishFrench'; + $this->assertEquals($name, $format->clean_question_name($name)); + + $name = 'Evil '; + $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 = 'A' . + 'B' . + 'C' . + 'D' . + 'E' . + 'F' . + 'G' . + 'H' . + 'I' . + 'J'; + $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 EnglishFrench'; + $this->assertEquals($text, $format->create_default_question_name($text, 'Default')); + + $text = 'Evil '; + $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 = 'A' . + 'B' . + 'C' . + 'D' . + 'E' . + 'F' . + 'G' . + 'H' . + 'I' . + 'J'; + $this->assertEquals(shorten_text($text, 1), $format->create_default_question_name($text, 'Default')); + } } -- 2.11.4.GIT