mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-18 05:58:34 +01:00
Code Issues Projects Releases Wiki Activity

MDL-39846 more fixing and cleanup of new events

Browse Source 
* fix typos (thanks Rajesh)
* move log related data out from event API specification
* change callable key in definition to callback
* use event data in observers instead of cached records if possible
* event level is now a number 1…100
* improved event safety checks
* add event name and description method
* new can_view() method
* improve unit tests to test all callable types
* improved performance
This commit is contained in:
Petr Škoda 2013-07-06 14:35:02 +02:00
parent e99a209094
commit 4b734e74ae
9 changed files with 227 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
enrol/category/classes/observer.php
View File

@ -44,7 +44,10 @@ class enrol_category_observer {
return;
}
$ra = $event->get_cached_record('role_assignments', $event->extra['id']);
$ra = new stdClass();
$ra->roleid = $event->objectid;
$ra->userid = $event->relateduserid;
$ra->contextid = $event->contextid;
//only category level roles are interesting
$parentcontext = context::instance_by_id($ra->contextid);
@ -83,7 +86,7 @@ class enrol_category_observer {
$params = array('courselevel'=>CONTEXT_COURSE, 'match'=>$parentcontext->path.'/%', 'userid'=>$ra->userid);
$rs = $DB->get_recordset_sql($sql, $params);
foreach ($rs as $instance) {
$plugin->enrol_user($instance, $ra->userid, null, $ra->timemodified);
$plugin->enrol_user($instance, $ra->userid, null, time());
}
$rs->close();
}
@ -100,7 +103,9 @@ class enrol_category_observer {
return;
}
$ra = $event->get_cached_record('role_assignments', $event->extra['id']);
$ra = new stdClass();
$ra->userid = $event->relateduserid;
$ra->contextid = $event->contextid;
// only category level roles are interesting
$parentcontext = context::instance_by_id($ra->contextid);

4
enrol/category/db/events.php
View File

@ -29,12 +29,12 @@ $observers = array (
array (
'eventname' => '\core\event\role_assigned',
'callable' => 'enrol_category_observer::role_assigned',
'callback' => 'enrol_category_observer::role_assigned',
),
array (
'eventname' => '\core\event\role_unassigned',
'callable' => 'enrol_category_observer::role_unassigned',
'callback' => 'enrol_category_observer::role_unassigned',
),
);

2
lib/accesslib.php
View File

@ -6913,7 +6913,7 @@ class context_module extends context {
* Is this context part of any course? If yes return course context.
*
* @param bool $strict true means throw exception if not found, false means return false if not found
* @return course_context context of the enclosing course, null if not found or exception
* @return context_course context of the enclosing course, null if not found or exception
*/
public function get_course_context($strict = true) {
return $this->get_parent_context();

191
lib/classes/event/base.php
View File

@ -38,7 +38,7 @@ namespace core\event;
* @property-read string $object what/who was object of the action (usually similar to database table name)
* @property-read int $objectid optional id of the object
* @property-read string $crud letter indicating event type
* @property-read int $level log level
* @property-read int $level log level (number between 1 and 100)
* @property-read int $contextid
* @property-read int $contextlevel
* @property-read int $contextinstanceid
@ -46,14 +46,15 @@ namespace core\event;
* @property-read int $courseid
* @property-read int $relateduserid
* @property-read mixed $extra array or scalar, can not contain objects
* @property-read int $realuserid who really did this?
* @property-read string $origin
* @property-read int $timecreated
*/
abstract class base {
/** @var array event data */
protected $data;
/** @var array the format is standardised by logging API */
protected $logdata;
/** @var \context of this event */
protected $context;
@ -67,7 +68,7 @@ abstract class base {
private static $fields = array(
'eventname', 'component', 'action', 'object', 'objectid', 'crud', 'level', 'contextid',
'contextlevel', 'contextinstanceid', 'userid', 'courseid', 'relateduserid', 'extra',
'realuserid', 'origin', 'timecreated');
'timecreated');
/** @var array simple record cache */
protected $cachedrecords = array();
@ -103,6 +104,9 @@ abstract class base {
$event->triggered = false;
$event->restored = false;
// Set automatic data.
$event->data['timecreated'] = time();
$classname = get_class($event);
$parts = explode('\\', $classname);
if (count($parts) !== 3 or $parts[1] !== 'event') {
@ -118,32 +122,7 @@ abstract class base {
$event->data['object'] = substr($parts[2], 0, $pos);
$event->data['action'] = substr($parts[2], $pos+1);
// Do not let developers to something crazy.
if (debugging('', DEBUG_DEVELOPER)) {
$keys = array('eventname', 'component', 'action', 'object', 'realuserid', 'origin', 'timecreated');
foreach ($keys as $key) {
if (array_key_exists($key, $data)) {
debugging("Data key '$key' is no allowed in event \\core\\event\\base::create() method, it is set automatically.", DEBUG_DEVELOPER);
}
}
$keys = array('crud', 'level');
foreach ($keys as $key) {
if (array_key_exists($key, $data)) {
debugging("Data key '$key' is no allowed in event \\core\\event\\base::create() method, you need to set it in init method.", DEBUG_DEVELOPER);
}
}
}
unset($data['eventname']);
unset($data['component']);
unset($data['action']);
unset($data['object']);
unset($data['crud']);
unset($data['level']);
unset($data['realuserid']);
unset($data['origin']);
unset($data['timecreated']);
// Set optional data.
// Set optional data or use defaults.
$event->data['objectid'] = isset($data['objectid']) ? $data['objectid'] : null;
$event->data['courseid'] = isset($data['courseid']) ? $data['courseid'] : null;
$event->data['userid'] = isset($data['userid']) ? $data['userid'] : $USER->id;
@ -163,7 +142,6 @@ abstract class base {
if (!$event->context) {
$event->context = \context_system::instance();
}
unset($data['context']);
$event->data['contextid'] = $event->context->id;
$event->data['contextlevel'] = $event->context->contextlevel;
$event->data['contextinstanceid'] = $event->context->instanceid;
@ -180,25 +158,30 @@ abstract class base {
$event->data['relateduserid'] = $event->context->instanceid;
}
if (CLI_SCRIPT) {
$event->data['origin'] = 'cli';
} else if (AJAX_SCRIPT) {
$event->data['origin'] = 'ajax:'.getremoteaddr();
} else {
$event->data['origin'] = 'web:'.getremoteaddr();
// TODO: detect web services somehow, for now it is logged separately.
}
// Set static event data specific for child class, this should also validate extra data.
$event->init();
// Warn developers if they do something wrong.
if (debugging('', DEBUG_DEVELOPER)) {
foreach (array_keys($data) as $key) {
if (!in_array($key, self::$fields)) {
debugging("Unsupported event data field '$key' detected.");
static $automatickeys = array('eventname', 'component', 'action', 'object', 'timecreated');
static $initkeys = array('crud', 'level');
foreach ($data as $key => $ignored) {
if ($key === 'context') {
continue;
} else if (in_array($key, $automatickeys)) {
debugging("Data key '$key' is not allowed in \\core\\event\\base::create() method, it is set automatically");
} else if (in_array($key, $initkeys)) {
debugging("Data key '$key' is not allowed in \\core\\event\\base::create() method, you need to set it in init() method");
} else if (!in_array($key, self::$fields)) {
debugging("Data key '$key' does not exist in \\core\\event\\base");
}
}
}
$event->init();
return $event;
}
@ -206,20 +189,58 @@ abstract class base {
* Override in subclass.
*
* Set all required data properties:
* 1/ crud
* 2/ level
* 1/ crud - letter [crud]
* 2/ level - number 1...100
*
* Optionally validate $this->data['extra'].
*
* @return void
*/
protected abstract function init();
/**
* Returns localised general event name.
*
* Override in subclass, we can not make it static and abstract at the same time.
*
* @return string|\lang_string
*/
public static function get_name() {
// Override in subclass with real lang string.
$parts = explode('\\', __CLASS__);
if (count($parts) !== 3) {
return 'unknown event';
}
return $parts[0].': '.str_replace('_', ' ', $parts[2]);
}
/**
* Returns localised description of what happened.
*
* @return string|\lang_string
*/
public function get_description() {
return null;
}
/**
* Define whether a user can view the event or not.
*
* @param int|\stdClass $user_or_id ID of the user.
* @return bool True if the user can view the event, false otherwise.
*/
public function can_view($user_or_id = null) {
return is_siteadmin($user_or_id);
}
/**
* Restore event from existing historic data.
*
* @param array $data
* @param array $logdata the format is standardised by logging API
* @return bool|\core\event\base
*/
public static final function restore(array $data = null) {
public static final function restore(array $data, array $logdata) {
$classname = $data['eventname'];
$component = $data['component'];
$action = $data['action'];
@ -240,30 +261,27 @@ abstract class base {
$event->triggered = true;
$event->restored = true;
$event->logdata = $logdata;
foreach (self::$fields as $key) {
if (array_key_exists($key, $data)) {
$event->data[$key] = $data[$key];
} else {
if (!array_key_exists($key, $data)) {
debugging("Event restore data must contain key $key");
$event->data[$key] = null;
$data[$key] = null;
}
}
if (count($data) != count(self::$fields)) {
foreach ($data as $key => $value) {
if (!in_array($key, self::$fields)) {
debugging("Event restore data cannot contain key $key");
unset($data[$key]);
}
}
}
$event->data = $data;
return $event;
}
/**
* Returns localised event name.
*
* Note: override in child class.
*
* @return string
*/
public function get_name() {
return $this->data['eventname'];
}
/**
* Returns event context.
* @return \context
@ -278,6 +296,7 @@ abstract class base {
/**
* Returns relevant URL, override in subclasses.
* @return \moodle_url
*/
public function get_url() {
return null;
@ -286,14 +305,21 @@ abstract class base {
/**
* Return standardised event data as array.
*
* Useful especially for logging of events.
*
* @return array
*/
public function get_data() {
return $this->data;
}
/**
* Return auxiliary data that was stored in logs.
*
* @return array the format is standardised by logging API
*/
public function get_logdata() {
return $this->logdata;
}
/**
* Does this event replace legacy event?
*
@ -337,18 +363,33 @@ abstract class base {
}
if (debugging('', DEBUG_DEVELOPER)) {
if (!in_array($this->data['crud'], array('c', 'r', 'u', 'd'), true)) {
debugging("Invalid event crud value specified.", DEBUG_DEVELOPER);
}
// Ideally these should be coding exceptions, but we need to skip these for performance reasons
// on production servers.
if (self::$fields !== array_keys($this->data)) {
debugging('Number of event data fields must not be changed in event classes', DEBUG_DEVELOPER);
}
if (!in_array($this->data['crud'], array('c', 'r', 'u', 'd'), true)) {
debugging("Invalid event crud value specified.");
}
if (!is_number($this->data['level'])) {
debugging('Event property level must be a number');
}
if (self::$fields !== array_keys($this->data)) {
debugging('Number of event data fields must not be changed in event classes');
}
$encoded = json_encode($this->data['extra']);
if ($encoded === false or $this->data['extra'] !== json_decode($encoded, true)) {
debugging('Extra event data must be compatible with json encoding', DEBUG_DEVELOPER);
debugging('Extra event data must be compatible with json encoding');
}
if ($this->data['userid'] and !is_number($this->data['userid'])) {
debugging('Event property userid must be a number');
}
if ($this->data['courseid'] and !is_number($this->data['courseid'])) {
debugging('Event property courseid must be a number');
}
if ($this->data['objectid'] and !is_number($this->data['objectid'])) {
debugging('Event property objectid must be a number');
}
if ($this->data['relateduserid'] and !is_number($this->data['relateduserid'])) {
debugging('Event property relateduserid must be a number');
}
}
}
@ -395,7 +436,7 @@ abstract class base {
}
/**
* Was this evetn restored?
* Was this event restored?
*
* @return bool
*/
@ -407,7 +448,7 @@ abstract class base {
* Add cached data that will be most probably used in event observers.
*
* This is used to improve performance, but it is required for data
* thar was just deleted.
* that was just deleted.
*
* @param string $tablename
* @param \stdClass $record
@ -453,14 +494,12 @@ abstract class base {
*
* @param string $name
* @return mixed
*
* @throws \coding_exception
*/
public function __get($name) {
if (array_key_exists($name, $this->data)) {
return $this->data[$name];
}
throw new \coding_exception("Accessing non-existent property $name from event class");
debugging("Accessing non-existent event property '$name'");
}
}

6
lib/classes/event/manager.php
View File

@ -232,12 +232,12 @@ class manager {
if ($observer['eventname'] !== '*' and strpos($observer['eventname'], '\\') !== 0) {
$observer['eventname'] = '\\'.$observer['eventname'];
}
if (empty($observer['callable'])) {
debugging("Invalid 'callable' detected in $file observer definition", DEBUG_DEVELOPER);
if (empty($observer['callback'])) {
debugging("Invalid 'callback' detected in $file observer definition", DEBUG_DEVELOPER);
continue;
}
$o = new \stdClass();
$o->callable = $observer['callable'];
$o->callable = $observer['callback'];
if (!isset($observer['priority'])) {
$o->priority = 0;
} else {

30
lib/classes/event/role_assigned.php
View File

@ -27,7 +27,35 @@ namespace core\event;
class role_assigned extends base {
protected function init() {
$this->data['crud'] = 'c';
$this->data['level'] = 1; // TODO
$this->data['level'] = 50;
}
/**
* Returns localised general event name.
*
* @return string|\lang_string
*/
public static function get_name() {
//TODO: localise
return 'Role assigned';
}
/**
* Returns localised description of what happened.
*
* @return string|\lang_string
*/
public function get_description() {
//TODO: localise
return 'Role '.$this->objectid.' was assigned to user '.$this->relateduserid.' in context '.$this->contextid;
}
/**
* Returns relevant URL.
* @return \moodle_url
*/
public function get_url() {
return new moodle_url('/admin/roles/assign.php', array('contextid'=>$this->contextid, 'roleid'=>$this->objectid));
}
/**

30
lib/classes/event/role_unassigned.php
View File

@ -27,7 +27,35 @@ namespace core\event;
class role_unassigned extends base {
protected function init() {
$this->data['crud'] = 'd';
$this->data['level'] = 1; // TODO
$this->data['level'] = 50;
}
/**
* Returns localised general event name.
*
* @return string|\lang_string
*/
public static function get_name() {
//TODO: localise
return 'Role unassigned';
}
/**
* Returns localised description of what happened.
*
* @return string|\lang_string
*/
public function get_description() {
//TODO: localise
return 'Role '.$this->objectid.'was unassigned from user '.$this->relateduserid.' in context '.$this->contextid;
}
/**
* Returns relevant URL.
* @return \moodle_url
*/
public function get_url() {
return new moodle_url('/admin/roles/assign.php', array('contextid'=>$this->contextid, 'roleid'=>$this->objectid));
}
/**

62
lib/tests/event_test.php
View File

@ -34,24 +34,24 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
'includefile' => 'lib/tests/fixtures/event_fixtures.php',
),
array(
'eventname' => '*',
'callable' => '\core_tests\event\unittest_observer::observe_all',
'callback' => array('\core_tests\event\unittest_observer', 'observe_all'),
'includefile' => null,
'internal' => 1,
'priority' => 9999,
),
array(
'eventname' => '\core\event\unknown_executed',
'callable' => '\core_tests\event\unittest_observer::broken_observer',
'callback' => '\core_tests\event\unittest_observer::broken_observer',
'priority' => 100,
),
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::external_observer',
'callback' => '\core_tests\event\unittest_observer::external_observer',
'priority' => 200,
'internal' => 0,
),
@ -65,7 +65,7 @@ class core_event_testcase extends advanced_testcase {
$expected = array();
$observer = new stdClass();
$observer->callable = '\core_tests\event\unittest_observer::observe_all';
$observer->callable = array('\core_tests\event\unittest_observer', 'observe_all');
$observer->priority = 9999;
$observer->internal = true;
$observer->includefile = null;
@ -87,7 +87,7 @@ class core_event_testcase extends advanced_testcase {
$expected = array();
$observer = new stdClass();
$observer->callable = '\core_tests\event\unittest_observer::observe_all';
$observer->callable = array('\core_tests\event\unittest_observer', 'observe_all');
$observer->priority = 9999;
$observer->internal = true;
$observer->includefile = null;
@ -103,7 +103,7 @@ class core_event_testcase extends advanced_testcase {
$expected = array();
$observer = new stdClass();
$observer->callable = '\core_tests\event\unittest_observer::observe_all';
$observer->callable = array('\core_tests\event\unittest_observer', 'observe_all');
$observer->priority = 9999;
$observer->internal = true;
$observer->includefile = null;
@ -117,7 +117,7 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => 'core_tests\event\unittest_executed', // Fix leading backslash.
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
'includefile' => 'lib/tests/fixtures/event_fixtures.php',
'internal' => 1, // Cast to bool.
),
@ -136,7 +136,7 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
// Missing eventclass.
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
'includefile' => 'lib/tests/fixtures/event_fixtures.php',
),
);
@ -147,7 +147,7 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => '', // Empty eventclass.
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
'includefile' => 'lib/tests/fixtures/event_fixtures.php',
),
);
@ -169,7 +169,7 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '', // empty callable
'callback' => '', // empty callable
'includefile' => 'lib/tests/fixtures/event_fixtures.php',
),
);
@ -180,7 +180,7 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
'includefile' => 'lib/tests/fixtures/event_fixtures.php_xxx', // Missing file.
),
);
@ -193,11 +193,11 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
),
array(
'eventname' => '*',
'callable' => '\core_tests\event\unittest_observer::observe_all',
'callback' => '\core_tests\event\unittest_observer::observe_all',
'includefile' => null,
'internal' => 1,
'priority' => 9999,
@ -228,12 +228,12 @@ class core_event_testcase extends advanced_testcase {
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
),
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::broken_observer',
'callback' => '\core_tests\event\unittest_observer::broken_observer',
'priority' => 100,
),
);
@ -263,12 +263,12 @@ class core_event_testcase extends advanced_testcase {
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
),
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::external_observer',
'callback' => '\core_tests\event\unittest_observer::external_observer',
'priority' => 200,
'internal' => 0,
),
@ -335,11 +335,11 @@ class core_event_testcase extends advanced_testcase {
$observers = array(
array(
'eventname' => '\core_tests\event\unittest_executed',
'callable' => '\core_tests\event\unittest_observer::observe_one',
'callback' => '\core_tests\event\unittest_observer::observe_one',
),
array(
'eventname' => '*',
'callable' => '\core_tests\event\unittest_observer::observe_all',
'callback' => '\core_tests\event\unittest_observer::observe_all',
'includefile' => null,
'internal' => 1,
'priority' => 9999,
@ -396,7 +396,7 @@ class core_event_testcase extends advanced_testcase {
$event1 = \core_tests\event\unittest_executed::create(array('courseid'=>1, 'context'=>\context_system::instance(), 'extra'=>array('sample'=>1, 'xx'=>10)));
$data1 = $event1->get_data();
$event2 = \core\event\base::restore($data1);
$event2 = \core\event\base::restore($data1, array('origin'=>'clid'));
$data2 = $event2->get_data();
$this->assertTrue($event2->is_triggered());
@ -409,20 +409,20 @@ class core_event_testcase extends advanced_testcase {
// Now test problematic data.
$data3 = $data1;
$data3['eventname'] = '\\a\\b\\c';
$event3 = \core\event\base::restore($data3);
$event3 = \core\event\base::restore($data3, array());
$this->assertFalse($event3, 'Class name must match');
$data4 = $data1;
unset($data4['userid']);
$event4 = \core\event\base::restore($data4);
$event4 = \core\event\base::restore($data4, array());
$this->assertInstanceOf('core_tests\event\unittest_executed', $event4);
$this->assertDebuggingCalled();
$data5 = $data1;
$data5['xx'] = 'xx';
$event5 = \core\event\base::restore($data5);
$event5 = \core\event\base::restore($data5, array());
$this->assertInstanceOf('core_tests\event\unittest_executed', $event5);
$this->assertDebuggingNotCalled();
$this->assertDebuggingCalled();
}
@ -437,7 +437,7 @@ class core_event_testcase extends advanced_testcase {
}
$data = $event->get_data();
$restored = \core_tests\event\unittest_executed::restore($data);
$restored = \core_tests\event\unittest_executed::restore($data, array());
$this->assertTrue($restored->is_triggered());
$this->assertTrue($restored->is_restored());
@ -467,15 +467,15 @@ class core_event_testcase extends advanced_testcase {
}
$event = \core_tests\event\bad_event3::create();
$event->trigger();
@$event->trigger();
$this->assertDebuggingCalled();
$event = \core_tests\event\bad_event4::create();
$event->trigger();
@$event->trigger();
$this->assertDebuggingCalled();
$event = \core_tests\event\bad_event5::create();
$event->trigger();
@$event->trigger();
$this->assertDebuggingCalled();
}
@ -483,6 +483,8 @@ class core_event_testcase extends advanced_testcase {
global $CFG;
$event1 = \core_tests\event\problematic_event1::create();
$this->assertDebuggingNotCalled();
$this->assertNull($event1->xxx);
$this->assertDebuggingCalled();
$event2 = \core_tests\event\problematic_event1::create(array('xxx'=>0));
$this->assertDebuggingCalled();
@ -524,4 +526,4 @@ class core_event_testcase extends advanced_testcase {
$this->assertEquals(1, $user->id);
$this->assertSame('guest', $user->username);
}
}
}

8
lib/tests/fixtures/event_fixtures.php vendored
View File

@ -31,6 +31,14 @@ defined('MOODLE_INTERNAL') || die();
class unittest_executed extends \core\event\base {
public $nest = false;
public static function get_name() {
return 'xxx';
}
public function get_description() {
return 'yyy';
}
protected function init() {
$this->data['crud'] = 'u';
$this->data['level'] = 10;