diff --git a/lib/classes/hook/manager.php b/lib/classes/hook/manager.php index 68475fa6707..259a7a27539 100644 --- a/lib/classes/hook/manager.php +++ b/lib/classes/hook/manager.php @@ -82,17 +82,34 @@ 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): manager { + public static function phpunit_get_instance(array $componentfiles, bool $persist = false): 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. * @@ -576,9 +593,25 @@ final class manager implements * * @param string $plugincallback short callback name without the component prefix * @return bool + * @deprecated in favour of get_hooks_deprecating_plugin_callback since Moodle 4.4. + * @todo Remove in Moodle 4.8 (MDL-80327). */ public function is_deprecated_plugin_callback(string $plugincallback): bool { - return isset($this->alldeprecations[$plugincallback]); + debugging( + 'is_deprecated_plugin_callback method is deprecated, use get_hooks_deprecating_plugin_callback instead.', + DEBUG_DEVELOPER + ); + return (bool)$this->get_hooks_deprecating_plugin_callback($plugincallback); + } + + /** + * If the plugin callback from lib.php is deprecated by any hooks, return the hooks' classnames. + * + * @param string $plugincallback short callback name without the component prefix + * @return ?array + */ + public function get_hooks_deprecating_plugin_callback(string $plugincallback): ?array { + return $this->alldeprecations[$plugincallback] ?? null; } /** diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 79053882ced..13391af3a39 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -7453,13 +7453,17 @@ 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 (\core\hook\manager::get_instance()->is_deprecated_plugin_callback($plugincallback)) { + 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)) { // Ignore the old callback, it is there only for older Moodle versions. unset($pluginfunctions[$plugintype][$plugin]); } else { - debugging("Callback $plugincallback in $component component should be migrated to new hook callback", - DEBUG_DEVELOPER); + $hookmessage = count($hooks) == 1 ? reset($hooks) : 'one of ' . implode(', ', $hooks); + debugging( + "Callback $plugincallback in $component component should be migrated to new " . + "hook callback for $hookmessage", + DEBUG_DEVELOPER + ); } } } @@ -7688,13 +7692,15 @@ function component_callback($component, $function, array $params = array(), $def if ($functionname) { if ($migratedtohook) { - if (\core\hook\manager::get_instance()->is_deprecated_plugin_callback($function)) { + 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)) { // Do not call the old lib.php callback, // it is there for compatibility with older Moodle versions only. return null; } else { - debugging("Callback $function in $component component should be migrated to new hook callback", + $hookmessage = count($hooks) == 1 ? reset($hooks) : 'one of ' . implode(', ', $hooks); + debugging( + "Callback $function in $component component should be migrated to new hook callback for $hookmessage", DEBUG_DEVELOPER); } } @@ -7769,9 +7775,10 @@ function component_callback_exists($component, $function) { * @param string $methodname The name of the staticically defined method on the class. * @param array $params The arguments to pass into the method. * @param mixed $default The default value. + * @param bool $migratedtohook True if the callback has been migrated to a hook. * @return mixed The return value. */ -function component_class_callback($classname, $methodname, array $params, $default = null) { +function component_class_callback($classname, $methodname, array $params, $default = null, bool $migratedtohook = false) { if (!class_exists($classname)) { return $default; } @@ -7781,6 +7788,24 @@ function component_class_callback($classname, $methodname, array $params, $defau } $fullfunction = $classname . '::' . $methodname; + + if ($migratedtohook) { + $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)) { + // Do not call the old class callback, + // it is there for compatibility with older Moodle versions only. + return null; + } else { + $hookmessage = count($hooks) == 1 ? reset($hooks) : 'one of ' . implode(', ', $hooks); + debugging("Callback $callback in $component component should be migrated to new hook callback for $hookmessage", + DEBUG_DEVELOPER); + } + } + } + $result = call_user_func_array($fullfunction, $params); if (null === $result) { diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index 86e8fa47cbe..1d2b5fd216e 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -107,6 +107,9 @@ class phpunit_util extends testing_util { // Stop all hook redirections. \core\hook\manager::get_instance()->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/fixtures/fakeplugins/hooktest/classes/callbacks.php b/lib/tests/fixtures/fakeplugins/hooktest/classes/callbacks.php new file mode 100644 index 00000000000..c0ac0d22cbc --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/classes/callbacks.php @@ -0,0 +1,44 @@ +. +namespace fake_hooktest; + +/** + * Class callback container for fake_hooktest + * + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class callbacks { + /** + * Test callback that is not replaced by a hook. + * + * @return string + */ + public static function current_class_callback(): string { + return 'Called current class callback'; + } + + /** + * Test callback that is replaced by a hook. + * + * @return string + */ + public static function old_class_callback(): string { + return 'Called deprecated class callback'; + } +} diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php b/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php new file mode 100644 index 00000000000..921b8670af5 --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_callback.php @@ -0,0 +1,53 @@ +. + +namespace fake_hooktest\hook; + +/** + * Fixture for testing of hooks. + * + * @package core + * @author Mark Johnson + * @copyright 2024 Catalyst IT Europe Ltd. + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +final class hook_replacing_callback implements + \core\hook\described_hook, + \core\hook\deprecated_callback_replacement { + + /** + * Hook description. + */ + public static function get_hook_description(): string { + return 'Test hook replacing a plugin callback function.'; + } + + /** + * Deprecation info. + */ + public static function get_deprecated_plugin_callbacks(): array { + return ['old_callback']; + } + + /** + * List of tags that describe this hook. + * + * @return string[] + */ + public static function get_hook_tags(): array { + return ['test']; + } +} diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php b/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php new file mode 100644 index 00000000000..7c96aaebd20 --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/classes/hook/hook_replacing_class_callback.php @@ -0,0 +1,33 @@ +. + +namespace fake_hooktest\hook; + +use core\attribute; + +/** + * Fixture for testing of hooks. + * + * @package core + * @author Mark Johnson + * @copyright 2024 Catalyst IT Europe Ltd. + * @license https://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +#[attribute\label('Test hook replacing a class callback.')] +#[attribute\tags('test')] +#[attribute\hook\replaces_callbacks('callbacks::old_class_callback')] +final class hook_replacing_class_callback { +} diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hook_callbacks.php b/lib/tests/fixtures/fakeplugins/hooktest/classes/hook_callbacks.php new file mode 100644 index 00000000000..c3991214f08 --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/classes/hook_callbacks.php @@ -0,0 +1,44 @@ +. +namespace fake_hooktest; + +/** + * Hook callbacks + * + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class hook_callbacks { + /** + * Test callback which replaces a plugin callback. + * + * @return string + */ + public function component_callback_replacement(): string { + return 'Called component callback replacement'; + } + + /** + * Test callback which replaced a plugin class callback. + * + * @return string + */ + public function component_class_callback_replacement(): string { + return 'Called component class callback replacement'; + } +} diff --git a/lib/tests/fixtures/fakeplugins/hooktest/classes/hooks.php b/lib/tests/fixtures/fakeplugins/hooktest/classes/hooks.php new file mode 100644 index 00000000000..0e3c6c21fc4 --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/classes/hooks.php @@ -0,0 +1,41 @@ +. +namespace fake_hooktest; + +/** + * Hook discovery for fake plugin. + * + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class hooks implements \core\hook\discovery_agent { + public static function discover_hooks(): array { + return [ + 'fake_hooktest\hook\hook_replacing_callback' => [ + 'class' => 'fake_hooktest\hook\hook_replacing_callback', + 'description' => 'Hook replacing callback', + 'tags' => ['test'], + ], + 'fake_hooktest\hook\hook_replacing_class_callback' => [ + 'class' => 'fake_hooktest\hook\hook_replacing_class_callback', + 'description' => 'Hook replacing class callback', + 'tags' => ['test'], + ], + ]; + } +} diff --git a/lib/tests/fixtures/fakeplugins/hooktest/db/hooks.php b/lib/tests/fixtures/fakeplugins/hooktest/db/hooks.php new file mode 100644 index 00000000000..c2d71c8ddbd --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/db/hooks.php @@ -0,0 +1,37 @@ +. +/** + * Hook callback definitions for core + * + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +defined('MOODLE_INTERNAL') || die(); + +$callbacks = [ + [ + 'hook' => 'fake_hooktest\hook\hook_replacing_callback', + 'callback' => 'fake_hooktest\hook_callbacks::component_callback_replacement', + 'priority' => 500, + ], + [ + 'hook' => 'fake_hooktest\hook\hook_replacing_class_callback', + 'callback' => 'fake_hooktest\hook_callbacks::component_class_callback_replacement', + 'priority' => 600, + ], +]; diff --git a/mod/quiz/report/statistics/db/events.php b/lib/tests/fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php similarity index 73% rename from mod/quiz/report/statistics/db/events.php rename to lib/tests/fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php index 9028cdfb1cb..12c4b0cb992 100644 --- a/mod/quiz/report/statistics/db/events.php +++ b/lib/tests/fixtures/fakeplugins/hooktest/db/hooks_nocallbacks.php @@ -13,21 +13,15 @@ // // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . - /** - * Add event observers for quiz_statistics + * Hook callback definitions for core * - * @package quiz_statistics - * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} * @author Mark Johnson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ - defined('MOODLE_INTERNAL') || die(); -$observers = [ - [ - 'eventname' => '\mod_quiz\event\attempt_submitted', - 'callback' => '\quiz_statistics\event\observer\attempt_submitted::process', - ], +$callbacks = [ ]; diff --git a/lib/tests/fixtures/fakeplugins/hooktest/lib.php b/lib/tests/fixtures/fakeplugins/hooktest/lib.php new file mode 100644 index 00000000000..c9f4e12e952 --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/lib.php @@ -0,0 +1,41 @@ +. +/** + * Library functions for fake_hooktest + * + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +/** + * Test callback that is not replaced by a hook. + * + * @return string + */ +function fake_hooktest_current_callback() { + return 'Called current callback'; +} + +/** + * Test callback that is replaced by a hook. + * + * @return string + */ +function fake_hooktest_old_callback() { + return 'Called deprecated callback'; +} diff --git a/lib/tests/fixtures/fakeplugins/hooktest/version.php b/lib/tests/fixtures/fakeplugins/hooktest/version.php new file mode 100644 index 00000000000..dac28c28f6e --- /dev/null +++ b/lib/tests/fixtures/fakeplugins/hooktest/version.php @@ -0,0 +1,30 @@ +. + +/** + * Fake plugin for testing hooks. + * + * @package core + * @copyright 2024 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +$plugin->version = 2024012200; +$plugin->requires = 2024011900; +$plugin->component = 'fake_hooktest'; diff --git a/lib/tests/hook/manager_test.php b/lib/tests/hook/manager_test.php index 77a3f55033b..076a8a2abcc 100644 --- a/lib/tests/hook/manager_test.php +++ b/lib/tests/hook/manager_test.php @@ -49,8 +49,27 @@ final class manager_test extends \advanced_testcase { $componentfiles = [ 'test_plugin1' => __DIR__ . '/../fixtures/hook/hooks1_valid.php', ]; - $testmanager = manager::phpunit_get_instance($componentfiles); + $testmanager = manager::phpunit_get_instance($componentfiles, true); $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); } /** @@ -295,6 +314,211 @@ final class manager_test extends \advanced_testcase { $this->assertSame(['test1'], \test_plugin\callbacks::$calls); \test_plugin\callbacks::$calls = []; $this->assertDebuggingNotCalled(); + $CFG->hooks_callback_overrides = []; + } + + /** + * Register a fake plugin called hooktest in the component manager. + * + * @return void + */ + 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); + } + + /** + * Call a plugin callback that has been replaced by a hook, but has no hook callback. + * + * The original callback should be called, but a debugging message should be output. + * + * @covers ::get_hooks_deprecating_plugin_callback() + * @covers ::is_deprecating_hook_present() + * @return void + * @throws \coding_exception + */ + 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'); + // Register the fake plugin with the component manager. + $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 + ); + + // Confirm a non-deprecated callback is called as expected. + $this->assertEquals('Called current callback', component_callback('fake_hooktest', 'current_callback')); + + // Confirm the deprecated callback is called as expected. + $this->assertEquals( + 'Called deprecated callback', + component_callback('fake_hooktest', 'old_callback', [], null, true) + ); + $this->assertDebuggingCalled( + '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(); + } + + /** + * Call a plugin callback that has been replaced by a hook, and has a hook callback. + * + * The original callback should not be called, and no debugging should be output. + * + * @covers ::get_hooks_deprecating_plugin_callback() + * @covers ::is_deprecating_hook_present() + * @return void + * @throws \coding_exception + */ + 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'); + // Register the fake plugin with the component manager. + $this->setup_hooktest_plugin(); + + // Register the fake plugin with the hook manager, including the hook callback. + manager::phpunit_get_instance( + [ + 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks.php', + ], + true + ); + + // Confirm a non-deprecated callback is called as expected. + $this->assertEquals('Called current callback', component_callback('fake_hooktest', 'current_callback')); + + // 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(); + } + + /** + * Call a plugin class callback that has been replaced by a hook, but has no hook callback. + * + * 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() + * @return void + * @throws \coding_exception + */ + 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'); + // Register the fake plugin with the component manager. + $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 + ); + + // Confirm a non-deprecated class callback is called as expected. + $this->assertEquals( + 'Called current class callback', + component_class_callback('fake_hooktest\callbacks', 'current_class_callback', []) + ); + + // Confirm the deprecated class callback is called as expected. + $this->assertEquals( + 'Called deprecated class callback', + 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 for fake_hooktest\hook\hook_replacing_class_callback' + ); + $this->remove_hooktest_plugin(); + } + + /** + * Call a plugin class callback that has been replaced by a hook, and has a hook callback. + * + * The original callback should not be called, and no debugging should be output. + * + * @covers ::get_hooks_deprecating_plugin_callback() + * @covers ::is_deprecating_hook_present() + * @return void + * @throws \coding_exception + */ + public function test_migrated_class_callback_with_replacement(): void { + $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'); + // Register the fake plugin with the component manager. + $this->setup_hooktest_plugin(); + + // Register the fake plugin with the hook manager, including the hook callback. + manager::phpunit_get_instance( + [ + 'fake_hooktest' => __DIR__ . '/../fixtures/fakeplugins/hooktest/db/hooks.php', + ], + true + ); + + // Confirm a non-deprecated class callback is called as expected. + $this->assertEquals( + 'Called current class callback', + component_class_callback('fake_hooktest\callbacks', 'current_class_callback', []) + ); + + // 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(); } /** diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 1763040996f..d94d7131981 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -94,6 +94,11 @@ information provided here is intended especially for developers. * Added a new parameter to `core_renderer::container` and `core_renderer::container_start` to allow for the addition of custom attributes. * Added a new method `navigation_node::add_attribute()` to allow adding HTML attributes to the node. +* 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. === 4.3 === diff --git a/mod/quiz/classes/grade_calculator.php b/mod/quiz/classes/grade_calculator.php index f0d74f7735a..122088297af 100644 --- a/mod/quiz/classes/grade_calculator.php +++ b/mod/quiz/classes/grade_calculator.php @@ -18,6 +18,7 @@ namespace mod_quiz; use coding_exception; use mod_quiz\event\quiz_grade_updated; +use mod_quiz\hook\structure_modified; use question_engine_data_mapper; use stdClass; @@ -92,10 +93,14 @@ class grade_calculator { self::update_quiz_maximum_grade(0); } + // This class callback is deprecated, and will be removed in Moodle 4.8 (MDL-80327). + // Use the structure_modified hook instead. $callbackclasses = \core_component::get_plugin_list_with_class('quiz', 'quiz_structure_modified'); foreach ($callbackclasses as $callbackclass) { - component_class_callback($callbackclass, 'callback', [$quiz->id]); + component_class_callback($callbackclass, 'callback', [$quiz->id], null, true); } + + \core\hook\manager::get_instance()->dispatch(new structure_modified($this->quizobj->get_structure())); } /** diff --git a/mod/quiz/classes/hook/attempt_state_changed.php b/mod/quiz/classes/hook/attempt_state_changed.php new file mode 100644 index 00000000000..0a97413645c --- /dev/null +++ b/mod/quiz/classes/hook/attempt_state_changed.php @@ -0,0 +1,71 @@ +. +namespace mod_quiz\hook; + +use core\attribute; + +/** + * A quiz attempt changed state. + * + * @package mod_quiz + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +#[attribute\label('A quiz attempt changed state.')] +#[attribute\tags('quiz', 'attempt')] +#[attribute\hook\replaces_callbacks('quiz_attempt_deleted::callback')] +class attempt_state_changed { + /** + * Create a new hook instance. + * + * @param ?\stdClass $originalattempt The original database record for the attempt, null if it has just been created. + * @param ?\stdClass $updatedattempt The updated database record of the new attempt, null if it has just been deleted. + */ + public function __construct( + protected ?\stdClass $originalattempt, + protected ?\stdClass $updatedattempt, + ) { + if (is_null($this->originalattempt) && is_null($this->updatedattempt)) { + throw new \InvalidArgumentException('originalattempt and updatedattempt cannot both be null.'); + } + if ( + !is_null($this->originalattempt) + && !is_null($this->updatedattempt) + && $this->originalattempt->id != $this->updatedattempt->id + ) { + throw new \InvalidArgumentException('originalattempt and updatedattempt must have the same id.'); + } + } + + /** + * Get the original attempt, null if it has just been created. + * + * @return ?\stdClass + */ + public function get_original_attempt(): ?\stdClass { + return $this->originalattempt; + } + + /** + * Get the updated attempt, null if it has just been deleted. + * + * @return ?\stdClass + */ + public function get_updated_attempt(): ?\stdClass { + return $this->updatedattempt; + } +} diff --git a/mod/quiz/classes/hook/structure_modified.php b/mod/quiz/classes/hook/structure_modified.php new file mode 100644 index 00000000000..e39bbf0c349 --- /dev/null +++ b/mod/quiz/classes/hook/structure_modified.php @@ -0,0 +1,51 @@ +. +namespace mod_quiz\hook; + +use core\attribute; +use mod_quiz\structure; + +/** + * The quiz structure has been modified + * + * @package mod_quiz + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +#[attribute\label('The quiz structure has been modified')] +#[attribute\tags('quiz', 'structure')] +#[attribute\hook\replaces_callbacks('quiz_structure_modified::callback')] +class structure_modified { + /** + * Create a new hook with the modified structure. + * + * @param structure $structure The new structure. + */ + public function __construct( + protected structure $structure + ) { + } + + /** + * Returns the new structure of the quiz. + * + * @return structure The structure object. + */ + public function get_structure(): structure { + return $this->structure; + } +} diff --git a/mod/quiz/classes/quiz_attempt.php b/mod/quiz/classes/quiz_attempt.php index 9354959d04d..08c40dac12d 100644 --- a/mod/quiz/classes/quiz_attempt.php +++ b/mod/quiz/classes/quiz_attempt.php @@ -23,6 +23,7 @@ use coding_exception; use context_module; use Exception; use html_writer; +use mod_quiz\hook\attempt_state_changed; use mod_quiz\output\links_to_other_attempts; use mod_quiz\output\renderer; use mod_quiz\question\bank\qbank_helper; @@ -1763,6 +1764,8 @@ class quiz_attempt { question_engine::save_questions_usage_by_activity($this->quba); + $originalattempt = clone $this->attempt; + $this->attempt->timemodified = $timestamp; $this->attempt->timefinish = $timefinish ?? $timestamp; $this->attempt->sumgrades = $this->quba->get_total_mark(); @@ -1784,6 +1787,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)); // Tell any access rules that care that the attempt is over. $this->get_access_manager($timestamp)->current_attempt_finished(); } @@ -1820,6 +1824,7 @@ class quiz_attempt { public function process_going_overdue($timestamp, $studentisonline) { global $DB; + $originalattempt = clone $this->attempt; $transaction = $DB->start_delegated_transaction(); $this->attempt->timemodified = $timestamp; $this->attempt->state = self::OVERDUE; @@ -1830,6 +1835,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)); $transaction->allow_commit(); quiz_send_overdue_message($this); @@ -1844,6 +1850,7 @@ class quiz_attempt { public function process_abandon($timestamp, $studentisonline) { global $DB; + $originalattempt = clone $this->attempt; $transaction = $DB->start_delegated_transaction(); $this->attempt->timemodified = $timestamp; $this->attempt->state = self::ABANDONED; @@ -1852,6 +1859,8 @@ 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)); + $transaction->allow_commit(); } @@ -1872,6 +1881,7 @@ class quiz_attempt { throw new coding_exception('Can only reopen an attempt that was never submitted.'); } + $originalattempt = clone $this->attempt; $transaction = $DB->start_delegated_transaction(); $this->attempt->timemodified = $timestamp; $this->attempt->state = self::IN_PROGRESS; @@ -1880,6 +1890,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)); $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 aa5167d68af..eb2b00b7c71 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -39,6 +39,7 @@ use core_question\local\bank\condition; use mod_quiz\access_manager; use mod_quiz\event\attempt_submitted; use mod_quiz\grade_calculator; +use mod_quiz\hook\attempt_state_changed; use mod_quiz\question\bank\qbank_helper; use mod_quiz\question\display_options; use mod_quiz\quiz_attempt; @@ -145,6 +146,8 @@ function quiz_create_attempt(quiz_settings $quizobj, $attemptnumber, $lastattemp $attempt->timecheckstate = $timeclose; } + \core\hook\manager::get_instance()->dispatch(new attempt_state_changed(null, $attempt)); + return $attempt; } /** @@ -448,10 +451,14 @@ function quiz_delete_attempt($attempt, $quiz) { $event->add_record_snapshot('quiz_attempts', $attempt); $event->trigger(); + // This class callback is deprecated, and will be removed in Moodle 4.8 (MDL-80327). + // Use the attempt_state_changed hook instead. $callbackclasses = \core_component::get_plugin_list_with_class('quiz', 'quiz_attempt_deleted'); foreach ($callbackclasses as $callbackclass) { - component_class_callback($callbackclass, 'callback', [$quiz->id]); + component_class_callback($callbackclass, 'callback', [$quiz->id], null, true); } + + \core\hook\manager::get_instance()->dispatch(new attempt_state_changed($attempt, null)); } // Search quiz_attempts for other instances by this user. diff --git a/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php b/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php index b83ff8a7ef9..8d058feb23c 100644 --- a/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php +++ b/mod/quiz/report/statistics/classes/event/observer/attempt_submitted.php @@ -16,6 +16,7 @@ namespace quiz_statistics\event\observer; +use core\check\performance\debugging; use quiz_statistics\task\recalculate; /** @@ -25,6 +26,8 @@ use quiz_statistics\task\recalculate; * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} * @author Mark Johnson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @deprecated Since Moodle 4.4 MDL-80099. + * @todo Final deprecation in Moodle 4.8 MDL-80956. */ class attempt_submitted { /** @@ -35,8 +38,11 @@ class attempt_submitted { * * @param \mod_quiz\event\attempt_submitted $event * @return void + * @deprecated Since Moodle 4.4 MDL-80099 */ public static function process(\mod_quiz\event\attempt_submitted $event): void { + debugging('quiz_statistics\event\observer\attempt_submitted event observer has been deprecated in favour of ' . + 'the quiz_statistics\hook_callbacks::quiz_attempt_submitted_or_deleted hook callback.', DEBUG_DEVELOPER); $data = $event->get_data(); recalculate::queue_future_run($data['other']['quizid']); } diff --git a/mod/quiz/report/statistics/classes/quiz_structure_modified.php b/mod/quiz/report/statistics/classes/hook_callbacks.php similarity index 55% rename from mod/quiz/report/statistics/classes/quiz_structure_modified.php rename to mod/quiz/report/statistics/classes/hook_callbacks.php index 1cd8b52e8c9..da441d415f7 100644 --- a/mod/quiz/report/statistics/classes/quiz_structure_modified.php +++ b/mod/quiz/report/statistics/classes/hook_callbacks.php @@ -13,34 +13,34 @@ // // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . - namespace quiz_statistics; use core\dml\sql_join; +use mod_quiz\hook\attempt_state_changed; +use mod_quiz\hook\structure_modified; +use mod_quiz\quiz_attempt; +use quiz_statistics\task\recalculate; /** - * Clear the statistics cache when the quiz structure is modified. + * Hook callbacks * * @package quiz_statistics * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} * @author Mark Johnson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class quiz_structure_modified { +class hook_callbacks { /** - * Clear the statistics cache. + * Clear the statistics cache for the quiz where the structure was modified. * - * @param int $quizid The quiz to clear the cache for. + * @param structure_modified $hook The structure_modified hook containing the new structure. * @return void */ - public static function callback(int $quizid): void { - global $DB, $CFG; + public static function quiz_structure_modified(structure_modified $hook) { + global $CFG; require_once($CFG->dirroot . '/mod/quiz/report/statistics/statisticslib.php'); require_once($CFG->dirroot . '/mod/quiz/report/statistics/report.php'); - $quiz = $DB->get_record('quiz', ['id' => $quizid]); - if (!$quiz) { - throw new \coding_exception('Could not find quiz with ID ' . $quizid . '.'); - } + $quiz = $hook->get_structure()->get_quiz(); $qubaids = quiz_statistics_qubaids_condition( $quiz->id, new sql_join(), @@ -50,4 +50,20 @@ class quiz_structure_modified { $report = new \quiz_statistics_report(); $report->clear_cached_data($qubaids); } + + /** + * Queue a statistics recalculation when an attempt is submitted or deleting. + * + * @param attempt_state_changed $hook + * @return bool True if a task was queued. + */ + public static function quiz_attempt_submitted_or_deleted(attempt_state_changed $hook): bool { + $originalattempt = $hook->get_original_attempt(); + $updatedattempt = $hook->get_updated_attempt(); + if (is_null($updatedattempt) || $updatedattempt->state === quiz_attempt::FINISHED) { + // Only recalculate on deletion or submission. + return recalculate::queue_future_run($originalattempt->quiz); + } + return false; + } } diff --git a/mod/quiz/report/statistics/classes/quiz_attempt_deleted.php b/mod/quiz/report/statistics/classes/quiz_attempt_deleted.php index 8c6dccda6c7..011842e9025 100644 --- a/mod/quiz/report/statistics/classes/quiz_attempt_deleted.php +++ b/mod/quiz/report/statistics/classes/quiz_attempt_deleted.php @@ -25,6 +25,8 @@ use quiz_statistics\task\recalculate; * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} * @author Mark Johnson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @deprecated Since Moodle 4.4 MDL-80099. + * @todo Final deprecation in Moodle 4.8 MDL-80956. */ class quiz_attempt_deleted { /** @@ -32,8 +34,11 @@ class quiz_attempt_deleted { * * @param int $quizid The quiz the attempt belongs to. * @return void + * @deprecated Since Moodle 4.4 MDL-80099. */ public static function callback(int $quizid): void { + debugging('quiz_statistics\quiz_attempt_deleted callback class has been deprecated in favour of ' . + 'the quiz_statistics\hook_callbacks::quiz_attempt_submitted_or_deleted hook callback.', DEBUG_DEVELOPER); recalculate::queue_future_run($quizid); } } diff --git a/mod/quiz/report/statistics/db/hooks.php b/mod/quiz/report/statistics/db/hooks.php new file mode 100644 index 00000000000..dae66ed389b --- /dev/null +++ b/mod/quiz/report/statistics/db/hooks.php @@ -0,0 +1,37 @@ +. +/** + * Hook callback definitions for quiz_statistics + * + * @package quiz_statistics + * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +defined('MOODLE_INTERNAL') || die(); + +$callbacks = [ + [ + 'hook' => mod_quiz\hook\structure_modified::class, + 'callback' => quiz_statistics\hook_callbacks::class . '::quiz_structure_modified', + 'priority' => 500, + ], + [ + 'hook' => mod_quiz\hook\attempt_state_changed::class, + 'callback' => quiz_statistics\hook_callbacks::class . '::quiz_attempt_submitted_or_deleted', + 'priority' => 500, + ], +]; diff --git a/mod/quiz/report/statistics/tests/quiz_attempt_deleted_test.php b/mod/quiz/report/statistics/tests/quiz_attempt_deleted_test.php index 6b1ca956e3a..47fa441db1c 100644 --- a/mod/quiz/report/statistics/tests/quiz_attempt_deleted_test.php +++ b/mod/quiz/report/statistics/tests/quiz_attempt_deleted_test.php @@ -32,7 +32,7 @@ use quiz_statistics\tests\statistics_test_trait; * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} * @author Mark Johnson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - * @covers \quiz_statistics\quiz_attempt_deleted + * @covers \quiz_statistics\hook_callbacks::quiz_attempt_submitted_or_deleted */ class quiz_attempt_deleted_test extends \advanced_testcase { use \quiz_question_helper_test_trait; diff --git a/mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php b/mod/quiz/report/statistics/tests/quiz_attempt_submitted_test.php similarity index 96% rename from mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php rename to mod/quiz/report/statistics/tests/quiz_attempt_submitted_test.php index 796f5f08edf..8e8592c8529 100644 --- a/mod/quiz/report/statistics/tests/event/observer/attempt_submitted_test.php +++ b/mod/quiz/report/statistics/tests/quiz_attempt_submitted_test.php @@ -13,7 +13,7 @@ // // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -namespace quiz_statistics\event\observer; +namespace quiz_statistics; defined('MOODLE_INTERNAL') || die(); @@ -32,13 +32,12 @@ use quiz_statistics\tests\statistics_test_trait; * @copyright 2023 onwards Catalyst IT EU {@link https://catalyst-eu.net} * @author Mark Johnson * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - * @covers \quiz_statistics\event\observer\attempt_submitted + * @covers \quiz_statistics\hook_callbacks::quiz_attempt_submitted_or_deleted */ -class attempt_submitted_test extends \advanced_testcase { +class quiz_attempt_submitted_test extends \advanced_testcase { use \quiz_question_helper_test_trait; use statistics_test_trait; - /** * Attempting a quiz should queue the recalculation task for that quiz in 1 hour's time. * diff --git a/mod/quiz/report/statistics/version.php b/mod/quiz/report/statistics/version.php index 25a1f70a18d..23228b92f76 100644 --- a/mod/quiz/report/statistics/version.php +++ b/mod/quiz/report/statistics/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2023100900; +$plugin->version = 2023100901; $plugin->requires = 2023100400; $plugin->component = 'quiz_statistics';