diff --git a/mod/qbank/classes/task/transfer_question_categories.php b/mod/qbank/classes/task/transfer_question_categories.php index cc486249264..958e0b0a3be 100644 --- a/mod/qbank/classes/task/transfer_question_categories.php +++ b/mod/qbank/classes/task/transfer_question_categories.php @@ -105,17 +105,17 @@ class transfer_question_categories extends adhoc_task { switch ($oldcontext->contextlevel) { case CONTEXT_SYSTEM: $course = get_site(); - $bankname = get_string('systembank', 'question'); + $bankname = question_bank_helper::get_bank_name_string('systembank', 'question'); break; case CONTEXT_COURSECAT: $coursecategory = core_course_category::get($oldcontext->instanceid); $courseshortname = "{$coursecategory->name}-{$coursecategory->id}"; $course = $this->create_course($coursecategory, $courseshortname); - $bankname = get_string("sharedbank", "mod_qbank", $coursecategory->name); + $bankname = question_bank_helper::get_bank_name_string('sharedbank', 'mod_qbank', $coursecategory->name); break; case CONTEXT_COURSE: $course = get_course($oldcontext->instanceid); - $bankname = get_string("sharedbank", "mod_qbank", $course->shortname); + $bankname = question_bank_helper::get_bank_name_string('sharedbank', 'mod_qbank', $course->shortname); break; default: // This shouldn't be possible, so we can't really transfer it. diff --git a/mod/qbank/mod_form.php b/mod/qbank/mod_form.php index 792755efb31..465a6f59572 100644 --- a/mod/qbank/mod_form.php +++ b/mod/qbank/mod_form.php @@ -55,6 +55,7 @@ class mod_qbank_mod_form extends moodleform_mod { } $mform->addHelpButton('name', 'qbankname', 'mod_qbank'); $mform->addRule('name', null, 'required', null, 'client'); + $mform->addRule('name', null, 'maxlength', \core_question\local\bank\question_bank_helper::BANK_NAME_MAX_LENGTH, 'client'); // Add intro editor. $mform->addElement('editor', 'introeditor', get_string('moduleintro'), ['rows' => 10], [ diff --git a/question/banks.php b/question/banks.php index d28e2daf837..d86f0aac41b 100644 --- a/question/banks.php +++ b/question/banks.php @@ -57,7 +57,7 @@ if ($createdefault) { require_sesskey(); question_bank_helper::create_default_open_instance( $course, - get_string('defaultbank', 'core_question', ['coursename' => $course->fullname]) + question_bank_helper::get_bank_name_string('defaultbank', 'core_question', ['coursename' => $course->fullname]), ); \core\notification::add(get_string('defaultcreated', 'question'), \core\notification::SUCCESS); redirect($pageurl); diff --git a/question/classes/local/bank/question_bank_helper.php b/question/classes/local/bank/question_bank_helper.php index b0c78999116..6470692fdae 100644 --- a/question/classes/local/bank/question_bank_helper.php +++ b/question/classes/local/bank/question_bank_helper.php @@ -65,6 +65,11 @@ class question_bank_helper { */ private const CATEGORY_DELIMITER = '<->'; + /** + * Maximum length for the question bank name database field. + */ + public const BANK_NAME_MAX_LENGTH = 255; + /** * Modules that share questions via FEATURE_PUBLISHES_QUESTIONS. * @@ -411,7 +416,11 @@ class question_bank_helper { } if (!$systembank && $createifnotexists) { - $systembank = self::create_default_open_instance($course, get_string('systembank', 'question'), self::TYPE_SYSTEM); + $systembank = self::create_default_open_instance( + $course, + self::get_bank_name_string('systembank', 'question'), + self::TYPE_SYSTEM, + ); } return $systembank; @@ -440,7 +449,11 @@ class question_bank_helper { } if (!$previewbank && $createifnotexists) { - $previewbank = self::create_default_open_instance($site, get_string('previewbank', 'question'), self::TYPE_PREVIEW); + $previewbank = self::create_default_open_instance( + $site, + self::get_bank_name_string('previewbank', 'question'), + self::TYPE_PREVIEW + ); } return $previewbank; @@ -521,6 +534,13 @@ class question_bank_helper { } } + if (strlen($bankname) > self::BANK_NAME_MAX_LENGTH) { + throw new \coding_exception( + 'The provided bankname is too long for the database field.', + 'Use question_bank_helper::get_bank_name_string to get a suitably truncated name.', + ); + } + $data = new stdClass(); $data->section = 0; $data->visible = 0; @@ -585,4 +605,39 @@ class question_bank_helper { global $CFG; return $CFG->corequestion_defaultqbankmod ?? 'qbank'; } + + /** + * Get the requested language string, with parameters truncated to ensure the result fits in the database. + * + * Since we may be generating a question bank name based on an existing course or category name, we need to ensure + * that the resulting string isn't longer than the maximum module name. + * + * @param string $identifier The string identifier + * @param string $component The string component + * @param mixed|null $params The string parameters (a single string, array or object as accepted by get_string) + * @return string The string truncated to a length that will fit in the database. + */ + public static function get_bank_name_string(string $identifier, string $component, mixed $params = null): string { + if (is_object($params)) { + $shortparams = (array) $params; + } else { + $shortparams = $params; + } + $bankname = get_string($identifier, $component, $shortparams); + if (!is_null($shortparams)) { + $trimlength = self::BANK_NAME_MAX_LENGTH - 4; + while (\core_text::strlen($bankname) > self::BANK_NAME_MAX_LENGTH && $trimlength > 0) { + // Gradually shorten the string parameters until the resulting string is short enough. + if (is_array($shortparams)) { + $shortparams = array_map(fn($param) => shorten_text(trim($param), $trimlength), $shortparams); + } else { + $shortparams = shorten_text(trim($shortparams), $trimlength); + } + $bankname = get_string($identifier, $component, $shortparams); + $trimlength -= 10; + } + } + // As a failsafe, limit the length of the final string in case the lang string is too long. + return shorten_text($bankname, self::BANK_NAME_MAX_LENGTH); + } } diff --git a/question/tests/local/bank/question_bank_helper_test.php b/question/tests/local/bank/question_bank_helper_test.php index 13bbe1b7b34..9a4e5b00724 100644 --- a/question/tests/local/bank/question_bank_helper_test.php +++ b/question/tests/local/bank/question_bank_helper_test.php @@ -200,6 +200,28 @@ final class question_bank_helper_test extends \advanced_testcase { $this->assertEquals(1, $cminfo->showdescription); } + /** + * Create a default instance, passing a name that is too long for the database. + * + * @return void + * @throws \coding_exception + * @throws \dml_exception + * @throws \moodle_exception + */ + public function test_create_default_open_instance_with_long_name(): void { + $this->resetAfterTest(); + self::setAdminUser(); + + $coursename = random_string(question_bank_helper::BANK_NAME_MAX_LENGTH); + $course = self::getDataGenerator()->create_course(['shortname' => $coursename]); + + $this->expectExceptionMessage('The provided bankname is too long for the database field.'); + question_bank_helper::create_default_open_instance( + $course, + get_string('defaultbank', 'core_question', ['coursename' => $coursename]), + ); + } + /** * Assert that viewing a question bank logs the view for that user up to a maximum of 5 unique bank views. * @@ -310,4 +332,80 @@ final class question_bank_helper_test extends \advanced_testcase { $modrecord = $DB->get_record('qbank', ['id' => $qbank->instance]); $this->assertEquals(question_bank_helper::TYPE_SYSTEM, $modrecord->type); } + + /** + * Assert that get_bank_name_string returns suitably truncated strings. + * + * @dataProvider bank_name_strings + * @param string $identifier + * @param string $component + * @param mixed $params + * @param string $expected + */ + public function test_get_bank_name_string(string $identifier, string $component, mixed $params, string $expected): void { + $this->assertEquals($expected, question_bank_helper::get_bank_name_string($identifier, $component, $params)); + } + + /** + * Get string examples with different parameter types and lengths. + * + * @return array[] + */ + public static function bank_name_strings(): array { + $longname = 'One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen ' . + 'eighteen nineteen twenty twenty-one twenty-two twenty-three twenty-four twenty-five twenty-six twenty-seven ' . + 'twenty-eight twenty-nine thirty thirty-one'; + return [ + 'String with no parameters' => [ + 'systembank', + 'question', + null, + 'System shared question bank', + ], + 'String with short string parameter' => [ + 'topfor', + 'question', + 'Test course', + 'Top for Test course', + ], + 'String with long string parameter' => [ + 'topfor', + 'question', + $longname, + 'Top for One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen ' . + 'seventeen eighteen nineteen twenty twenty-one twenty-two twenty-three twenty-four twenty-five twenty-six ' . + 'twenty-seven twenty-eight ...', + ], + 'String with short array parameter' => [ + 'defaultbank', + 'question', + ['coursename' => 'Test course'], + 'Test course course question bank', + ], + 'String with long array parameter' => [ + 'defaultbank', + 'question', + ['coursename' => $longname], + 'One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen ' . + 'eighteen nineteen twenty twenty-one twenty-two twenty-three twenty-four twenty-five twenty-six ' . + 'twenty-seven twenty-eight ... course question bank', + ], + 'String with multiple long array parameters' => [ + 'markoutofmax', + 'question', + ['mark' => $longname, 'max' => $longname], + 'Mark One two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen ' . + 'eighteen ... out of One two three four five six seven eight nine ten eleven twelve thirteen fourteen ' . + 'fifteen sixteen seventeen eighteen ...', + ], + 'Long lang string' => [ + 'howquestionsbehave_help', + 'question', + null, + 'Students can interact with the questions in the quiz in various different ways. For example, you may wish the ' . + 'students to enter an answer to each question and then submit the entire quiz, before anything is graded or ' . + 'they get any feedback. That would ...', + ], + ]; + } }