From b18c503241d96128aacbcd177f7253960a8a5686 Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Mon, 12 Jun 2023 09:48:57 +1000 Subject: [PATCH 1/7] MDL-73734 output: Add streaming output api This adds a new output method which enables a page to fully render a complete html page and then keep the connection open and stream additional content into an element anywhere in the document. This enables pages which take some time to render cleanly without layout bugs and reduces layout shift issues and was inspired by Facebook BigPipe. --- lib/outputrenderers.php | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php index 59bfce5ce7d..1dfcbe3a881 100644 --- a/lib/outputrenderers.php +++ b/lib/outputrenderers.php @@ -5157,6 +5157,94 @@ EOD; } return $addblockbutton; } + + /** + * Prepares an element for streaming output + * + * This must be used with NO_OUTPUT_BUFFERING set to true. After using this method + * any subsequent prints or echos to STDOUT result in the outputted content magically + * being appended inside that element rather than where the current html would be + * normally. This enables pages which take some time to render incremental content to + * first output a fully formed html page, including the footer, and to then stream + * into an element such as the main content div. This fixes a class of page layout + * bugs and reduces layout shift issues and was inspired by Facebook BigPipe. + * + * Some use cases such as a simple page which loads content via ajax could be swapped + * to this method wich saves another http request and its network latency resulting + * in both lower server load and better front end performance. + * + * You should consider giving the element you stream into a minimum height to further + * reduce layout shift as the content initally streams into the element. + * + * You can safely finish the output without closing the streamed element. You can also + * call this method again to swap the target of the streaming to a new element as + * often as you want. + + * https://www.youtube.com/watch?v=LLRig4s1_yA&t=1022s + * Watch this video segment to explain how and why this 'One Weird Trick' works. + * + * @param string $selector where new content should be appended + * @param string $element which contains the streamed content + */ + public function select_element_for_append(string $selector = '#region-main [role=main]', string $element = 'div') { + + static $currentselector = ''; + static $currentelement = ''; + + if (!CLI_SCRIPT && !NO_OUTPUT_BUFFERING) { + throw new coding_exception('select_element_for_append used in a non-CLI script without setting NO_OUTPUT_BUFFERING.', + DEBUG_DEVELOPER); + } + + // We are already streaming into this element so don't change anything. + if ($currentselector === $selector && $currentelement === $element) { + return; + } + + $html = ''; + + // We have a streaming element so close it before starting a new one. + if ($currentselector !== '') { + $html .= html_writer::end_tag($currentelement); + } + + $currentselector = $selector; + $currentelement = $element; + + // Create an unclosed element for the streamed content to append into. + $id = uniqid(); + $html .= html_writer::start_tag($element, ['id' => $id]); + $html .= html_writer::tag('script', "document.querySelector('$selector').append(document.getElementById('$id'))"); + return $html; + } + + /** + * A companion method to select_element_for_append + * + * This must be used with NO_OUTPUT_BUFFERING set to true. + * + * This is similar but instead of appending into the element it replaces + * the content in the element. Depending on the 3rd argument it can replace + * the innerHTML or the outerHTML which can be useful to completely remove + * the element if needed. + * + * @param string $selector where new content should be replaced + * @param string $html A chunk of well formed html + * @param bool $outer Wether it replaces the innerHTML or the outerHTML + */ + public function select_element_for_replace(string $selector, string $html, bool $outer = false) { + + if (!CLI_SCRIPT && !NO_OUTPUT_BUFFERING) { + throw new coding_exception('select_element_for_replace used in a non-CLI script without setting NO_OUTPUT_BUFFERING.', + DEBUG_DEVELOPER); + } + + // Escape html for use inside a javascript string. + $html = addslashes_js($html); + $property = $outer ? 'outerHTML' : 'innerHTML'; + $output = html_writer::tag('script', "document.querySelector('$selector').$property = '$html';"); + return $output; + } } /** From 442f18a92da58eebe192232d803c6e5f8678f865 Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Mon, 12 Jun 2023 09:50:27 +1000 Subject: [PATCH 2/7] MDL-73734 tool_task: Use streaming output api --- admin/tool/task/schedule_task.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/admin/tool/task/schedule_task.php b/admin/tool/task/schedule_task.php index 538e6b5fe8a..ecaaf7efde0 100644 --- a/admin/tool/task/schedule_task.php +++ b/admin/tool/task/schedule_task.php @@ -83,13 +83,16 @@ require_sesskey(); \core\session\manager::write_close(); +// Prepare for streamed output. +echo $OUTPUT->footer(); +echo $OUTPUT->select_element_for_append(); + // Prepare to handle output via mtrace. -echo html_writer::start_tag('pre'); +echo html_writer::start_tag('pre', ['style' => 'color: #fff; background: #333; padding: 1em; min-height: 24lh']); $CFG->mtrace_wrapper = 'tool_task_mtrace_wrapper'; // Run the specified task (this will output an error if it doesn't exist). \core\task\manager::run_from_cli($task); - echo html_writer::end_tag('pre'); $output = $PAGE->get_renderer('tool_task'); @@ -100,4 +103,3 @@ echo $OUTPUT->single_button(new moodle_url('/admin/tool/task/schedule_task.php', get_string('runagain', 'tool_task')); echo $output->link_back(get_class($task)); -echo $OUTPUT->footer(); From d5860ac566b93473389b6aee837b96e9586356de Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Mon, 12 Jun 2023 09:51:48 +1000 Subject: [PATCH 3/7] MDL-73734 tool_customlang: Use streaming output api --- admin/tool/customlang/index.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/admin/tool/customlang/index.php b/admin/tool/customlang/index.php index f8530dcb94e..db2b68412e8 100644 --- a/admin/tool/customlang/index.php +++ b/admin/tool/customlang/index.php @@ -57,13 +57,17 @@ if ($action === 'checkout') { $progressbar = new progress_bar(); $progressbar->create(); // prints the HTML code of the progress bar - // we may need a bit of extra execution time and memory here + echo $output->continue_button(new moodle_url("/admin/tool/customlang/{$next}.php", array('lng' => $lng)), 'get'); + echo $output->footer(); + + \core\session\manager::write_close(); + echo $OUTPUT->select_element_for_append(); + + // We may need a bit of extra execution time and memory here. core_php_time_limit::raise(HOURSECS); raise_memory_limit(MEMORY_EXTRA); tool_customlang_utils::checkout($lng, $progressbar); - echo $output->continue_button(new moodle_url("/admin/tool/customlang/{$next}.php", array('lng' => $lng)), 'get'); - echo $output->footer(); exit; } if ($action === 'checkin') { From 9a8fccbabcb5df145ac32f3c850998e264781d42 Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Mon, 12 Jun 2023 09:49:34 +1000 Subject: [PATCH 4/7] MDL-73734 tool_httpreplace: Use streaming output api --- admin/tool/httpsreplace/classes/url_finder.php | 1 + admin/tool/httpsreplace/tool.php | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/admin/tool/httpsreplace/classes/url_finder.php b/admin/tool/httpsreplace/classes/url_finder.php index 7ac5bc3cf2b..1f7ab80eb4e 100644 --- a/admin/tool/httpsreplace/classes/url_finder.php +++ b/admin/tool/httpsreplace/classes/url_finder.php @@ -151,6 +151,7 @@ class url_finder { sort($tables); // Make it easier to see progress because they are ordered. $numberoftables = count($tables); $tablenumber = 0; + sort($tables); foreach ($tables as $table) { if ($progress) { $progress->update($tablenumber, $numberoftables, get_string('searching', 'tool_httpsreplace', $table)); diff --git a/admin/tool/httpsreplace/tool.php b/admin/tool/httpsreplace/tool.php index 6d2d96349d7..59d6b54b855 100644 --- a/admin/tool/httpsreplace/tool.php +++ b/admin/tool/httpsreplace/tool.php @@ -58,6 +58,14 @@ $finder = new \tool_httpsreplace\url_finder(); $PAGE->set_cacheable(false); $progressbar = new progress_bar(); +// Preemptively reset the navcache before closing, so it remains the same on shutdown. +navigation_cache::destroy_volatile_caches(); +\core\session\manager::write_close(); + +// Prepare for streamed output. +echo $OUTPUT->footer(); +echo $OUTPUT->select_element_for_append(); + if (!$data = $form->get_data()) { echo $progressbar->create(); @@ -103,4 +111,3 @@ if (!$data = $form->get_data()) { echo $OUTPUT->continue_button(new moodle_url('/admin/settings.php', ['section' => 'httpsecurity'])); } -echo $OUTPUT->footer(); From a6055feebf02914ec7dc98ef969aa3eca88e93bd Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Tue, 11 Jul 2023 22:32:53 +1000 Subject: [PATCH 5/7] MDL-73734 course: Use streaming output api when deleting courses --- .../tests/behat/basic_functionality.feature | 1 + course/delete.php | 11 ++++++++++- course/tests/behat/create_delete_course.feature | 2 ++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/admin/tool/recyclebin/tests/behat/basic_functionality.feature b/admin/tool/recyclebin/tests/behat/basic_functionality.feature index 11f462316d0..65d773c84df 100644 --- a/admin/tool/recyclebin/tests/behat/basic_functionality.feature +++ b/admin/tool/recyclebin/tests/behat/basic_functionality.feature @@ -58,6 +58,7 @@ Feature: Basic recycle bin functionality And I am on "Course 1" course homepage And I should see "Test assign 1" in the "Topic 1" "section" + @javascript Scenario: Restore a deleted course Given I log in as "admin" And I go to the courses management page diff --git a/course/delete.php b/course/delete.php index c4a942ffe9c..99bc99ace0c 100644 --- a/course/delete.php +++ b/course/delete.php @@ -22,6 +22,8 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +define('NO_OUTPUT_BUFFERING', true); + require_once(__DIR__ . '/../config.php'); require_once($CFG->dirroot . '/course/lib.php'); require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); @@ -64,13 +66,20 @@ if ($delete === md5($course->timemodified)) { echo $OUTPUT->heading($strdeletingcourse); // This might take a while. Raise the execution time limit. core_php_time_limit::raise(); + // We do this here because it spits out feedback as it goes. + echo $OUTPUT->footer(); + echo $OUTPUT->select_element_for_append(); + + // Preemptively reset the navcache before closing, so it remains the same on shutdown. + navigation_cache::destroy_volatile_caches(); + \core\session\manager::write_close(); + delete_course($course); echo $OUTPUT->heading( get_string("deletedcourse", "", $courseshortname) ); // Update course count in categories. fix_course_sortorder(); echo $OUTPUT->continue_button($categoryurl); - echo $OUTPUT->footer(); exit; // We must exit here!!! } diff --git a/course/tests/behat/create_delete_course.feature b/course/tests/behat/create_delete_course.feature index fde89630b3f..f3e465698d0 100644 --- a/course/tests/behat/create_delete_course.feature +++ b/course/tests/behat/create_delete_course.feature @@ -32,6 +32,7 @@ Feature: Test we can both create and delete a course. And I should see "Cat 1" in the "#category-listing" "css_element" And I should see "Test course: create a course" in the "#course-listing" "css_element" + @javascript Scenario: Delete a course via its management listing Given the following "categories" exist: | name | category 0| idnumber | @@ -64,6 +65,7 @@ Feature: Test we can both create and delete a course. And I should see "Cat 1" in the "#category-listing" "css_element" And I should see "Test course 2: create another course" in the "#course-listing" "css_element" + @javascript Scenario: Delete a course via its management details page Given the following "categories" exist: | name | category 0| idnumber | From 072a15ced37e4768d619e07f4ec564504e81b2e5 Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Mon, 12 Jun 2023 09:49:59 +1000 Subject: [PATCH 6/7] MDL-73734 tool_generator: Use streaming output api --- admin/tool/generator/maketestcourse.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/admin/tool/generator/maketestcourse.php b/admin/tool/generator/maketestcourse.php index fd20563926e..bccfa4a04d4 100644 --- a/admin/tool/generator/maketestcourse.php +++ b/admin/tool/generator/maketestcourse.php @@ -49,6 +49,15 @@ if (!debugging('', DEBUG_DEVELOPER)) { exit; } +// Finish page. +echo $OUTPUT->footer(); +echo $OUTPUT->select_element_for_append(); + +// Preemptively reset the navcache before closing, so it remains the same on shutdown. +navigation_cache::destroy_volatile_caches(); + +\core\session\manager::write_close(); + // Set up the form. $mform = new tool_generator_make_course_form('maketestcourse.php'); if ($data = $mform->get_data()) { @@ -75,5 +84,3 @@ if ($data = $mform->get_data()) { $mform->display(); } -// Finish page. -echo $OUTPUT->footer(); From 20b8f8f9c99910c527cdfc463652af10160acf23 Mon Sep 17 00:00:00 2001 From: Brendan Heywood Date: Fri, 18 Aug 2023 01:23:56 +1000 Subject: [PATCH 7/7] MDL-73734 core: Use streaming output for perfdebug --- lang/en/admin.php | 1 + lib/classes/shutdown_manager.php | 11 ++++++++++- lib/outputrenderers.php | 15 +++++++++++++-- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lang/en/admin.php b/lang/en/admin.php index cfacbfeaea0..50a211eae3f 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -1020,6 +1020,7 @@ $string['pathtosassc'] = 'Path to SassC'; $string['pathtosassc_help'] = 'Specifying the location of the SassC binary will switch the SASS compiler from Moodle\'s PHP implementation to SassC. See https://github.com/sass/sassc for more information.'; $string['pcreunicodewarning'] = 'It is strongly recommended to use PCRE PHP extension that is compatible with Unicode characters.'; $string['perfdebug'] = 'Performance info'; +$string['perfdebugdeferred'] = 'Waiting until the script ends to show the performance debugging ...'; $string['performance'] = 'Performance'; $string['pgcluster'] = 'PostgreSQL Cluster'; $string['pgclusterdescription'] = 'PostgreSQL version/cluster parameter for command line operations. If you only have one postgresql on your system or you are not sure what this is, leave this blank.'; diff --git a/lib/classes/shutdown_manager.php b/lib/classes/shutdown_manager.php index c610f637701..bc8b5cd3f99 100644 --- a/lib/classes/shutdown_manager.php +++ b/lib/classes/shutdown_manager.php @@ -195,7 +195,7 @@ class core_shutdown_manager { * Standard shutdown sequence. */ protected static function request_shutdown() { - global $CFG; + global $CFG, $OUTPUT; // Help apache server if possible. $apachereleasemem = false; @@ -216,6 +216,15 @@ class core_shutdown_manager { $perf = get_performance_info(); error_log("PERF: " . $perf['txt']); } + if (MDL_PERFTOFOOT || debugging() || (!empty($CFG->perfdebug) && $CFG->perfdebug > 7)) { + if (NO_OUTPUT_BUFFERING) { + // If the performance footer was deferred then print it now. + if (!CLI_SCRIPT && !WS_SERVER) { + $perf = get_performance_info(); + echo $OUTPUT->select_element_for_replace('#perfdebugfooter', $perf['html']); + } + } + } if (MDL_PERFINC) { $inc = get_included_files(); $ts = 0; diff --git a/lib/outputrenderers.php b/lib/outputrenderers.php index 1dfcbe3a881..05bbbf757f5 100644 --- a/lib/outputrenderers.php +++ b/lib/outputrenderers.php @@ -1512,9 +1512,20 @@ class core_renderer extends renderer_base { // Provide some performance info if required $performanceinfo = ''; if (MDL_PERF || (!empty($CFG->perfdebug) && $CFG->perfdebug > 7)) { - $perf = get_performance_info(); if (MDL_PERFTOFOOT || debugging() || (!empty($CFG->perfdebug) && $CFG->perfdebug > 7)) { - $performanceinfo = $perf['html']; + if (NO_OUTPUT_BUFFERING) { + // If the output buffer was off then we render a placeholder and stream the + // performance debugging into it at the very end in the shutdown handler. + $performanceinfo .= html_writer::tag('div', + get_string('perfdebugdeferred', 'admin'), + [ + 'id' => 'perfdebugfooter', + 'style' => 'min-height: 30em', + ]); + } else { + $perf = get_performance_info(); + $performanceinfo = $perf['html']; + } } }