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.

diff --git a/mod/lesson/format.php b/mod/lesson/format.php
index 95bb645..134c14b 100644 (file)
--- a/mod/lesson/format.php
+++ b/mod/lesson/format.php
@@ -541,6 +541,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 dd02cfc..13c1232 100644 (file)
--- a/question/format.php
+++ b/question/format.php
@@ -660,6 +660,37 @@ class qformat_default {
         return $question;
     }
 
+    /**
+     * 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

diff --git a/question/format/aiken/format.php b/question/format/aiken/format.php
index 4e64c2f..968a211 100644 (file)
--- 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 1e7fa0d..9d30323 100644 (file)
--- 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 80867f3..a6c942c 100644 (file)
--- 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 223d9f6..37545fd 100644 (file)
--- 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 9cb9bb5..2f3eccd 100644 (file)
--- 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 1cb0e25..ecca6e2 100644 (file)
--- 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 {
@@ -251,13 +251,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 fa80475..ae80f13 100644 (file)
--- 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 15e1d71..d4347bc 100644 (file)
--- 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 d7a7ac7..b7c412e 100644 (file)
--- 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 29c3224..8e1d301 100644 (file)
--- 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 c599ca8..28841e8 100644 (file)
--- 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 740279f..1a0e992 100644 (file)
--- 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 0ffe435..96d0704 100644 (file)
--- a/question/tests/importexport_test.php
+++ b/question/tests/importexport_test.php
@@ -87,4 +87,71 @@ class qformat_default_test extends advanced_testcase {
         $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'));
+    }
 }