From fddd1018f600a6826ab32f387e39f473a25fa220 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20S=CC=8Ckoda?= Date: Mon, 15 Jul 2013 11:37:00 +0200 Subject: [PATCH] MDL-39846 improve validation and enable hardcoding of system context --- lib/classes/event/base.php | 44 +++++++++++++++++++++------ lib/tests/event_test.php | 22 ++++++++++++++ lib/tests/fixtures/event_fixtures.php | 30 ++++++++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) diff --git a/lib/classes/event/base.php b/lib/classes/event/base.php index d523309795d..0f4dabd95db 100644 --- a/lib/classes/event/base.php +++ b/lib/classes/event/base.php @@ -136,6 +136,9 @@ abstract class base implements \IteratorAggregate { $event->data['object'] = substr($parts[2], 0, $pos); $event->data['action'] = substr($parts[2], $pos+1); + // Set static event data specific for child class. + $event->init(); + // Set optional data or use defaults. $event->data['objectid'] = isset($data['objectid']) ? $data['objectid'] : null; $event->data['courseid'] = isset($data['courseid']) ? $data['courseid'] : null; @@ -143,8 +146,19 @@ abstract class base implements \IteratorAggregate { $event->data['other'] = isset($data['other']) ? $data['other'] : null; $event->data['relateduserid'] = isset($data['relateduserid']) ? $data['relateduserid'] : null; - if (!empty($data['context'])) { + if (isset($event->context)) { + if (isset($data['context']) and $event->context->id != $data['context']->id) { + throw new \coding_exception('Given context does not match expected context'); + } + $event->data['contextid'] = $event->context->id; + $event->data['contextlevel'] = $event->context->contextlevel; + $event->data['contextinstanceid'] = $event->context->instanceid; + + } else if (!empty($data['context'])) { $event->context = $data['context']; + if (isset($event->data['contextlevel']) and $event->data['contextlevel'] != $event->context->contextlevel) { + throw new \coding_exception('Context does not match expected context level'); + } $event->data['contextid'] = $event->context->id; $event->data['contextlevel'] = $event->context->contextlevel; $event->data['contextinstanceid'] = $event->context->instanceid; @@ -164,7 +178,7 @@ abstract class base implements \IteratorAggregate { $event->data['contextinstanceid'] = $event->context->instanceid; } } else { - throw new \coding_exception('context (or contextid) is a required event property.'); + throw new \coding_exception('context (or contextid) is a required event property, system context may be hardcoded in init() method.'); } if (!isset($event->data['courseid'])) { @@ -179,9 +193,6 @@ abstract class base implements \IteratorAggregate { $event->data['relateduserid'] = $event->context->instanceid; } - // Set static event data specific for child class, this should also validate other data. - $event->init(); - // Warn developers if they do something wrong. if (debugging('', DEBUG_DEVELOPER)) { // This should be replaced by new $CFG->slowdebug flag if introduced. static $automatickeys = array('eventname', 'component', 'action', 'object', 'timecreated'); @@ -203,6 +214,9 @@ abstract class base implements \IteratorAggregate { } } + // Let developers validate their custom data (such as $this->date['other']). + $event->validate_data(); + return $event; } @@ -210,18 +224,28 @@ abstract class base implements \IteratorAggregate { * Override in subclass. * * Set all required data properties: - * 1/ crud - letter [crud] - * 2/ level - number 1...100 + * 1/ crud - letter [crud] TODO: MDL-37658 + * 2/ level - number 1...100 TODO: MDL-37658 * 3/ objecttable - name of database table if objectid specified * - * TODO: MDL-37658 - * - * Optionally validate $this->data['other']. + * Optionally it can set: + * a/ fixed system context + * b/ required context level * * @return void */ protected abstract function init(); + /** + * Let developers validate their custom data (such as $this->date['other']). + * + * Throw \coding_exception or debugging() notice in case of any problems. + */ + protected function validate_data() { + // Override if you want to validate event properties when + // creating new events. + } + /** * Returns localised general event name. * diff --git a/lib/tests/event_test.php b/lib/tests/event_test.php index 9f7a94f1cee..8a211d59ea2 100644 --- a/lib/tests/event_test.php +++ b/lib/tests/event_test.php @@ -622,6 +622,28 @@ class core_event_testcase extends advanced_testcase { $this->assertDebuggingNotCalled(); $event6->trigger(); $this->assertDebuggingCalled(); + + $event = \core_tests\event\problematic_event2::create(array('context'=>\context_course::instance(1))); + try { + $event = \core_tests\event\problematic_event2::create(array('context'=>\context_system::instance())); + $this->fail('Exception expected when $data contains context that does not match hardcoded level'); + } catch (\moodle_exception $e) { + $this->assertInstanceOf('\coding_exception', $e); + } + + $event = \core_tests\event\problematic_event3::create(array()); + $event = \core_tests\event\problematic_event3::create(array('context'=>\context_system::instance())); + try { + $event = \core_tests\event\problematic_event3::create(array('context'=>\context_course::instance(1))); + $this->fail('Exception expected when $data contains matching or no context if hardcoded in init'); + } catch (\moodle_exception $e) { + $this->assertInstanceOf('\coding_exception', $e); + } + + $event = \core_tests\event\problematic_event4::create(array('other'=>1)); + $this->assertDebuggingNotCalled(); + $event = \core_tests\event\problematic_event4::create(array()); + $this->assertDebuggingCalled(); } public function test_record_snapshots() { diff --git a/lib/tests/fixtures/event_fixtures.php b/lib/tests/fixtures/event_fixtures.php index 368e9cafc38..29b99250586 100644 --- a/lib/tests/fixtures/event_fixtures.php +++ b/lib/tests/fixtures/event_fixtures.php @@ -162,3 +162,33 @@ class problematic_event1 extends \core\event\base { $this->data['level'] = 10; } } + +class problematic_event2 extends \core\event\base { + protected function init() { + $this->data['crud'] = 'c'; + $this->data['level'] = 10; + $this->data['contextlevel'] = CONTEXT_COURSE; + } +} + +class problematic_event3 extends \core\event\base { + protected function init() { + $this->data['crud'] = 'c'; + $this->data['level'] = 10; + $this->context = \context_system::instance(); + } +} + +class problematic_event4 extends \core\event\base { + protected function init() { + $this->data['crud'] = 'c'; + $this->data['level'] = 10; + $this->context = \context_system::instance(); + } + + protected function validate_data() { + if (empty($this->data['other'])) { + debugging('other is missing'); + } + } +}