From 2b49ad42f556e1cb853b2aa7a6c7eef7fd92ffb2 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Sun, 7 Apr 2024 22:33:30 +0800 Subject: [PATCH 1/7] MDL-81456 core: Use DI for all hook access Using DI for all hook access means that it becomes significantly easier to mock hooks and callbacks for unit testing without fundamentally altering the structure of the code purely for the purposes of unit testing. --- admin/classes/table/hook_list_table.php | 2 +- admin/hooks.php | 2 +- admin/tool/mfa/classes/manager.php | 2 +- admin/user/user_bulk_forms.php | 2 +- .../util/helper/backup_cron_helper.class.php | 2 +- course/edit_form.php | 11 ++++++--- course/format/classes/local/cmactions.php | 5 ++-- course/lib.php | 21 ++++++++++------ course/modlib.php | 15 ++++++++---- lib/classes/navigation/views/primary.php | 2 +- lib/moodlelib.php | 24 ++++++++++++------- lib/phpunit/bootstrap.php | 1 + lib/phpunit/classes/advanced_testcase.php | 9 +++++-- lib/phpunit/classes/util.php | 7 +++++- mod/quiz/classes/grade_calculator.php | 6 ++++- mod/quiz/classes/quiz_attempt.php | 12 ++++++---- mod/quiz/locallib.php | 8 +++++-- 17 files changed, 89 insertions(+), 42 deletions(-) diff --git a/admin/classes/table/hook_list_table.php b/admin/classes/table/hook_list_table.php index 1ffc092708b..7f14bb73a83 100644 --- a/admin/classes/table/hook_list_table.php +++ b/admin/classes/table/hook_list_table.php @@ -91,7 +91,7 @@ class hook_list_table extends flexible_table { */ public function out(): void { // All hook consumers referenced from the db/hooks.php files. - $hookmanager = \core\hook\manager::get_instance(); + $hookmanager = \core\di::get(\core\hook\manager::class); $allhooks = (array)$hookmanager->get_all_callbacks(); // Add any unused hooks. diff --git a/admin/hooks.php b/admin/hooks.php index e132b65525d..9cca9da9570 100644 --- a/admin/hooks.php +++ b/admin/hooks.php @@ -30,7 +30,7 @@ require_once($CFG->libdir . '/tablelib.php'); admin_externalpage_setup('hooksoverview'); require_capability('moodle/site:config', \core\context\system::instance()); -$hookmanager = \core\hook\manager::get_instance(); +$hookmanager = \core\di::get(\core\hook\manager::class); echo $OUTPUT->header(); echo $OUTPUT->heading(get_string('hooksoverview', 'core_admin')); diff --git a/admin/tool/mfa/classes/manager.php b/admin/tool/mfa/classes/manager.php index c9a0454dda5..d8d093c44f1 100644 --- a/admin/tool/mfa/classes/manager.php +++ b/admin/tool/mfa/classes/manager.php @@ -334,7 +334,7 @@ class manager { // Allow plugins to callback as soon possible after user has passed MFA. $hook = new \tool_mfa\hook\after_user_passed_mfa(); - \core\hook\manager::get_instance()->dispatch($hook); + \core\di::get(\core\hook\manager::class)->dispatch($hook); // Add/update record in DB for users last mfa auth. self::update_pass_time(); diff --git a/admin/user/user_bulk_forms.php b/admin/user/user_bulk_forms.php index 0b36c941d15..1a1604e225a 100644 --- a/admin/user/user_bulk_forms.php +++ b/admin/user/user_bulk_forms.php @@ -130,7 +130,7 @@ class user_bulk_action_form extends moodleform { } // Any plugin can append user bulk actions to this list by implementing a hook callback. - \core\hook\manager::get_instance()->dispatch($hook); + \core\di::get(\core\hook\manager::class)->dispatch($hook); // This method may be called from 'Bulk actions' and 'Browse user list' pages. Some actions // may be irrelevant in one of the contexts and they can be excluded by specifying the diff --git a/backup/util/helper/backup_cron_helper.class.php b/backup/util/helper/backup_cron_helper.class.php index 2674f5d00c6..ecd1617fa7e 100644 --- a/backup/util/helper/backup_cron_helper.class.php +++ b/backup/util/helper/backup_cron_helper.class.php @@ -797,7 +797,7 @@ abstract class backup_cron_automated_helper { // Exclude events defined by hook. $hook = new \core_backup\hook\before_course_modified_check(); - \core\hook\manager::get_instance()->dispatch($hook); + \core\di::get(\core\hook\manager::class)->dispatch($hook); foreach ($readers as $readerpluginname => $reader) { $params = [ diff --git a/course/edit_form.php b/course/edit_form.php index 5a60a5604f4..2e60b3a9ab9 100644 --- a/course/edit_form.php +++ b/course/edit_form.php @@ -1,5 +1,10 @@ libdir.'/formslib.php'); @@ -419,7 +424,7 @@ class course_edit_form extends moodleform { $handler->instance_form_definition($mform, empty($course->id) ? 0 : $course->id); $hook = new \core_course\hook\after_form_definition($this, $mform); - \core\hook\manager::get_instance()->dispatch($hook); + di::get(hook\manager::class)->dispatch($hook); // When two elements we need a group. $buttonarray = array(); @@ -502,7 +507,7 @@ class course_edit_form extends moodleform { $handler->instance_form_definition_after_data($mform, empty($courseid) ? 0 : $courseid); $hook = new \core_course\hook\after_form_definition_after_data($this, $mform); - \core\hook\manager::get_instance()->dispatch($hook); + di::get(hook\manager::class)->dispatch($hook); } /** @@ -550,7 +555,7 @@ class course_edit_form extends moodleform { $errors = array_merge($errors, $handler->instance_form_validation($data, $files)); $hook = new \core_course\hook\after_form_validation($this, $data, $files); - \core\hook\manager::get_instance()->dispatch($hook); + di::get(hook\manager::class)->dispatch($hook); $pluginerrors = $hook->get_errors(); if (!empty($pluginerrors)) { $errors = array_merge($errors, $pluginerrors); diff --git a/course/format/classes/local/cmactions.php b/course/format/classes/local/cmactions.php index 347b2a31a5e..3e9f1e54c69 100644 --- a/course/format/classes/local/cmactions.php +++ b/course/format/classes/local/cmactions.php @@ -72,8 +72,9 @@ class cmactions extends baseactions { // Modules may add some logic to renaming. $modinfo = get_fast_modinfo($cm->course); - $hook = new \core_courseformat\hook\after_cm_name_edited($modinfo->get_cm($cm->id), $name); - \core\hook\manager::get_instance()->dispatch($hook); + \core\di::get(\core\hook\manager::class)->dispatch( + new \core_courseformat\hook\after_cm_name_edited($modinfo->get_cm($cm->id), $name), + ); // Attempt to update the grade item if relevant. $grademodule = $DB->get_record($cm->modname, ['id' => $cm->instance]); diff --git a/course/lib.php b/course/lib.php index 0d43762d618..a4a5a4da1ba 100644 --- a/course/lib.php +++ b/course/lib.php @@ -24,6 +24,10 @@ defined('MOODLE_INTERNAL') || die; +use core\{ + di, + hook, +}; use core_course\external\course_summary_exporter; use core_courseformat\base as course_format; use core_courseformat\formatactions; @@ -2032,10 +2036,11 @@ function create_course($data, $editoroptions = NULL) { $data->id = $newcourseid; // Dispatch the hook for post course create actions. - $hook = new \core_course\hook\after_course_created( - course: $data, + di::get(hook\manager::class)->dispatch( + new \core_course\hook\after_course_created( + course: $data, + ), ); - \core\di::get(\core\hook\manager::class)->dispatch($hook); // Setup the blocks blocks_add_default_course_blocks($course); @@ -2068,8 +2073,9 @@ function create_course($data, $editoroptions = NULL) { $data->id = $course->id; $handler->instance_form_save($data, true); - $hook = new \core_course\hook\after_form_submission($data, true); - \core\hook\manager::get_instance()->dispatch($hook); + di::get(hook\manager::class)->dispatch( + new \core_course\hook\after_form_submission($data, true), + ); return $course; } @@ -2186,8 +2192,9 @@ function update_course($data, $editoroptions = NULL) { $handler = core_course\customfield\course_handler::create(); $handler->instance_form_save($data); - $hook = new \core_course\hook\after_form_submission($data); - \core\hook\manager::get_instance()->dispatch($hook); + di::get(hook\manager::class)->dispatch( + new \core_course\hook\after_form_submission($data), + ); // Update with the new data $DB->update_record('course', $data); diff --git a/course/modlib.php b/course/modlib.php index 8b99058b60e..9ec24d70cb0 100644 --- a/course/modlib.php +++ b/course/modlib.php @@ -27,8 +27,12 @@ defined('MOODLE_INTERNAL') || die; -use \core_grades\component_gradeitems; +use core\{ + di, + hook, +}; use core_courseformat\formatactions; +use core_grades\component_gradeitems; require_once($CFG->dirroot.'/course/lib.php'); @@ -725,11 +729,12 @@ function update_moduleinfo($cm, $moduleinfo, $course, $mform = null) { } if ($cm->name != $moduleinfo->name) { - $hook = new \core_courseformat\hook\after_cm_name_edited( - get_fast_modinfo($course)->get_cm($cm->id), - $moduleinfo->name + di::get(hook\manager::class)->dispatch( + new \core_courseformat\hook\after_cm_name_edited( + get_fast_modinfo($course)->get_cm($cm->id), + $moduleinfo->name + ), ); - \core\hook\manager::get_instance()->dispatch($hook); } $cm->name = $moduleinfo->name; diff --git a/lib/classes/navigation/views/primary.php b/lib/classes/navigation/views/primary.php index c8d9402030f..f02bf3ebf34 100644 --- a/lib/classes/navigation/views/primary.php +++ b/lib/classes/navigation/views/primary.php @@ -82,7 +82,7 @@ class primary extends view { // Allow plugins to add nodes to the primary navigation. $hook = new \core\hook\navigation\primary_extend($this); - \core\hook\manager::get_instance()->dispatch($hook); + \core\di::get(\core\hook\manager::class)->dispatch($hook); // Search and set the active node. $this->set_active_node(); diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 22b56caf6f7..2bd1749730a 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -28,6 +28,11 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core\{ + di, + hook, +}; + defined('MOODLE_INTERNAL') || die(); // CONSTANTS (Encased in phpdoc proper comments). @@ -3581,7 +3586,7 @@ function delete_user(stdClass $user) { $hook = new \core_user\hook\before_user_deleted( user: $user, ); - \core\di::get(\core\hook\manager::class)->dispatch($hook); + di::get(hook\manager::class)->dispatch($hook); // Keep user record before updating it, as we have to pass this to user_deleted event. $olduser = clone $user; @@ -4085,8 +4090,7 @@ function complete_user_login($user, array $extrauserinfo = []) { $event->trigger(); // Allow plugins to callback as soon possible after user has completed login. - $hook = new \core\hook\user\after_complete_login(); - \core\hook\manager::get_instance()->dispatch($hook); + di::get(\core\hook\manager::class)->dispatch(new \core\hook\user\after_complete_login()); // Check if the user is using a new browser or session (a new MoodleSession cookie is set in that case). // If the user is accessing from the same IP, ignore everything (most of the time will be a new session in the same browser). @@ -7436,8 +7440,8 @@ function get_plugins_with_function($function, $file = 'lib.php', $include = true foreach ($pluginfunctions as $plugintype => $plugins) { foreach ($plugins as $plugin => $unusedfunction) { $component = $plugintype . '_' . $plugin; - if ($hooks = \core\hook\manager::get_instance()->get_hooks_deprecating_plugin_callback($plugincallback)) { - if (\core\hook\manager::get_instance()->is_deprecating_hook_present($component, $plugincallback)) { + if ($hooks = di::get(hook\manager::class)->get_hooks_deprecating_plugin_callback($plugincallback)) { + if (di::get(hook\manager::class)->is_deprecating_hook_present($component, $plugincallback)) { // Ignore the old callback, it is there only for older Moodle versions. unset($pluginfunctions[$plugintype][$plugin]); } else { @@ -7675,8 +7679,9 @@ function component_callback($component, $function, array $params = array(), $def if ($functionname) { if ($migratedtohook) { - if ($hooks = \core\hook\manager::get_instance()->get_hooks_deprecating_plugin_callback($function)) { - if (\core\hook\manager::get_instance()->is_deprecating_hook_present($component, $function)) { + $hookmanager = di::get(hook\manager::class); + if ($hooks = $hookmanager->get_hooks_deprecating_plugin_callback($function)) { + if ($hookmanager->is_deprecating_hook_present($component, $function)) { // Do not call the old lib.php callback, // it is there for compatibility with older Moodle versions only. return null; @@ -7776,8 +7781,9 @@ function component_class_callback($classname, $methodname, array $params, $defau $functionparts = explode('\\', trim($fullfunction, '\\')); $component = $functionparts[0]; $callback = end($functionparts); - if ($hooks = \core\hook\manager::get_instance()->get_hooks_deprecating_plugin_callback($callback)) { - if (\core\hook\manager::get_instance()->is_deprecating_hook_present($component, $callback)) { + $hookmanager = di::get(hook\manager::class); + if ($hooks = $hookmanager->get_hooks_deprecating_plugin_callback($callback)) { + if ($hookmanager->is_deprecating_hook_present($component, $callback)) { // Do not call the old class callback, // it is there for compatibility with older Moodle versions only. return null; diff --git a/lib/phpunit/bootstrap.php b/lib/phpunit/bootstrap.php index 0ac815e3143..9aedea38c3e 100644 --- a/lib/phpunit/bootstrap.php +++ b/lib/phpunit/bootstrap.php @@ -262,6 +262,7 @@ if (PHPUNIT_UTIL) { // Make sure the hook manager gets initialised before anybody tries to override callbacks, // this is not using caches intentionally to help with development. +// Note: We cannot use DI at this point in the bootstrap either. \core\hook\manager::get_instance(); // Is database and dataroot ready for testing? diff --git a/lib/phpunit/classes/advanced_testcase.php b/lib/phpunit/classes/advanced_testcase.php index 4f039900f6c..598a37b82b7 100644 --- a/lib/phpunit/classes/advanced_testcase.php +++ b/lib/phpunit/classes/advanced_testcase.php @@ -14,6 +14,11 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . +use core\{ + di, + hook, +}; + /** * Advanced PHPUnit test case customised for Moodle. * @@ -486,7 +491,7 @@ abstract class advanced_testcase extends base_testcase { * @return void */ public function redirectHook(string $hookname, callable $callback): void { - \core\hook\manager::get_instance()->phpunit_redirect_hook($hookname, $callback); + di::get(hook\manager::class)->phpunit_redirect_hook($hookname, $callback); } /** @@ -495,7 +500,7 @@ abstract class advanced_testcase extends base_testcase { * @return void */ public function stopHookRedirections(): void { - \core\hook\manager::get_instance()->phpunit_stop_redirections(); + di::get(hook\manager::class)->phpunit_stop_redirections(); } /** diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index 1d2b5fd216e..f26f3c04a28 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -23,6 +23,11 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +use core\{ + di, + hook, +}; + require_once(__DIR__.'/../../testing/classes/util.php'); require_once(__DIR__ . "/coverage_info.php"); @@ -105,7 +110,7 @@ class phpunit_util extends testing_util { global $DB, $CFG, $USER, $SITE, $COURSE, $PAGE, $OUTPUT, $SESSION, $FULLME, $FILTERLIB_PRIVATE; // Stop all hook redirections. - \core\hook\manager::get_instance()->phpunit_stop_redirections(); + di::get(hook\manager::class)->phpunit_stop_redirections(); // Reset the hook manager instance. \core\hook\manager::phpunit_reset_instance(); diff --git a/mod/quiz/classes/grade_calculator.php b/mod/quiz/classes/grade_calculator.php index 122088297af..69a8158ad91 100644 --- a/mod/quiz/classes/grade_calculator.php +++ b/mod/quiz/classes/grade_calculator.php @@ -17,6 +17,10 @@ namespace mod_quiz; use coding_exception; +use core\{ + di, + hook, +}; use mod_quiz\event\quiz_grade_updated; use mod_quiz\hook\structure_modified; use question_engine_data_mapper; @@ -100,7 +104,7 @@ class grade_calculator { component_class_callback($callbackclass, 'callback', [$quiz->id], null, true); } - \core\hook\manager::get_instance()->dispatch(new structure_modified($this->quizobj->get_structure())); + di::get(hook\manager::class)->dispatch(new structure_modified($this->quizobj->get_structure())); } /** diff --git a/mod/quiz/classes/quiz_attempt.php b/mod/quiz/classes/quiz_attempt.php index 08c40dac12d..6b93de93761 100644 --- a/mod/quiz/classes/quiz_attempt.php +++ b/mod/quiz/classes/quiz_attempt.php @@ -21,6 +21,10 @@ use block_contents; use cm_info; use coding_exception; use context_module; +use core\{ + di, + hook, +}; use Exception; use html_writer; use mod_quiz\hook\attempt_state_changed; @@ -1787,7 +1791,7 @@ class quiz_attempt { // Trigger event. $this->fire_state_transition_event('\mod_quiz\event\attempt_submitted', $timestamp, $studentisonline); - \core\hook\manager::get_instance()->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); + di::get(hook\manager::class)->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); // Tell any access rules that care that the attempt is over. $this->get_access_manager($timestamp)->current_attempt_finished(); } @@ -1835,7 +1839,7 @@ class quiz_attempt { $this->fire_state_transition_event('\mod_quiz\event\attempt_becameoverdue', $timestamp, $studentisonline); - \core\hook\manager::get_instance()->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); + di::get(hook\manager::class)->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); $transaction->allow_commit(); quiz_send_overdue_message($this); @@ -1859,7 +1863,7 @@ class quiz_attempt { $this->fire_state_transition_event('\mod_quiz\event\attempt_abandoned', $timestamp, $studentisonline); - \core\hook\manager::get_instance()->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); + di::get(hook\manager::class)->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); $transaction->allow_commit(); } @@ -1890,7 +1894,7 @@ class quiz_attempt { $this->fire_state_transition_event('\mod_quiz\event\attempt_reopened', $timestamp, false); - \core\hook\manager::get_instance()->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); + di::get(hook\manager::class)->dispatch(new attempt_state_changed($originalattempt, $this->attempt)); $timeclose = $this->get_access_manager($timestamp)->get_end_time($this->attempt); if ($timeclose && $timestamp > $timeclose) { $this->process_finish($timestamp, false, $timeclose); diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index 7459560f2d3..7d60989a26b 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -35,6 +35,10 @@ require_once($CFG->libdir . '/completionlib.php'); require_once($CFG->libdir . '/filelib.php'); require_once($CFG->libdir . '/questionlib.php'); +use core\{ + di, + hook, +}; use core_question\local\bank\condition; use mod_quiz\access_manager; use mod_quiz\event\attempt_submitted; @@ -147,7 +151,7 @@ function quiz_create_attempt(quiz_settings $quizobj, $attemptnumber, $lastattemp $attempt->timecheckstate = $timeclose; } - \core\hook\manager::get_instance()->dispatch(new attempt_state_changed(null, $attempt)); + di::get(hook\manager::class)->dispatch(new attempt_state_changed(null, $attempt)); return $attempt; } @@ -459,7 +463,7 @@ function quiz_delete_attempt($attempt, $quiz) { component_class_callback($callbackclass, 'callback', [$quiz->id], null, true); } - \core\hook\manager::get_instance()->dispatch(new attempt_state_changed($attempt, null)); + di::get(hook\manager::class)->dispatch(new attempt_state_changed($attempt, null)); } // Search quiz_attempts for other instances by this user. From fa2e11105a24309f643454a5528f28ef252f9f58 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Sun, 7 Apr 2024 22:34:58 +0800 Subject: [PATCH 2/7] MDL-81456 core: Stop injecting test data into real hook manager --- lib/classes/hook/manager.php | 21 ++---------- lib/phpunit/classes/util.php | 3 -- lib/tests/hook/manager_test.php | 59 ++++++++++++--------------------- lib/upgrade.txt | 2 -- 4 files changed, 24 insertions(+), 61 deletions(-) diff --git a/lib/classes/hook/manager.php b/lib/classes/hook/manager.php index 259a7a27539..febb8bae0f7 100644 --- a/lib/classes/hook/manager.php +++ b/lib/classes/hook/manager.php @@ -82,34 +82,17 @@ final class manager implements * Factory method for testing of hook manager in PHPUnit tests. * * @param array $componentfiles list of hook callback files for each component. - * @param bool $persist If true, the test instance will be stored in self::$instance. Be sure to call $this->resetAfterTest() - * in your test if you use this. * @return self */ - public static function phpunit_get_instance(array $componentfiles, bool $persist = false): manager { + public static function phpunit_get_instance(array $componentfiles): manager { if (!PHPUNIT_TEST) { throw new \coding_exception('Invalid call of manager::phpunit_get_instance() outside of tests'); } $instance = new self(); $instance->load_callbacks($componentfiles); - if ($persist) { - self::$instance = $instance; - } return $instance; } - /** - * Reset self::$instance so that future calls to ::get_instance() will return a regular instance. - * - * @return void - */ - public static function phpunit_reset_instance(): void { - if (!PHPUNIT_TEST) { - throw new \coding_exception('Invalid call of manager::phpunit_reset_instance() outside of tests'); - } - self::$instance = null; - } - /** * Override hook callbacks for testing purposes. * @@ -599,7 +582,7 @@ final class manager implements public function is_deprecated_plugin_callback(string $plugincallback): bool { debugging( 'is_deprecated_plugin_callback method is deprecated, use get_hooks_deprecating_plugin_callback instead.', - DEBUG_DEVELOPER + DEBUG_DEVELOPER, ); return (bool)$this->get_hooks_deprecating_plugin_callback($plugincallback); } diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index f26f3c04a28..527036ce5be 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -112,9 +112,6 @@ class phpunit_util extends testing_util { // Stop all hook redirections. di::get(hook\manager::class)->phpunit_stop_redirections(); - // Reset the hook manager instance. - \core\hook\manager::phpunit_reset_instance(); - // Stop any message redirection. self::stop_message_redirection(); diff --git a/lib/tests/hook/manager_test.php b/lib/tests/hook/manager_test.php index 076a8a2abcc..574926ed51c 100644 --- a/lib/tests/hook/manager_test.php +++ b/lib/tests/hook/manager_test.php @@ -16,6 +16,8 @@ namespace core\hook; +use core\di; + /** * Hooks tests. * @@ -49,27 +51,8 @@ final class manager_test extends \advanced_testcase { $componentfiles = [ 'test_plugin1' => __DIR__ . '/../fixtures/hook/hooks1_valid.php', ]; - $testmanager = manager::phpunit_get_instance($componentfiles, true); + $testmanager = manager::phpunit_get_instance($componentfiles); $this->assertSame(['test_plugin\\hook\\hook'], $testmanager->get_hooks_with_callbacks()); - // With $persist = true, get_instance() returns the test instance until reset. - $manager = manager::get_instance(); - $this->assertSame($testmanager, $manager); - } - - /** - * Test resetting the manager test instance. - * - * @covers ::phpunit_reset_instance - * @return void - */ - public function test_phpunit_reset_instance(): void { - $testmanager = manager::phpunit_get_instance([], true); - $manager = manager::get_instance(); - $this->assertSame($testmanager, $manager); - - manager::phpunit_reset_instance(); - $manager = manager::get_instance(); - $this->assertNotSame($testmanager, $manager); } /** @@ -377,11 +360,13 @@ final class manager_test extends \advanced_testcase { $this->setup_hooktest_plugin(); // Register the fake plugin with the hook manager, but don't define any hook callbacks. - manager::phpunit_get_instance( - [ - 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php', - ], - true + di::set( + manager::class, + manager::phpunit_get_instance( + [ + 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php', + ], + ), ); // Confirm a non-deprecated callback is called as expected. @@ -419,11 +404,11 @@ final class manager_test extends \advanced_testcase { $this->setup_hooktest_plugin(); // Register the fake plugin with the hook manager, including the hook callback. - manager::phpunit_get_instance( - [ + di::set( + manager::class, + manager::phpunit_get_instance([ 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks.php', - ], - true + ]), ); // Confirm a non-deprecated callback is called as expected. @@ -455,11 +440,11 @@ final class manager_test extends \advanced_testcase { $this->setup_hooktest_plugin(); // Register the fake plugin with the hook manager, but don't define any hook callbacks. - manager::phpunit_get_instance( - [ + di::set( + manager::class, + manager::phpunit_get_instance([ 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php', - ], - true + ]), ); // Confirm a non-deprecated class callback is called as expected. @@ -502,11 +487,11 @@ final class manager_test extends \advanced_testcase { $this->setup_hooktest_plugin(); // Register the fake plugin with the hook manager, including the hook callback. - manager::phpunit_get_instance( - [ + di::set( + manager::class, + manager::phpunit_get_instance([ 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks.php', - ], - true + ]), ); // Confirm a non-deprecated class callback is called as expected. diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 0ec2de0aade..09a2ec88eb5 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -103,8 +103,6 @@ information provided here is intended especially for developers. * Deprecated core\hook\manager::is_deprecated_plugin_callback() in favour of ::get_hooks_deprecating_plugin_callback(), which will return the classnames of hooks deprecating a callback, or null if it's not deprecated. The return value can be cast to bool if the original functionality is desired. -* core\hook\manager::phpunit_get_instance() now sets self::$instance to the mocked instance if the optional $persist argument is - true, so future calls to ::get_instance() will return it. * The triggerSelector method in the `core/comboboxsearch/search_combobox` JS module is deprecated. It was not used. * PHPUnit has been upgraded to 9.6 (see MDL-81266 for details). The main goal of the update is to allow developers to know in advance, From d7e1f2cef703608018c0d95a6e49555a368b91a8 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 8 Apr 2024 08:54:08 +0800 Subject: [PATCH 3/7] MDL-81456 core: Run tests modifying core_component in a separate process --- lib/phpunit/classes/advanced_testcase.php | 62 +++++++++++++++++++++++ lib/tests/hook/manager_test.php | 51 +++---------------- 2 files changed, 69 insertions(+), 44 deletions(-) diff --git a/lib/phpunit/classes/advanced_testcase.php b/lib/phpunit/classes/advanced_testcase.php index 598a37b82b7..6e4716ac78f 100644 --- a/lib/phpunit/classes/advanced_testcase.php +++ b/lib/phpunit/classes/advanced_testcase.php @@ -768,4 +768,66 @@ abstract class advanced_testcase extends base_testcase { return $clock; } + + /** + * Add a mocked plugintype to Moodle. + * + * A new plugintype name must be provided with a path to the plugintype's root. + * + * Please note that tests calling this method must be run in separate isolation mode. + * Please avoid using this if at all possible. + * + * @param string $plugintype The name of the plugintype + * @param string $path The path to the plugintype's root + */ + protected function add_mocked_plugintype( + string $plugintype, + string $path, + ): void { + require_phpunit_isolation(); + + $mockedcomponent = new \ReflectionClass(\core_component::class); + $plugintypes = $mockedcomponent->getStaticPropertyValue('plugintypes'); + + if (array_key_exists($plugintype, $plugintypes)) { + throw new \coding_exception("The plugintype '{$plugintype}' already exists."); + } + + $plugintypes[$plugintype] = $path; + $mockedcomponent->setStaticPropertyValue('plugintypes', $plugintypes); + + $this->resetDebugging(); + } + + /** + * Add a mocked plugin to Moodle. + * + * A new plugin name must be provided with a path to the plugin's root. + * The plugin type must already exist (or have been mocked separately). + * + * Please note that tests calling this method must be run in separate isolation mode. + * Please avoid using this if at all possible. + * + * @param string $plugintype The name of the plugintype + * @param string $pluginname The name of the plugin + * @param string $path The path to the plugin's root + */ + protected function add_mocked_plugin( + string $plugintype, + string $pluginname, + string $path, + ): void { + require_phpunit_isolation(); + + $mockedcomponent = new \ReflectionClass(\core_component::class); + $plugins = $mockedcomponent->getStaticPropertyValue('plugins'); + + if (!array_key_exists($plugintype, $plugins)) { + $plugins[$plugintype] = []; + } + + $plugins[$plugintype][$pluginname] = $path; + $mockedcomponent->setStaticPropertyValue('plugins', $plugins); + $this->resetDebugging(); + } } diff --git a/lib/tests/hook/manager_test.php b/lib/tests/hook/manager_test.php index 574926ed51c..577cc163407 100644 --- a/lib/tests/hook/manager_test.php +++ b/lib/tests/hook/manager_test.php @@ -303,42 +303,13 @@ final class manager_test extends \advanced_testcase { /** * Register a fake plugin called hooktest in the component manager. * - * @return void + * Tests consuming this helpers must run in a separate process. */ protected function setup_hooktest_plugin(): void { global $CFG; - $mockedcomponent = new \ReflectionClass(\core_component::class); - $mockedplugintypes = $mockedcomponent->getProperty('plugintypes'); - $mockedplugintypes->setAccessible(true); - $plugintypes = $mockedplugintypes->getValue(); - $plugintypes['fake'] = "{$CFG->dirroot}/lib/tests/fixtures/fakeplugins"; - $mockedplugintypes->setValue(null, $plugintypes); - $mockedplugins = $mockedcomponent->getProperty('plugins'); - $mockedplugins->setAccessible(true); - $plugins = $mockedplugins->getValue(); - $plugins['fake'] = ['hooktest' => "{$CFG->dirroot}/lib/tests/fixtures/fakeplugins/hooktest"]; - $mockedplugins->setValue(null, $plugins); - $this->resetDebugging(); - } - - /** - * Remove the fake plugin to avoid interference with other tests. - * - * @return void - */ - protected function remove_hooktest_plugin(): void { - $mockedcomponent = new \ReflectionClass(\core_component::class); - $mockedplugintypes = $mockedcomponent->getProperty('plugintypes'); - $mockedplugintypes->setAccessible(true); - $plugintypes = $mockedplugintypes->getValue(); - unset($plugintypes['fake']); - $mockedplugintypes->setValue(null, $plugintypes); - $mockedplugins = $mockedcomponent->getProperty('plugins'); - $mockedplugins->setAccessible(true); - $plugins = $mockedplugins->getValue(); - unset($plugins['fake']); - $mockedplugins->setValue(null, $plugins); + $this->add_mocked_plugintype('fake', "{$CFG->dirroot}/lib/tests/fixtures/fakeplugins"); + $this->add_mocked_plugin('fake', 'hooktest', "{$CFG->dirroot}/lib/tests/fixtures/fakeplugins/hooktest"); } /** @@ -348,8 +319,7 @@ final class manager_test extends \advanced_testcase { * * @covers ::get_hooks_deprecating_plugin_callback() * @covers ::is_deprecating_hook_present() - * @return void - * @throws \coding_exception + * @runInSeparateProcess */ public function test_migrated_callback(): void { $this->resetAfterTest(true); @@ -381,7 +351,6 @@ final class manager_test extends \advanced_testcase { 'Callback old_callback in fake_hooktest component should be migrated to new hook '. 'callback for fake_hooktest\hook\hook_replacing_callback' ); - $this->remove_hooktest_plugin(); } /** @@ -391,8 +360,7 @@ final class manager_test extends \advanced_testcase { * * @covers ::get_hooks_deprecating_plugin_callback() * @covers ::is_deprecating_hook_present() - * @return void - * @throws \coding_exception + * @runInSeparateProcess */ public function test_migrated_callback_with_replacement(): void { $this->resetAfterTest(true); @@ -417,7 +385,6 @@ final class manager_test extends \advanced_testcase { // Confirm the deprecated callback is not called, as expected. $this->assertNull(component_callback('fake_hooktest', 'old_callback', [], null, true)); $this->assertDebuggingNotCalled(); - $this->remove_hooktest_plugin(); } /** @@ -427,8 +394,7 @@ final class manager_test extends \advanced_testcase { * * @covers ::get_hooks_deprecating_plugin_callback() * @covers ::is_deprecating_hook_present() - * @return void - * @throws \coding_exception + * @runInSeparateProcess */ public function test_migrated_class_callback(): void { $this->resetAfterTest(true); @@ -462,7 +428,6 @@ final class manager_test extends \advanced_testcase { 'Callback callbacks::old_class_callback in fake_hooktest component should be migrated to new hook '. 'callback for fake_hooktest\hook\hook_replacing_class_callback' ); - $this->remove_hooktest_plugin(); } /** @@ -472,8 +437,7 @@ final class manager_test extends \advanced_testcase { * * @covers ::get_hooks_deprecating_plugin_callback() * @covers ::is_deprecating_hook_present() - * @return void - * @throws \coding_exception + * @runInSeparateProcess */ public function test_migrated_class_callback_with_replacement(): void { $this->resetAfterTest(true); @@ -503,7 +467,6 @@ final class manager_test extends \advanced_testcase { // Confirm the deprecated class callback is not called, as expected. $this->assertNull(component_class_callback('fake_hooktest\callbacks', 'old_class_callback', [], null, true)); $this->assertDebuggingNotCalled(); - $this->remove_hooktest_plugin(); } /** From 71e8387a96aadf02091b776eea5ac253febd98cd Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 8 Apr 2024 09:22:58 +0800 Subject: [PATCH 4/7] MDL-81456 core: Move hook fixtures to more appropriate location --- .../hooktest/classes/callbacks.php | 0 .../classes/hook/hook_replacing_callback.php | 0 .../hook/hook_replacing_class_callback.php | 0 .../hooktest/classes/hook_callbacks.php | 0 .../fakeplugins/hooktest/classes/hooks.php | 0 .../fakeplugins/hooktest/db/hooks.php | 0 .../hooktest/db/hooks_nocallbacks.php | 0 .../{ => hook}/fakeplugins/hooktest/lib.php | 0 .../fakeplugins/hooktest/version.php | 0 lib/tests/hook/manager_test.php | 36 +++++++++---------- 10 files changed, 18 insertions(+), 18 deletions(-) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/classes/callbacks.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/classes/hook_callbacks.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/classes/hooks.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/db/hooks.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/db/hooks_nocallbacks.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/lib.php (100%) rename lib/tests/fixtures/{ => hook}/fakeplugins/hooktest/version.php (100%) diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/callbacks.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/classes/callbacks.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/classes/callbacks.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/classes/callbacks.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hook_callbacks.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hook_callbacks.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/classes/hook_callbacks.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hook_callbacks.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hooks.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hooks.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/classes/hooks.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/classes/hooks.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/db/hooks.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/db/hooks.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/db/hooks.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/db/hooks.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/db/hooks_nocallbacks.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/db/hooks_nocallbacks.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/lib.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/lib.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/lib.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/lib.php diff --git a/lib/tests/fixtures/fakeplugins/hooktest/version.php b/lib/tests/fixtures/hook/fakeplugins/hooktest/version.php similarity index 100% rename from lib/tests/fixtures/fakeplugins/hooktest/version.php rename to lib/tests/fixtures/hook/fakeplugins/hooktest/version.php diff --git a/lib/tests/hook/manager_test.php b/lib/tests/hook/manager_test.php index 577cc163407..77354b58b8b 100644 --- a/lib/tests/hook/manager_test.php +++ b/lib/tests/hook/manager_test.php @@ -308,8 +308,8 @@ final class manager_test extends \advanced_testcase { protected function setup_hooktest_plugin(): void { global $CFG; - $this->add_mocked_plugintype('fake', "{$CFG->dirroot}/lib/tests/fixtures/fakeplugins"); - $this->add_mocked_plugin('fake', 'hooktest', "{$CFG->dirroot}/lib/tests/fixtures/fakeplugins/hooktest"); + $this->add_mocked_plugintype('fake', "{$CFG->dirroot}/lib/tests/fixtures/hook/fakeplugins"); + $this->add_mocked_plugin('fake', 'hooktest', "{$CFG->dirroot}/lib/tests/fixtures/hook/fakeplugins/hooktest"); } /** @@ -324,8 +324,8 @@ final class manager_test extends \advanced_testcase { public function test_migrated_callback(): void { $this->resetAfterTest(true); // Include plugin hook discovery agent, and the hook that replaces the callback. - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hooks.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hooks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php'); // Register the fake plugin with the component manager. $this->setup_hooktest_plugin(); @@ -334,7 +334,7 @@ final class manager_test extends \advanced_testcase { manager::class, manager::phpunit_get_instance( [ - 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php', + 'fake_hooktest' => __DIR__ . '/../fixtures/hook/fakeplugins/hooktest/db/hooks_nocallbacks.php', ], ), ); @@ -365,9 +365,9 @@ final class manager_test extends \advanced_testcase { public function test_migrated_callback_with_replacement(): void { $this->resetAfterTest(true); // Include plugin hook discovery agent, and the hook that replaces the callback, and a hook callback for the hook. - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hooks.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hook_callbacks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hooks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hook_callbacks.php'); // Register the fake plugin with the component manager. $this->setup_hooktest_plugin(); @@ -375,7 +375,7 @@ final class manager_test extends \advanced_testcase { di::set( manager::class, manager::phpunit_get_instance([ - 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks.php', + 'fake_hooktest' => __DIR__ . '/../fixtures/hook/fakeplugins/hooktest/db/hooks.php', ]), ); @@ -399,9 +399,9 @@ final class manager_test extends \advanced_testcase { public function test_migrated_class_callback(): void { $this->resetAfterTest(true); // Include plugin hook discovery agent, the class containing callbacks, and the hook that replaces the class callback. - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/callbacks.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hooks.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/callbacks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hooks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php'); // Register the fake plugin with the component manager. $this->setup_hooktest_plugin(); @@ -409,7 +409,7 @@ final class manager_test extends \advanced_testcase { di::set( manager::class, manager::phpunit_get_instance([ - 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php', + 'fake_hooktest' => __DIR__ . '/../fixtures/hook/fakeplugins/hooktest/db/hooks_nocallbacks.php', ]), ); @@ -443,10 +443,10 @@ final class manager_test extends \advanced_testcase { $this->resetAfterTest(true); // Include plugin hook discovery agent, the class containing callbacks, the hook that replaces the class callback, // and a hook callback for the new hook. - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/callbacks.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hooks.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php'); - require_once(__DIR__ . '/../fixtures/fakeplugins/hooktest/classes/hook_callbacks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/callbacks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hooks.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php'); + require_once(__DIR__ . '/../fixtures/hook/fakeplugins/hooktest/classes/hook_callbacks.php'); // Register the fake plugin with the component manager. $this->setup_hooktest_plugin(); @@ -454,7 +454,7 @@ final class manager_test extends \advanced_testcase { di::set( manager::class, manager::phpunit_get_instance([ - 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks.php', + 'fake_hooktest' => __DIR__ . '/../fixtures/hook/fakeplugins/hooktest/db/hooks.php', ]), ); From 3cc3511fa262b328da0df4ae852ade1fdb2f4beb Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 8 Apr 2024 09:25:17 +0800 Subject: [PATCH 5/7] MDL-81456 core: Remove superflous coverage information --- lib/tests/hook/manager_test.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/tests/hook/manager_test.php b/lib/tests/hook/manager_test.php index 77354b58b8b..55c8edf1db0 100644 --- a/lib/tests/hook/manager_test.php +++ b/lib/tests/hook/manager_test.php @@ -317,8 +317,6 @@ final class manager_test extends \advanced_testcase { * * The original callback should be called, but a debugging message should be output. * - * @covers ::get_hooks_deprecating_plugin_callback() - * @covers ::is_deprecating_hook_present() * @runInSeparateProcess */ public function test_migrated_callback(): void { @@ -358,8 +356,6 @@ final class manager_test extends \advanced_testcase { * * The original callback should not be called, and no debugging should be output. * - * @covers ::get_hooks_deprecating_plugin_callback() - * @covers ::is_deprecating_hook_present() * @runInSeparateProcess */ public function test_migrated_callback_with_replacement(): void { @@ -392,8 +388,6 @@ final class manager_test extends \advanced_testcase { * * The original class callback should be called, but a debugging message should be output. * - * @covers ::get_hooks_deprecating_plugin_callback() - * @covers ::is_deprecating_hook_present() * @runInSeparateProcess */ public function test_migrated_class_callback(): void { @@ -435,8 +429,6 @@ final class manager_test extends \advanced_testcase { * * The original callback should not be called, and no debugging should be output. * - * @covers ::get_hooks_deprecating_plugin_callback() - * @covers ::is_deprecating_hook_present() * @runInSeparateProcess */ public function test_migrated_class_callback_with_replacement(): void { From 4a1752781f6fec8cc3f14ae601fd393a7272069d Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 8 Apr 2024 09:26:10 +0800 Subject: [PATCH 6/7] MDL-81456 core: Coding style fixups --- lib/classes/hook/manager.php | 16 ++++------------ lib/tests/hook/manager_test.php | 5 ++--- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/lib/classes/hook/manager.php b/lib/classes/hook/manager.php index febb8bae0f7..a98c42cc9d8 100644 --- a/lib/classes/hook/manager.php +++ b/lib/classes/hook/manager.php @@ -45,7 +45,6 @@ use Psr\EventDispatcher\StoppableEventInterface; final class manager implements EventDispatcherInterface, ListenerProviderInterface { - /** @var ?manager the one instance of listener provider and dispatcher */ private static $instance = null; @@ -98,7 +97,6 @@ final class manager implements * * @param string $hookname * @param callable $callback - * @return void */ public function phpunit_redirect_hook(string $hookname, callable $callback): void { if (!PHPUNIT_TEST) { @@ -109,8 +107,6 @@ final class manager implements /** * Cancel all redirections of hook callbacks. - * - * @return void */ public function phpunit_stop_redirections(): void { if (!PHPUNIT_TEST) { @@ -150,7 +146,7 @@ final class manager implements * callable MUST be type-compatible with $event. * Please note that in Moodle the callable must be a string. */ - public function getListenersForEvent(object $event): iterable { + public function getListenersForEvent(object $event): iterable { // phpcs:ignore // Callbacks are sorted by priority, highest first at load-time. $hookclassname = get_class($event); $callbacks = $this->get_callbacks_for_hook($hookclassname); @@ -237,7 +233,7 @@ final class manager implements debugging( "Cannot execute callback '$callablename' from '$component'" . "Callback method not callable.", - DEBUG_DEVELOPER + DEBUG_DEVELOPER, ); return false; } @@ -307,8 +303,6 @@ final class manager implements /** * Initialise list of all callbacks for each hook. - * - * @return void */ private function init_standard_callbacks(): void { global $CFG; @@ -363,7 +357,6 @@ final class manager implements * Load callbacks from component db/hooks.php files. * * @param array $componentfiles list of all components with their callback files - * @return void */ private function load_callbacks(array $componentfiles): void { $this->allcallbacks = []; @@ -487,14 +480,13 @@ final class manager implements * * @param string $component component where hook callbacks are defined * @param string $hookfile file with list of all callbacks for component - * @return void */ private function add_component_callbacks(string $component, string $hookfile): void { if (!file_exists($hookfile)) { return; } - $parsecallbacks = function($hookfile) { + $parsecallbacks = function ($hookfile) { $callbacks = []; include($hookfile); return $callbacks; @@ -560,7 +552,7 @@ final class manager implements if (!str_contains($classmethod, '::')) { debugging( "Hook callback definition contains invalid 'callback' static class method string in '$component'", - DEBUG_DEVELOPER + DEBUG_DEVELOPER, ); return null; } diff --git a/lib/tests/hook/manager_test.php b/lib/tests/hook/manager_test.php index 55c8edf1db0..de1e29be6ed 100644 --- a/lib/tests/hook/manager_test.php +++ b/lib/tests/hook/manager_test.php @@ -346,7 +346,7 @@ final class manager_test extends \advanced_testcase { component_callback('fake_hooktest', 'old_callback', [], null, true) ); $this->assertDebuggingCalled( - 'Callback old_callback in fake_hooktest component should be migrated to new hook '. + 'Callback old_callback in fake_hooktest component should be migrated to new hook ' . 'callback for fake_hooktest\hook\hook_replacing_callback' ); } @@ -419,7 +419,7 @@ final class manager_test extends \advanced_testcase { component_class_callback('fake_hooktest\callbacks', 'old_class_callback', [], null, true) ); $this->assertDebuggingCalled( - 'Callback callbacks::old_class_callback in fake_hooktest component should be migrated to new hook '. + 'Callback callbacks::old_class_callback in fake_hooktest component should be migrated to new hook ' . 'callback for fake_hooktest\hook\hook_replacing_class_callback' ); } @@ -465,7 +465,6 @@ final class manager_test extends \advanced_testcase { * Normalise the sort order of callbacks to help with asserts. * * @param array $callbacks - * @return void */ private function normalise_callbacks(array &$callbacks): void { foreach ($callbacks as &$callback) { From 4475617583a96a8178608808b671093354a027fa Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 8 Apr 2024 09:39:18 +0800 Subject: [PATCH 7/7] MDL-81456 core: Provide info about mocking hooks --- lib/classes/hook/manager.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/classes/hook/manager.php b/lib/classes/hook/manager.php index a98c42cc9d8..a7a027b7c72 100644 --- a/lib/classes/hook/manager.php +++ b/lib/classes/hook/manager.php @@ -80,6 +80,8 @@ final class manager implements /** * Factory method for testing of hook manager in PHPUnit tests. * + * Please note that the result of this method should typically be passed to \core\di::set(). + * * @param array $componentfiles list of hook callback files for each component. * @return self */