From e798fa76f5833fbe45e3710bba70dc9419b964ca Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Mon, 13 Mar 2017 20:10:17 +0800 Subject: [PATCH] MDL-58110 core_calendar: Add proxy for modules Modules associated with an event are stored in the event table as the module's name and instance number not the actual ID of the instance in the modules table. So to lazy load them we need a proxy that uses the module name and instance rather than the ID. Part of MDL-55611 epic. --- calendar/classes/api.php | 15 +- .../classes/local/event/core_container.php | 11 +- .../factories/event_abstract_factory.php | 28 ++- .../local/event/proxies/module_std_proxy.php | 58 +++++ .../classes/local/event/proxies/std_proxy.php | 14 +- calendar/tests/event_factory_test.php | 73 +++++- calendar/tests/module_std_proxy_test.php | 216 ++++++++++++++++++ lib/deprecatedlib.php | 11 +- 8 files changed, 394 insertions(+), 32 deletions(-) create mode 100644 calendar/classes/local/event/proxies/module_std_proxy.php create mode 100644 calendar/tests/module_std_proxy_test.php diff --git a/calendar/classes/api.php b/calendar/classes/api.php index 13302644fbb40..846f8bde670c0 100644 --- a/calendar/classes/api.php +++ b/calendar/classes/api.php @@ -562,22 +562,17 @@ public static function get_courselink($courseid) { /** * Get current module cache. * - * @param array $coursecache list of course cache + * @param array $modulecache in memory module cache * @param string $modulename name of the module * @param int $instance module instance number * @return \stdClass|bool $module information */ - public static function get_module_cached(&$coursecache, $modulename, $instance) { - $module = get_coursemodule_from_instance($modulename, $instance); - - if ($module === false) { - return false; + public static function get_module_cached(&$modulecache, $modulename, $instance) { + if (!isset($modulecache[$modulename . '_' . $instance])) { + $modulecache[$modulename . '_' . $instance] = get_coursemodule_from_instance($modulename, $instance); } - if (!self::get_course_cached($coursecache, $module->course)) { - return false; - } - return $module; + return $modulecache[$modulename . '_' . $instance]; } /** diff --git a/calendar/classes/local/event/core_container.php b/calendar/classes/local/event/core_container.php index c83bba2d8959e..ead0a8f083e14 100644 --- a/calendar/classes/local/event/core_container.php +++ b/calendar/classes/local/event/core_container.php @@ -80,6 +80,11 @@ class core_container { */ protected static $coursecache = array(); + /** + * @var stdClass[] An array of cached modules to use with the event factory. + */ + protected static $modulecache = array(); + /** * Initialises the dependency graph if it hasn't yet been. */ @@ -103,7 +108,8 @@ function() { function() { return false; }, - self::$coursecache + self::$coursecache, + self::$modulecache ) ); @@ -121,7 +127,8 @@ function ($dbrow) { return !(bool)$cm->visible; }, - self::$coursecache + self::$coursecache, + self::$modulecache ); } diff --git a/calendar/classes/local/event/factories/event_abstract_factory.php b/calendar/classes/local/event/factories/event_abstract_factory.php index 06ee4b44b1099..946bf3ae6935c 100644 --- a/calendar/classes/local/event/factories/event_abstract_factory.php +++ b/calendar/classes/local/event/factories/event_abstract_factory.php @@ -29,6 +29,7 @@ use core_calendar\local\event\entities\event; use core_calendar\local\event\entities\repeat_event_collection; use core_calendar\local\event\exceptions\invalid_callback_exception; +use core_calendar\local\event\proxies\module_std_proxy; use core_calendar\local\event\proxies\std_proxy; use core_calendar\local\event\value_objects\event_description; use core_calendar\local\event\value_objects\event_times; @@ -58,6 +59,11 @@ abstract class event_abstract_factory implements event_factory_interface { */ protected $coursecachereference; + /** + * @var array Module cache reference for use with get_module_cached. + */ + protected $modulecachereference; + /** * @var callable Bail out check for create_instance. */ @@ -91,12 +97,14 @@ public function __construct( callable $actioncallbackapplier, callable $visibilitycallbackapplier, callable $bailoutcheck, - array &$coursecachereference + array &$coursecachereference, + array &$modulecachereference ) { $this->actioncallbackapplier = $actioncallbackapplier; $this->visibilitycallbackapplier = $visibilitycallbackapplier; $this->bailoutcheck = $bailoutcheck; $this->coursecachereference = &$coursecachereference; + $this->modulecachereference = &$modulecachereference; } public function create_instance(\stdClass $dbrow) { @@ -143,13 +151,17 @@ public function create_instance(\stdClass $dbrow) { if ($dbrow->instance && $dbrow->modulename) { $modulename = $dbrow->modulename; - $module = new std_proxy($dbrow->instance, function($id) use ($modulename) { - return get_coursemodule_from_instance($modulename, $id); - }, - (object)[ - 'modname' => $modulename, - 'instance' => $dbrow->instance - ]); + $module = new module_std_proxy( + $dbrow->modulename, + $dbrow->instance, + function($modulename, $instance) { + return \core_calendar\api::get_module_cached( + $this->modulecachereference, + $modulename, + $instance + ); + } + ); } if ($dbrow->subscriptionid) { diff --git a/calendar/classes/local/event/proxies/module_std_proxy.php b/calendar/classes/local/event/proxies/module_std_proxy.php new file mode 100644 index 0000000000000..0781a19980f46 --- /dev/null +++ b/calendar/classes/local/event/proxies/module_std_proxy.php @@ -0,0 +1,58 @@ +. + +/** + * Course module stdClass proxy. + * + * @package core_calendar + * @copyright 2017 Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core_calendar\local\event\proxies; + +defined('MOODLE_INTERNAL') || die(); + +use core_calendar\local\interfaces\proxy_interface; +use core_calendar\local\event\exceptions\member_does_not_exist_exception; + +/** + * Course module stdClass proxy. + * + * This implementation differs from the regular std_proxy in that it takes + * a module name and instance instead of an id to construct the proxied class. + * + * This is needed as the event table does not store the id of course modules + * instead it stores the module name and instance. + * + * @copyright 2017 Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class module_std_proxy extends std_proxy implements proxy_interface { + public function __construct($modulename, $instance, callable $callback, \stdClass $base = null) { + $this->modulename = $modulename; + $this->instance = $instance; + $this->callbackargs = [$modulename, $instance]; + $this->callback = $callback; + $this->base = $base = is_null($base) ? new \stdClass() : $base; + $this->base->modulename = $modulename; + $this->base->instance = $instance; + } + + public function get_id() { + return $this->get_proxied_instance()->id; + } +} diff --git a/calendar/classes/local/event/proxies/std_proxy.php b/calendar/classes/local/event/proxies/std_proxy.php index 309bf11fda0a2..5b0ac8d498d63 100644 --- a/calendar/classes/local/event/proxies/std_proxy.php +++ b/calendar/classes/local/event/proxies/std_proxy.php @@ -54,6 +54,11 @@ class std_proxy implements proxy_interface { */ protected $callback; + /** + * @var array $callbackargs Array of arguments to pass to the callback. + */ + protected $callbackargs; + /** * @var \stdClass $base Base class to get members from. */ @@ -69,6 +74,7 @@ class std_proxy implements proxy_interface { */ public function __construct($id, callable $callback, \stdClass $base = null) { $this->id = $id; + $this->callbackargs = [$id]; $this->callback = $callback; $this->base = $base; } @@ -102,11 +108,7 @@ public function set($member, $value) { } public function get_proxied_instance() { - if ($this->class) { - return $this->class; - } else { - $callback = $this->callback; - return $callback($this->id); - } + $callback = $this->callback; + return $this->class = $this->class ? $this->class : $callback(...$this->callbackargs); } } diff --git a/calendar/tests/event_factory_test.php b/calendar/tests/event_factory_test.php index 990c85d003f83..e8865dcaefcf0 100644 --- a/calendar/tests/event_factory_test.php +++ b/calendar/tests/event_factory_test.php @@ -57,11 +57,13 @@ public function test_create_instance( $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( $actioncallbackapplier, $visibilitycallbackapplier, $bailoutcheck, - $coursecache + $coursecache, + $modulecache ); $dbrow->id = $event->id; $instance = $factory->create_instance($dbrow); @@ -89,6 +91,7 @@ public function test_invalid_action_callback() { $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( function () { return 'hello'; @@ -99,7 +102,8 @@ function () { function () { return false; }, - $coursecache + $coursecache, + $modulecache ); $factory->create_instance( @@ -135,6 +139,7 @@ public function test_invalid_visibility_callback() { $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( function ($event) { return $event; @@ -145,7 +150,8 @@ function () { function () { return false; }, - $coursecache + $coursecache, + $modulecache ); $factory->create_instance( @@ -181,6 +187,7 @@ public function test_invalid_bail_callback() { $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( function ($event) { return $event; @@ -191,7 +198,8 @@ function () { function () { return 'asdf'; }, - $coursecache + $coursecache, + $modulecache ); $factory->create_instance( @@ -226,6 +234,7 @@ public function test_course_cache() { $course = self::getDataGenerator()->create_course(); $event = $this->create_event(['courseid' => $course->id]); $coursecache = []; + $modulecache = []; $factory = new event_factory( function ($event) { return $event; @@ -236,7 +245,8 @@ function () { function () { return false; }, - $coursecache + $coursecache, + $modulecache ); $instance = $factory->create_instance( @@ -265,6 +275,59 @@ function () { $this->assertArrayHasKey($course->id, $coursecache); } + /** + * Test the factory's module cache. + */ + public function test_module_cache() { + $this->resetAfterTest(true); + $this->setAdminUser(); + $course = self::getDataGenerator()->create_course(); + $event = $this->create_event(['courseid' => $course->id]); + $plugingenerator = $this->getDataGenerator()->get_plugin_generator('mod_assign'); + $assigninstance = $plugingenerator->create_instance(['course' => $course->id]); + + $coursecache = []; + $modulecache = []; + $factory = new event_factory( + function ($event) { + return $event; + }, + function () { + return true; + }, + function () { + return false; + }, + $coursecache, + $modulecache + ); + + $instance = $factory->create_instance( + (object)[ + 'id' => $event->id, + 'name' => 'test', + 'description' => 'Test description', + 'format' => 2, + 'courseid' => $course->id, + 'groupid' => 1, + 'userid' => 1, + 'repeatid' => 1, + 'modulename' => 'assign', + 'instance' => $assigninstance->id, + 'eventtype' => 'due', + 'timestart' => 123456789, + 'timeduration' => 12, + 'timemodified' => 123456789, + 'timesort' => 123456789, + 'visible' => 1, + 'subscriptionid' => 1 + ] + ); + + $instance->get_course_module()->get('course'); + $this->assertArrayHasKey('assign' . '_' . $assigninstance->id, $modulecache); + } + /** * Testcases for the create instance test. * diff --git a/calendar/tests/module_std_proxy_test.php b/calendar/tests/module_std_proxy_test.php new file mode 100644 index 0000000000000..090c3bf94b59f --- /dev/null +++ b/calendar/tests/module_std_proxy_test.php @@ -0,0 +1,216 @@ +. + +/** + * module_std_proxy tests. + * + * @package core_calendar + * @copyright 2017 Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +use core_calendar\local\event\proxies\module_std_proxy; + +/** + * std_proxy testcase. + * + * @copyright 2017 Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class core_calendar_module_std_proxy_testcase extends advanced_testcase { + /** + * @var \stdClass[] $objects Array of objects to proxy. + */ + public $objects; + + public function setUp() { + $this->objects = [ + 'somemodule_someinstance' => (object) [ + 'member1' => 'Hello', + 'member2' => 1729, + 'member3' => 'Something else' + ], + 'anothermodule_anotherinstance' => (object) [ + 'member1' => 'Hej', + 'member2' => 87539319, + 'member3' => 'nagot annat' + ] + ]; + } + + /** + * Test proxying. + * + * @dataProvider test_proxy_testcases() + * @param string $modulename Object module name. + * @param string $instance Object instance. + * @param string $member Object member to retrieve. + * @param mixed $expected Expected value of member. + */ + public function test_proxy($modulename, $instance, $member, $expected) { + $proxy = new module_std_proxy( + $modulename, + $instance, + function($modulename, $instance) { + return $this->objects[$modulename . '_' . $instance]; + } + ); + + $this->assertEquals($proxy->get($member), $expected); + + // Test changing the value. + $proxy->set($member, 'something even more else'); + $this->assertEquals($proxy->get($member), 'something even more else'); + } + + /** + * Test setting values with a base class. + * + * @dataProvider test_proxy_testcases() + * @param string $modulename Object module name. + * @param string $instance Object instance. + * @param string $member Object member to retrieve. + * @param mixed $storedvalue Value as would be stored externally. + */ + public function test_base_values($modulename, $instance, $member, $storedvalue) { + $proxy = new module_std_proxy( + $modulename, + $instance, + function($module, $instance) { + return $this->objects[$module . '_' . $instance]; + }, + (object)['member1' => 'should clobber 1'] + ); + + $expected = $member == 'member1' ? 'should clobber 1' : $storedvalue; + $this->assertEquals($proxy->get($member), $expected); + } + + /** + * Test getting a non existant member. + * + * @dataProvider test_get_set_testcases() + * @expectedException \core_calendar\local\event\exceptions\member_does_not_exist_exception + * @param string $modulename Object module name. + * @param string $instance Object instance. + */ + public function test_get_invalid_member($modulename, $instance) { + $proxy = new module_std_proxy( + $modulename, + $instance, + function($modulename, $instance) { + return $this->objects[$modulename . '_' . $instance]; + } + ); + + $proxy->get('thisdoesnotexist'); + } + + /** + * Test setting a non existant member. + * + * @dataProvider test_get_set_testcases() + * @expectedException \core_calendar\local\event\exceptions\member_does_not_exist_exception + * @param string $modulename Object module name. + * @param string $instance Object instance. + */ + public function test_set_invalid_member($modulename, $instance) { + $proxy = new module_std_proxy( + $modulename, + $instance, + function($modulename, $instance) { + return $this->objects[$modulename . '_' . $instance]; + } + ); + + $proxy->set('thisdoesnotexist', 'should break'); + } + + /** + * Test get proxied instance. + * + * @dataProvider test_get_set_testcases() + * @param string $modulename Object module name. + * @param string $instance Object instance. + */ + public function test_get_proxied_instance($modulename, $instance) { + $proxy = new module_std_proxy( + $modulename, + $instance, + function($modulename, $instance) { + return $this->objects[$modulename . '_' . $instance]; + } + ); + + $this->assertEquals($proxy->get_proxied_instance(), $this->objects[$modulename . '_' . $instance]); + } + + /** + * Test cases for proxying test. + */ + public function test_proxy_testcases() { + return [ + 'Object 1 member 1' => [ + 'somemodule', + 'someinstance', + 'member1', + 'Hello' + ], + 'Object 1 member 2' => [ + 'somemodule', + 'someinstance', + 'member2', + 1729 + ], + 'Object 1 member 3' => [ + 'somemodule', + 'someinstance', + 'member3', + 'Something else' + ], + 'Object 2 member 1' => [ + 'anothermodule', + 'anotherinstance', + 'member1', + 'Hej' + ], + 'Object 2 member 2' => [ + 'anothermodule', + 'anotherinstance', + 'member2', + 87539319 + ], + 'Object 3 member 3' => [ + 'anothermodule', + 'anotherinstance', + 'member3', + 'nagot annat' + ] + ]; + } + + /** + * Test cases for getting and setting tests. + */ + public function test_get_set_testcases() { + return [ + 'Object 1' => ['somemodule', 'someinstance'], + 'Object 2' => ['anothermodule', 'anotherinstance'] + ]; + } +} diff --git a/lib/deprecatedlib.php b/lib/deprecatedlib.php index 0bf21ad764164..123fd0a6fc4a0 100644 --- a/lib/deprecatedlib.php +++ b/lib/deprecatedlib.php @@ -6671,7 +6671,16 @@ function calendar_get_courselink($courseid) { function calendar_get_module_cached(&$coursecache, $modulename, $instance) { debugging(__FUNCTION__ . '() is deprecated, please use \core_calendar\api::get_module_cached() instead.', DEBUG_DEVELOPER); - return \core_calendar\api::get_module_cached($coursecache, $modulename, $instance); + + // We have a new implementation of this function in the calendar API class, + // so the old implementation must remain here. + $module = get_coursemodule_from_instance($modulename, $instance); + + if($module === false) return false; + if(!calendar_get_course_cached($coursecache, $module->course)) { + return false; + } + return $module; } /**