Merge branch 'MDL-68645-master-earlyoutputinit' of git://github.com/mudrd8mz/moodle
authorEloy Lafuente (stronk7) <stronk7@moodle.org>
Thu, 14 May 2020 14:35:35 +0000 (16:35 +0200)
committerEloy Lafuente (stronk7) <stronk7@moodle.org>
Thu, 14 May 2020 14:35:35 +0000 (16:35 +0200)
16 files changed:
completion/classes/api.php
lib/pagelib.php
lib/testing/generator/block_generator.php
lib/testing/generator/module_generator.php
lib/testing/generator/repository_generator.php
mod/assign/lib.php
mod/chat/lib.php
mod/choice/locallib.php
mod/data/locallib.php
mod/feedback/lib.php
mod/forum/locallib.php
mod/lesson/lib.php
mod/quiz/lib.php
mod/scorm/locallib.php
mod/upgrade.txt
mod/workshop/lib.php

index b569e37..f55aa90 100644 (file)
@@ -87,7 +87,8 @@ class api {
             if ($completionexpectedtime !== null) {
                 // Calendar event exists so update it.
                 $event->name = get_string('completionexpectedfor', 'completion', $lang);
-                $event->description = format_module_intro($modulename, $instance, $cmid);
+                $event->description = format_module_intro($modulename, $instance, $cmid, false);
+                $event->format = FORMAT_HTML;
                 $event->timestart = $completionexpectedtime;
                 $event->timesort = $completionexpectedtime;
                 $event->visible = instance_is_visible($modulename, $instance);
@@ -104,7 +105,8 @@ class api {
             // Event doesn't exist so create one.
             if ($completionexpectedtime !== null) {
                 $event->name = get_string('completionexpectedfor', 'completion', $lang);
-                $event->description = format_module_intro($modulename, $instance, $cmid);
+                $event->description = format_module_intro($modulename, $instance, $cmid, false);
+                $event->format = FORMAT_HTML;
                 $event->courseid = $instance->course;
                 $event->groupid = 0;
                 $event->userid = 0;
index 7e17307..99366fc 100644 (file)
@@ -1572,6 +1572,15 @@ class moodle_page {
         $this->_wherethemewasinitialised = debug_backtrace();
     }
 
+    /**
+     * For diagnostic/debugging purposes, find where the theme setup was triggered.
+     *
+     * @return null|array null if theme not yet setup. Stacktrace if it was.
+     */
+    public function get_where_theme_was_initialised() {
+        return $this->_wherethemewasinitialised;
+    }
+
     /**
      * Reset the theme and output for a new context. This only makes sense from
      * external::validate_context(). Do not cheat.
index ce5da1f..3c4d9a4 100644 (file)
@@ -114,10 +114,14 @@ abstract class testing_block_generator extends component_generator_base {
      * @return stdClass the block_instance record that has just been created.
      */
     public function create_instance($record = null, $options = array()) {
-        global $DB;
+        global $DB, $PAGE;
 
         $this->instancecount++;
 
+        // Creating a block is a back end operation, which should not cause any output to happen.
+        // This will allow us to check that the theme was not initialised while creating the block instance.
+        $outputstartedbefore = $PAGE->get_where_theme_was_initialised();
+
         $record = (object)(array)$record;
         $this->preprocess_record($record, $options);
         $record = $this->prepare_record($record);
@@ -133,6 +137,19 @@ abstract class testing_block_generator extends component_generator_base {
         context_block::instance($id);
 
         $instance = $DB->get_record('block_instances', array('id' => $id), '*', MUST_EXIST);
+
+        // If the theme was initialised while creating the block instance, something somewhere called an output
+        // function. Rather than leaving this as a hard-to-debug situation, let's make it fail with a clear error.
+        $outputstartedafter = $PAGE->get_where_theme_was_initialised();
+
+        if ($outputstartedbefore === null && $outputstartedafter !== null) {
+            throw new coding_exception('Creating a block_' . $this->get_blockname() . ' initialised the theme and output!',
+                'This should not happen. Creating a block should be a pure back-end operation. Unnecessarily initialising ' .
+                'the output mechanism at the wrong time can cause subtle bugs and is a significant performance hit. There is ' .
+                'likely a call to an output function that caused it:' . PHP_EOL . PHP_EOL .
+                format_backtrace($outputstartedafter, true));
+        }
+
         return $instance;
     }
 
index 5400648..4a2396e 100644 (file)
@@ -222,11 +222,15 @@ abstract class testing_module_generator extends component_generator_base {
      *     cmid (corresponding id in course_modules table)
      */
     public function create_instance($record = null, array $options = null) {
-        global $CFG, $DB;
+        global $CFG, $DB, $PAGE;
         require_once($CFG->dirroot.'/course/modlib.php');
 
         $this->instancecount++;
 
+        // Creating an activity is a back end operation, which should not cause any output to happen.
+        // This will allow us to check that the theme was not initialised while creating the module instance.
+        $outputstartedbefore = $PAGE->get_where_theme_was_initialised();
+
         // Merge options into record and add default values.
         $record = $this->prepare_moduleinfo_record($record, $options);
 
@@ -269,6 +273,19 @@ abstract class testing_module_generator extends component_generator_base {
         // Prepare object to return with additional field cmid.
         $instance = $DB->get_record($this->get_modulename(), array('id' => $moduleinfo->instance), '*', MUST_EXIST);
         $instance->cmid = $moduleinfo->coursemodule;
+
+        // If the theme was initialised while creating the module instance, something somewhere called an output
+        // function. Rather than leaving this as a hard-to-debug situation, let's make it fail with a clear error.
+        $outputstartedafter = $PAGE->get_where_theme_was_initialised();
+
+        if ($outputstartedbefore === null && $outputstartedafter !== null) {
+            throw new coding_exception('Creating a mod_' . $this->get_modulename() . ' activity initialised the theme and output!',
+                'This should not happen. Creating an activity should be a pure back-end operation. Unnecessarily initialising ' .
+                'the output mechanism at the wrong time can cause subtle bugs and is a significant performance hit. There is ' .
+                'likely a call to an output function that caused it:' . PHP_EOL . PHP_EOL .
+                format_backtrace($outputstartedafter, true));
+        }
+
         return $instance;
     }
 
index 3d5f560..21a4066 100644 (file)
@@ -111,12 +111,16 @@ class testing_repository_generator extends component_generator_base {
      * @return stdClass repository instance record
      */
     public function create_instance($record = null, array $options = null) {
-        global $CFG, $DB;
+        global $CFG, $DB, $PAGE;
         require_once($CFG->dirroot . '/repository/lib.php');
 
         $this->instancecount++;
         $record = (array) $record;
 
+        // Creating a repository is a back end operation, which should not cause any output to happen.
+        // This will allow us to check that the theme was not initialised while creating the repository instance.
+        $outputstartedbefore = $PAGE->get_where_theme_was_initialised();
+
         $typeid = $DB->get_field('repository', 'id', array('type' => $this->get_typename()), MUST_EXIST);
         $instanceoptions = repository::static_function($this->get_typename(), 'get_instance_option_names');
 
@@ -146,6 +150,18 @@ class testing_repository_generator extends component_generator_base {
             $id = repository::static_function($this->get_typename(), 'create', $this->get_typename(), 0, $context, $record);
         }
 
+        // If the theme was initialised while creating the repository instance, something somewhere called an output
+        // function. Rather than leaving this as a hard-to-debug situation, let's make it fail with a clear error.
+        $outputstartedafter = $PAGE->get_where_theme_was_initialised();
+
+        if ($outputstartedbefore === null && $outputstartedafter !== null) {
+            throw new coding_exception('Creating a repository_' . $this->get_typename() . ' initialised the theme and output!',
+                'This should not happen. Creating a repository should be a pure back-end operation. Unnecessarily initialising ' .
+                'the output mechanism at the wrong time can cause subtle bugs and is a significant performance hit. There is ' .
+                'likely a call to an output function that caused it:' . PHP_EOL . PHP_EOL .
+                format_backtrace($outputstartedafter, true));
+        }
+
         return $DB->get_record('repository_instances', array('id' => $id), '*', MUST_EXIST);
     }
 
index 5f65f2e..d9ded66 100644 (file)
@@ -297,7 +297,8 @@ function assign_update_events($assign, $override = null) {
 
         $event = new stdClass();
         $event->type = CALENDAR_EVENT_TYPE_ACTION;
-        $event->description = format_module_intro('assign', $assigninstance, $cmid);
+        $event->description = format_module_intro('assign', $assigninstance, $cmid, false);
+        $event->format = FORMAT_HTML;
         // Events module won't show user events when the courseid is nonzero.
         $event->courseid    = ($userid) ? 0 : $assigninstance->course;
         $event->groupid     = $groupid;
index 503023c..478fe8b 100644 (file)
@@ -123,7 +123,8 @@ function chat_add_instance($chat) {
         $event = new stdClass();
         $event->type        = CALENDAR_EVENT_TYPE_ACTION;
         $event->name        = $chat->name;
-        $event->description = format_module_intro('chat', $chat, $chat->coursemodule);
+        $event->description = format_module_intro('chat', $chat, $chat->coursemodule, false);
+        $event->format      = FORMAT_HTML;
         $event->courseid    = $chat->course;
         $event->groupid     = 0;
         $event->userid      = 0;
@@ -169,7 +170,8 @@ function chat_update_instance($chat) {
         if ($chat->schedule > 0) {
             $event->type        = CALENDAR_EVENT_TYPE_ACTION;
             $event->name        = $chat->name;
-            $event->description = format_module_intro('chat', $chat, $chat->coursemodule);
+            $event->description = format_module_intro('chat', $chat, $chat->coursemodule, false);
+            $event->format      = FORMAT_HTML;
             $event->timestart   = $chat->chattime;
             $event->timesort    = $chat->chattime;
 
@@ -186,7 +188,8 @@ function chat_update_instance($chat) {
             $event = new stdClass();
             $event->type        = CALENDAR_EVENT_TYPE_ACTION;
             $event->name        = $chat->name;
-            $event->description = format_module_intro('chat', $chat, $chat->coursemodule);
+            $event->description = format_module_intro('chat', $chat, $chat->coursemodule, false);
+            $event->format      = FORMAT_HTML;
             $event->courseid    = $chat->course;
             $event->groupid     = 0;
             $event->userid      = 0;
@@ -460,7 +463,8 @@ function chat_prepare_update_events($chat, $cm = null) {
     $event = new stdClass();
     $event->name        = $chat->name;
     $event->type        = CALENDAR_EVENT_TYPE_ACTION;
-    $event->description = format_module_intro('chat', $chat, $cm->id);
+    $event->description = format_module_intro('chat', $chat, $cm->id, false);
+    $event->format      = FORMAT_HTML;
     $event->timestart   = $chat->chattime;
     $event->timesort    = $chat->chattime;
     if ($event->id = $DB->get_field('event', 'id', array('modulename' => 'chat', 'instance' => $chat->id,
index b5cb29b..a00641d 100644 (file)
@@ -52,7 +52,8 @@ function choice_set_events($choice) {
         if ((!empty($choice->timeopen)) && ($choice->timeopen > 0)) {
             // Calendar event exists so update it.
             $event->name         = get_string('calendarstart', 'choice', $choice->name);
-            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule);
+            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->timestart    = $choice->timeopen;
             $event->timesort     = $choice->timeopen;
             $event->visible      = instance_is_visible('choice', $choice);
@@ -68,7 +69,8 @@ function choice_set_events($choice) {
         // Event doesn't exist so create one.
         if ((!empty($choice->timeopen)) && ($choice->timeopen > 0)) {
             $event->name         = get_string('calendarstart', 'choice', $choice->name);
-            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule);
+            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->courseid     = $choice->course;
             $event->groupid      = 0;
             $event->userid       = 0;
@@ -91,7 +93,8 @@ function choice_set_events($choice) {
         if ((!empty($choice->timeclose)) && ($choice->timeclose > 0)) {
             // Calendar event exists so update it.
             $event->name         = get_string('calendarend', 'choice', $choice->name);
-            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule);
+            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->timestart    = $choice->timeclose;
             $event->timesort     = $choice->timeclose;
             $event->visible      = instance_is_visible('choice', $choice);
@@ -107,7 +110,8 @@ function choice_set_events($choice) {
         // Event doesn't exist so create one.
         if ((!empty($choice->timeclose)) && ($choice->timeclose > 0)) {
             $event->name         = get_string('calendarend', 'choice', $choice->name);
-            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule);
+            $event->description  = format_module_intro('choice', $choice, $choice->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->courseid     = $choice->course;
             $event->groupid      = 0;
             $event->userid       = 0;
index b8bad67..c915408 100644 (file)
@@ -607,7 +607,8 @@ function data_set_events($data) {
         if ($data->timeavailablefrom > 0) {
             // Calendar event exists so update it.
             $event->name         = get_string('calendarstart', 'data', $data->name);
-            $event->description  = format_module_intro('data', $data, $data->coursemodule);
+            $event->description  = format_module_intro('data', $data, $data->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->timestart    = $data->timeavailablefrom;
             $event->timesort     = $data->timeavailablefrom;
             $event->visible      = instance_is_visible('data', $data);
@@ -623,7 +624,8 @@ function data_set_events($data) {
         // Event doesn't exist so create one.
         if (isset($data->timeavailablefrom) && $data->timeavailablefrom > 0) {
             $event->name         = get_string('calendarstart', 'data', $data->name);
-            $event->description  = format_module_intro('data', $data, $data->coursemodule);
+            $event->description  = format_module_intro('data', $data, $data->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->courseid     = $data->course;
             $event->groupid      = 0;
             $event->userid       = 0;
@@ -646,7 +648,8 @@ function data_set_events($data) {
         if ($data->timeavailableto > 0) {
             // Calendar event exists so update it.
             $event->name         = get_string('calendarend', 'data', $data->name);
-            $event->description  = format_module_intro('data', $data, $data->coursemodule);
+            $event->description  = format_module_intro('data', $data, $data->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->timestart    = $data->timeavailableto;
             $event->timesort     = $data->timeavailableto;
             $event->visible      = instance_is_visible('data', $data);
@@ -662,7 +665,8 @@ function data_set_events($data) {
         // Event doesn't exist so create one.
         if (isset($data->timeavailableto) && $data->timeavailableto > 0) {
             $event->name         = get_string('calendarend', 'data', $data->name);
-            $event->description  = format_module_intro('data', $data, $data->coursemodule);
+            $event->description  = format_module_intro('data', $data, $data->coursemodule, false);
+            $event->format       = FORMAT_HTML;
             $event->courseid     = $data->course;
             $event->groupid      = 0;
             $event->userid       = 0;
index a318641..458cb73 100644 (file)
@@ -809,7 +809,8 @@ function feedback_set_events($feedback) {
         $event->eventtype    = FEEDBACK_EVENT_TYPE_OPEN;
         $event->type         = empty($feedback->timeclose) ? CALENDAR_EVENT_TYPE_ACTION : CALENDAR_EVENT_TYPE_STANDARD;
         $event->name         = get_string('calendarstart', 'feedback', $feedback->name);
-        $event->description  = format_module_intro('feedback', $feedback, $feedback->coursemodule);
+        $event->description  = format_module_intro('feedback', $feedback, $feedback->coursemodule, false);
+        $event->format       = FORMAT_HTML;
         $event->timestart    = $feedback->timeopen;
         $event->timesort     = $feedback->timeopen;
         $event->visible      = instance_is_visible('feedback', $feedback);
@@ -844,7 +845,8 @@ function feedback_set_events($feedback) {
         $event->type         = CALENDAR_EVENT_TYPE_ACTION;
         $event->eventtype    = FEEDBACK_EVENT_TYPE_CLOSE;
         $event->name         = get_string('calendarend', 'feedback', $feedback->name);
-        $event->description  = format_module_intro('feedback', $feedback, $feedback->coursemodule);
+        $event->description  = format_module_intro('feedback', $feedback, $feedback->coursemodule, false);
+        $event->format       = FORMAT_HTML;
         $event->timestart    = $feedback->timeclose;
         $event->timesort     = $feedback->timeclose;
         $event->visible      = instance_is_visible('feedback', $feedback);
index ab333a9..d13042d 100644 (file)
@@ -723,7 +723,8 @@ function forum_update_calendar($forum, $cmid) {
 
     if (!empty($forum->duedate)) {
         $event->name = get_string('calendardue', 'forum', $forum->name);
-        $event->description = format_module_intro('forum', $forum, $cmid);
+        $event->description = format_module_intro('forum', $forum, $cmid, false);
+        $event->format = FORMAT_HTML;
         $event->courseid = $forum->course;
         $event->modulename = 'forum';
         $event->instance = $forum->id;
index 1758dc3..45444fc 100644 (file)
@@ -162,7 +162,8 @@ function lesson_update_events($lesson, $override = null) {
 
         $event = new stdClass();
         $event->type = !$deadline ? CALENDAR_EVENT_TYPE_ACTION : CALENDAR_EVENT_TYPE_STANDARD;
-        $event->description = format_module_intro('lesson', $lesson, $cmid);
+        $event->description = format_module_intro('lesson', $lesson, $cmid, false);
+        $event->format = FORMAT_HTML;
         // Events module won't show user events when the courseid is nonzero.
         $event->courseid    = ($userid) ? 0 : $lesson->course;
         $event->groupid     = $groupid;
index 20fe537..7330c3e 100644 (file)
@@ -1260,7 +1260,8 @@ function quiz_update_events($quiz, $override = null) {
 
         $event = new stdClass();
         $event->type = !$timeclose ? CALENDAR_EVENT_TYPE_ACTION : CALENDAR_EVENT_TYPE_STANDARD;
-        $event->description = format_module_intro('quiz', $quiz, $cmid);
+        $event->description = format_module_intro('quiz', $quiz, $cmid, false);
+        $event->format = FORMAT_HTML;
         // Events module won't show user events when the courseid is nonzero.
         $event->courseid    = ($userid) ? 0 : $quiz->course;
         $event->groupid     = $groupid;
index f2dd536..6647a2b 100644 (file)
@@ -2424,7 +2424,8 @@ function scorm_update_calendar(stdClass $scorm, $cmid) {
         if ((!empty($scorm->timeopen)) && ($scorm->timeopen > 0)) {
             // Calendar event exists so update it.
             $event->name = get_string('calendarstart', 'scorm', $scorm->name);
-            $event->description = format_module_intro('scorm', $scorm, $cmid);
+            $event->description = format_module_intro('scorm', $scorm, $cmid, false);
+            $event->format = FORMAT_HTML;
             $event->timestart = $scorm->timeopen;
             $event->timesort = $scorm->timeopen;
             $event->visible = instance_is_visible('scorm', $scorm);
@@ -2441,7 +2442,8 @@ function scorm_update_calendar(stdClass $scorm, $cmid) {
         // Event doesn't exist so create one.
         if ((!empty($scorm->timeopen)) && ($scorm->timeopen > 0)) {
             $event->name = get_string('calendarstart', 'scorm', $scorm->name);
-            $event->description = format_module_intro('scorm', $scorm, $cmid);
+            $event->description = format_module_intro('scorm', $scorm, $cmid, false);
+            $event->format = FORMAT_HTML;
             $event->courseid = $scorm->course;
             $event->groupid = 0;
             $event->userid = 0;
@@ -2465,7 +2467,8 @@ function scorm_update_calendar(stdClass $scorm, $cmid) {
         if ((!empty($scorm->timeclose)) && ($scorm->timeclose > 0)) {
             // Calendar event exists so update it.
             $event->name = get_string('calendarend', 'scorm', $scorm->name);
-            $event->description = format_module_intro('scorm', $scorm, $cmid);
+            $event->description = format_module_intro('scorm', $scorm, $cmid, false);
+            $event->format = FORMAT_HTML;
             $event->timestart = $scorm->timeclose;
             $event->timesort = $scorm->timeclose;
             $event->visible = instance_is_visible('scorm', $scorm);
@@ -2482,7 +2485,8 @@ function scorm_update_calendar(stdClass $scorm, $cmid) {
         // Event doesn't exist so create one.
         if ((!empty($scorm->timeclose)) && ($scorm->timeclose > 0)) {
             $event->name = get_string('calendarend', 'scorm', $scorm->name);
-            $event->description = format_module_intro('scorm', $scorm, $cmid);
+            $event->description = format_module_intro('scorm', $scorm, $cmid, false);
+            $event->format = FORMAT_HTML;
             $event->courseid = $scorm->course;
             $event->groupid = 0;
             $event->userid = 0;
index 3c25f25..998bce0 100644 (file)
@@ -5,6 +5,27 @@ information provided here is intended especially for developers.
 
 * The callback get_shortcuts() is now deprecated. Please use get_course_content_items and get_all_content_items instead.
   See source code examples in get_course_content_items() and get_all_content_items() in mod/lti/lib.php for details.
+* When creating the calendar events and setting the event description to match the module intro description, the filters
+  must not be applied on the passed description text. Doing so leads to loosing some expected text filters features and
+  causes unnecessarily early theme and output initialisation in unit tests. If your activity creates calendar events,
+  you probably have code like:
+    ```
+    $event->description = format_module_intro('quiz', $quiz, $cmid);
+    ```
+  You need to change it to:
+    ```
+    $event->description = format_module_intro('quiz', $quiz, $cmid, false);
+    $event->format = FORMAT_HTML;
+    ```
+  Even this is still technically wrong. Content should normally only be formatted just before it is output. Ideally, we
+  should pass the raw description text, format and have a way to copy the embedded files; or provide another way for the
+  calendar to call the right format_text() later. The calendar API does not allow us to do these things easily at the
+  moment. Therefore, this compromise approach is used. The false parameter added ensures that text filters are not run
+  at this time which is important. And the format must be set to HTML, because otherwise it would use the current user's
+  preferred editor default format.
+* Related to the above and to help with detecting the problematic places in contributed 3rd party modules, the
+  testing_module_generator::create_instance() now throws coding_exception if creating a module instance initialised the
+  theme and output as a side effect.
 
 === 3.8 ===
 
index c38199a..951b631 100644 (file)
@@ -1668,6 +1668,7 @@ function workshop_calendar_update(stdClass $workshop, $cmid) {
     // the common properties for all events
     $base = new stdClass();
     $base->description  = format_module_intro('workshop', $workshop, $cmid, false);
+    $base->format       = FORMAT_HTML;
     $base->courseid     = $workshop->course;
     $base->groupid      = 0;
     $base->userid       = 0;