Merge branch 'MDL-80079-402' of https://github.com/andrewnicols/moodle …

…into MOODLE_402_STABLE

course/classes/list_element.php

@@ -349,23 +349,35 @@ public function getIterator(): Traversable {
      * @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()],
+        );
     }
 
     /**
      * Returns the formatted fullname for this course.
      * @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()],
+        );
     }
 
     /**
      * Returns the formatted shortname for this course.
      * @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()],
+        );
     }
 
     /**

group/autogroup_form.php

@@ -93,7 +93,9 @@ function definition() {
         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');

lib/form/cohort.php

@@ -141,7 +141,7 @@ public function setValue($value) {
             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);
         }
 

lib/tests/weblib_format_text_test.php

@@ -25,6 +25,7 @@
  * @category  test
  * @copyright 2015 The Open University
  * @license   http://www.gnu.org/copyleft/gpl.html GNU Public License
+ * @covers ::format_text
  */
 class weblib_format_text_test extends \advanced_testcase {
 
@@ -268,4 +269,18 @@ public function format_text_cleaning_testcases() {
             ],
         ];
     }
+
+    public function test_with_context_as_options(): void {
+        $this->assertEquals(
+            '<p>Example</p>',
+            format_text('<p>Example</p>', FORMAT_HTML, \context_system::instance()),
+        );
+
+        $messages = $this->getDebuggingMessages();
+        $this->assertdebuggingcalledcount(1);
+        $this->assertStringContainsString(
+            'The options argument should not be a context object directly.',
+            $messages[0]->message,
+        );
+    }
 }

lib/tests/weblib_test.php

@@ -168,6 +168,113 @@ public function test_s() {
         }
     }
 
+    /**
+     * 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 <span lang="en" class="multilang">do not </span><span lang="de" class="multilang">do not </span>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
      */

lib/upgrade.txt

@@ -12,6 +12,8 @@ information provided here is intended especially for developers.
 * In enrollib.php, the method enrol_get_course_users() got an optional 5th parameter $usergroups that
   defaults to an empty array. Here, user group ids can be provided, to select enrolled users in a course
   that are also members of these groups.
+* The options for `format_string()`, and `format_text()` are now checked for incorrectly passed context objects.
+  Please note that this was never an accepted value but previously failed silently.
 
 === 4.2.3 ===
 * \moodle_page::set_title() has been updated to append the site name depending on the value of $CFG->sitenameintitle and whether

lib/weblib.php

@@ -1268,6 +1268,17 @@ function format_text($text, $format = FORMAT_MOODLE, $options = null, $courseidd
         return '';
     }
 
+    if ($options instanceof \core\context) {
+        // A common mistake has been to call this function with a context object.
+        // This has never been expected, 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];
+    }
+
     // Detach object, we can not modify it.
     $options = (array)$options;
 
@@ -1514,12 +1525,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'])) {

mod/assign/classes/output/renderer.php

@@ -656,7 +656,7 @@ public function render_assign_submission_status(assign_submission_status $status
             $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');

mod/assign/override_form.php

@@ -107,7 +107,7 @@ protected function definition() {
             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 @@ protected function definition() {
                 $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);

mod/lesson/override_form.php

@@ -96,7 +96,7 @@ protected function definition() {
             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 @@ protected function definition() {
                 $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);

mod/lti/classes/privacy/provider.php

@@ -398,7 +398,7 @@ protected static function export_user_data_lti_tool_proxies(approved_contextlist
         $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)

mod/quiz/classes/form/edit_override_form.php

@@ -102,7 +102,7 @@ protected function definition() {
             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);
@@ -120,7 +120,7 @@ protected function definition() {
                 $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);

question/bank/usage/classes/tables/question_usage_table.php

@@ -103,7 +103,9 @@ public function col_coursename(\stdClass $values): string {
         $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 {