diff --git a/calendar/classes/api.php b/calendar/classes/api.php index 13302644fbb..846f8bde670 100644 --- a/calendar/classes/api.php +++ b/calendar/classes/api.php @@ -562,22 +562,17 @@ class api { /** * 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 c83bba2d895..ead0a8f083e 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 @@ class core_container { function() { return false; }, - self::$coursecache + self::$coursecache, + self::$modulecache ) ); @@ -121,7 +127,8 @@ class core_container { 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 06ee4b44b10..946bf3ae693 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 @@ defined('MOODLE_INTERNAL') || die(); 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 @@ abstract class event_abstract_factory implements event_factory_interface { 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 @@ abstract class event_abstract_factory implements event_factory_interface { 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 00000000000..0781a19980f --- /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 309bf11fda0..5b0ac8d498d 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 @@ class std_proxy implements proxy_interface { } 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 990c85d003f..e8865dcaefc 100644 --- a/calendar/tests/event_factory_test.php +++ b/calendar/tests/event_factory_test.php @@ -57,11 +57,13 @@ class core_calendar_event_factory_testcase extends advanced_testcase { $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 @@ class core_calendar_event_factory_testcase extends advanced_testcase { $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( function () { return 'hello'; @@ -99,7 +102,8 @@ class core_calendar_event_factory_testcase extends advanced_testcase { function () { return false; }, - $coursecache + $coursecache, + $modulecache ); $factory->create_instance( @@ -135,6 +139,7 @@ class core_calendar_event_factory_testcase extends advanced_testcase { $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( function ($event) { return $event; @@ -145,7 +150,8 @@ class core_calendar_event_factory_testcase extends advanced_testcase { function () { return false; }, - $coursecache + $coursecache, + $modulecache ); $factory->create_instance( @@ -181,6 +187,7 @@ class core_calendar_event_factory_testcase extends advanced_testcase { $this->setAdminUser(); $event = $this->create_event(); $coursecache = []; + $modulecache = []; $factory = new event_factory( function ($event) { return $event; @@ -191,7 +198,8 @@ class core_calendar_event_factory_testcase extends advanced_testcase { function () { return 'asdf'; }, - $coursecache + $coursecache, + $modulecache ); $factory->create_instance( @@ -226,6 +234,7 @@ class core_calendar_event_factory_testcase extends advanced_testcase { $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 @@ class core_calendar_event_factory_testcase extends advanced_testcase { function () { return false; }, - $coursecache + $coursecache, + $modulecache ); $instance = $factory->create_instance( @@ -265,6 +275,59 @@ class core_calendar_event_factory_testcase extends advanced_testcase { $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 00000000000..090c3bf94b5 --- /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 0bf21ad7641..123fd0a6fc4 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; } /**