diff --git a/course/classes/list_element.php b/course/classes/list_element.php index 7f8f8137642..df124e363f3 100644 --- a/course/classes/list_element.php +++ b/course/classes/list_element.php @@ -349,7 +349,11 @@ class core_course_list_element implements IteratorAggregate { * @return string */ public function get_formatted_name() { - return format_string(get_course_display_name_for_list($this), true, $this->get_context()); + return format_string( + get_course_display_name_for_list($this), + true, + ['context' => $this->get_context()], + ); } /** @@ -357,7 +361,11 @@ class core_course_list_element implements IteratorAggregate { * @return string */ public function get_formatted_fullname() { - return format_string($this->__get('fullname'), true, $this->get_context()); + return format_string( + $this->__get('fullname'), + true, + ['context' => $this->get_context()], + ); } /** @@ -365,7 +373,11 @@ class core_course_list_element implements IteratorAggregate { * @return string */ public function get_formatted_shortname() { - return format_string($this->__get('shortname'), true, $this->get_context()); + return format_string( + $this->__get('shortname'), + true, + ['context' => $this->get_context()], + ); } /** diff --git a/group/autogroup_form.php b/group/autogroup_form.php index 31340b06164..cbf290ebd2b 100644 --- a/group/autogroup_form.php +++ b/group/autogroup_form.php @@ -93,7 +93,9 @@ class autogroup_form extends moodleform { if ($cohorts = cohort_get_available_cohorts($coursecontext, COHORT_WITH_ENROLLED_MEMBERS_ONLY, 0, 0)) { $options = array(0 => get_string('anycohort', 'cohort')); foreach ($cohorts as $c) { - $options[$c->id] = format_string($c->name, true, context::instance_by_id($c->contextid)); + $options[$c->id] = format_string($c->name, true, [ + 'context' => context::instance_by_id($c->contextid), + ]); } $mform->addElement('select', 'cohortid', get_string('selectfromcohort', 'cohort'), $options); $mform->setDefault('cohortid', '0'); diff --git a/lib/form/cohort.php b/lib/form/cohort.php index 88945a79366..bbe0c406f29 100644 --- a/lib/form/cohort.php +++ b/lib/form/cohort.php @@ -141,7 +141,7 @@ class MoodleQuickForm_cohort extends MoodleQuickForm_autocomplete { if (!cohort_can_view_cohort($cohort, $currentcontext)) { continue; } - $label = format_string($cohort->name, true, $currentcontext); + $label = format_string($cohort->name, true, ['context' => $currentcontext]); $this->addOption($label, $cohort->id); } diff --git a/lib/tests/weblib_test.php b/lib/tests/weblib_test.php index f34b81e5066..342bf43e097 100644 --- a/lib/tests/weblib_test.php +++ b/lib/tests/weblib_test.php @@ -168,6 +168,113 @@ class weblib_test extends advanced_testcase { } } + /** + * Test the format_string illegal options handling. + * + * @covers ::format_string + * @dataProvider format_string_illegal_options_provider + */ + public function test_format_string_illegal_options( + string $input, + string $result, + mixed $options, + string $pattern, + ): void { + $this->assertEquals( + $result, + format_string($input, false, $options), + ); + + $messages = $this->getDebuggingMessages(); + $this->assertdebuggingcalledcount(1); + $this->assertMatchesRegularExpression( + "/{$pattern}/", + $messages[0]->message, + ); + } + + /** + * Data provider for test_format_string_illegal_options. + * @return array + */ + public static function format_string_illegal_options_provider(): array { + return [ + [ + 'Example', + 'Example', + \core\context\system::instance(), + preg_quote('The options argument should not be a context object directly.'), + ], + [ + 'Example', + 'Example', + true, + preg_quote('The options argument should be an Array, or stdclass. boolean passed.'), + ], + [ + 'Example', + 'Example', + false, + preg_quote('The options argument should be an Array, or stdclass. boolean passed.'), + ], + ]; + } + + /** + * Ensure that if format_string is called with a context as the third param, that a debugging notice is emitted. + * + * @covers ::format_string + */ + public function test_format_string_context(): void { + global $CFG; + + $this->resetAfterTest(true); + + // Disable the formatstringstriptags option to ensure that the HTML tags are not stripped. + $CFG->stringfilters = 'multilang'; + + // Enable filters. + $CFG->filterall = true; + + $course = $this->getDataGenerator()->create_course(); + $coursecontext = \core\context\course::instance($course->id); + + // Set up the multilang filter at the system context, but disable it at the course. + filter_set_global_state('multilang', TEXTFILTER_ON); + filter_set_local_state('multilang', $coursecontext->id, TEXTFILTER_OFF); + + // Previously, if a context was passed, it was converted into an Array, and ignored. + // The PAGE context was used instead -- often this is the system context. + $input = 'I really do not do not like this!'; + + $result = format_string( + $input, + true, + $coursecontext, + ); + + // We emit a debugging notice to alert that the context has been moved to the options. + $this->assertdebuggingcalledcount(1); + + // Check the result was _not_ filtered. + $this->assertEquals( + // Tags are stripped out due to striptags. + "I really do not do not like this!", + $result, + ); + + // But it should be filtered if called with the system context. + $result = format_string( + $input, + true, + ['context' => \core\context\system::instance()], + ); + $this->assertEquals( + 'I really do not like this!', + $result, + ); + } + /** * @covers ::format_text_email */ diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 302721c5b41..a273d4138ae 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -10,6 +10,8 @@ information provided here is intended especially for developers. drag/drop, which can be used to add transition effects in calling code * course_modinfo now has a purge_course_modules_cache() method, which takes a list of cmids and purges them all in a single cache set. +* The options for `format_string()` are now checked for incorrectly passed context objects. + Please note that this was never an accepted value but previously failed silently. === 4.3 === diff --git a/lib/weblib.php b/lib/weblib.php index ec421819584..26b63c739d9 100644 --- a/lib/weblib.php +++ b/lib/weblib.php @@ -1514,12 +1514,35 @@ function format_string($string, $striplinks = true, $options = null) { $strcache = array(); } - if (is_numeric($options)) { + // This method only expects either: + // - an array of options; + // - a stdClass of options to be cast to an array; or + // - an integer courseid. + if ($options === null) { + $options = []; + } else if (is_numeric($options)) { // Legacy courseid usage. - $options = array('context' => context_course::instance($options)); + $options = ['context' => \core\context\course::instance($options)]; + } else if ($options instanceof \core\context) { + // A common mistake has been to call this function with a context object. + // This has never been expected, or nor supported. + debugging( + 'The options argument should not be a context object directly. ' . + ' Please pass an array with a context key instead.', + DEBUG_DEVELOPER, + ); + $options = ['context' => $options]; + } else if (is_array($options) || is_a($options, \stdClass::class)) { + // Re-cast to array to prevent modifications to the original object. + $options = (array) $options; } else { - // Detach object, we can not modify it. - $options = (array)$options; + // Something else was passed, so we'll just use an empty array. + // Attempt to cast to array since we always used to, but throw in some debugging. + debugging(sprintf( + 'The options argument should be an Array, or stdclass. %s passed.', + gettype($options), + ), DEBUG_DEVELOPER); + $options = (array) $options; } if (empty($options['context'])) { diff --git a/mod/assign/classes/output/renderer.php b/mod/assign/classes/output/renderer.php index bdbdad78e5d..8a326117baf 100644 --- a/mod/assign/classes/output/renderer.php +++ b/mod/assign/classes/output/renderer.php @@ -656,7 +656,7 @@ class renderer extends \plugin_renderer_base { $cell1content = get_string('submissionteam', 'assign'); $group = $status->submissiongroup; if ($group) { - $cell2content = format_string($group->name, false, $status->context); + $cell2content = format_string($group->name, false, ['context' => $status->context]); } else if ($status->preventsubmissionnotingroup) { if (count($status->usergroups) == 0) { $notification = new \core\output\notification(get_string('noteam', 'assign'), 'error'); diff --git a/mod/assign/override_form.php b/mod/assign/override_form.php index 1ed1baeb45a..e30e4d4234b 100644 --- a/mod/assign/override_form.php +++ b/mod/assign/override_form.php @@ -107,7 +107,7 @@ class assign_override_form extends moodleform { if ($this->groupid) { // There is already a groupid, so freeze the selector. $groupchoices = [ - $this->groupid => format_string(groups_get_group_name($this->groupid), true, $this->context), + $this->groupid => format_string(groups_get_group_name($this->groupid), true, ['context' => $this->context]), ]; $mform->addElement('select', 'groupid', get_string('overridegroup', 'assign'), $groupchoices); @@ -129,7 +129,7 @@ class assign_override_form extends moodleform { $groupchoices = array(); foreach ($groups as $group) { if ($group->visibility != GROUPS_VISIBILITY_NONE) { - $groupchoices[$group->id] = format_string($group->name, true, $this->context); + $groupchoices[$group->id] = format_string($group->name, true, ['context' => $this->context]); } } unset($groups); diff --git a/mod/lesson/override_form.php b/mod/lesson/override_form.php index f115993d3fa..2aeae2203b1 100644 --- a/mod/lesson/override_form.php +++ b/mod/lesson/override_form.php @@ -96,7 +96,7 @@ class lesson_override_form extends moodleform { if ($this->groupid) { // There is already a groupid, so freeze the selector. $groupchoices = [ - $this->groupid => format_string(groups_get_group_name($this->groupid), true, $this->context), + $this->groupid => format_string(groups_get_group_name($this->groupid), true, ['context' => $this->context]), ]; $mform->addElement('select', 'groupid', get_string('overridegroup', 'lesson'), $groupchoices); @@ -114,7 +114,7 @@ class lesson_override_form extends moodleform { $groupchoices = array(); foreach ($groups as $group) { if ($group->visibility != GROUPS_VISIBILITY_NONE) { - $groupchoices[$group->id] = format_string($group->name, true, $this->context); + $groupchoices[$group->id] = format_string($group->name, true, ['context' => $this->context]); } } unset($groups); diff --git a/mod/lti/classes/privacy/provider.php b/mod/lti/classes/privacy/provider.php index 1a42186eff3..a96697608ef 100644 --- a/mod/lti/classes/privacy/provider.php +++ b/mod/lti/classes/privacy/provider.php @@ -398,7 +398,7 @@ class provider implements $ltiproxies = $DB->get_recordset('lti_tool_proxies', ['createdby' => $user->id], 'timecreated ASC'); foreach ($ltiproxies as $ltiproxy) { $data[] = [ - 'name' => format_string($ltiproxy->name, true, $systemcontext), + 'name' => format_string($ltiproxy->name, true, ['context' => $systemcontext]), 'createdby' => transform::user($ltiproxy->createdby), 'timecreated' => transform::datetime($ltiproxy->timecreated), 'timemodified' => transform::datetime($ltiproxy->timemodified) diff --git a/mod/quiz/classes/form/edit_override_form.php b/mod/quiz/classes/form/edit_override_form.php index eb7ef272de6..c227e5e0f40 100644 --- a/mod/quiz/classes/form/edit_override_form.php +++ b/mod/quiz/classes/form/edit_override_form.php @@ -96,7 +96,7 @@ class edit_override_form extends moodleform { if ($this->groupid) { // There is already a groupid, so freeze the selector. $groupchoices = [ - $this->groupid => format_string(groups_get_group_name($this->groupid), true, $this->context), + $this->groupid => format_string(groups_get_group_name($this->groupid), true, ['context' => $this->context]), ]; $mform->addElement('select', 'groupid', get_string('overridegroup', 'quiz'), $groupchoices); @@ -114,7 +114,7 @@ class edit_override_form extends moodleform { $groupchoices = []; foreach ($groups as $group) { if ($group->visibility != GROUPS_VISIBILITY_NONE) { - $groupchoices[$group->id] = format_string($group->name, true, $this->context); + $groupchoices[$group->id] = format_string($group->name, true, ['context' => $this->context]); } } unset($groups); diff --git a/question/bank/usage/classes/tables/question_usage_table.php b/question/bank/usage/classes/tables/question_usage_table.php index 792127d26c4..e1148418f87 100644 --- a/question/bank/usage/classes/tables/question_usage_table.php +++ b/question/bank/usage/classes/tables/question_usage_table.php @@ -113,7 +113,9 @@ class question_usage_table extends table_sql { $course = get_course($values->courseid); $context = context_course::instance($course->id); - return html_writer::link(course_get_url($course), format_string($course->fullname, true, $context)); + return html_writer::link(course_get_url($course), format_string($course->fullname, true, [ + 'context' => $context, + ])); } public function col_attempts(\stdClass $values): string {