From a29bcf781937e8e440e6baac69c7f028f52c97eb Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Fri, 28 Oct 2016 00:06:31 +0200 Subject: [PATCH] MDL-47162 core_message: debug whenever courseid is missing Instead of silently defaulting to SITEID when courseid (coming from message_send()/\core\message\manager::send_message()) is missing, now a debugging message is shown to allow developers to fix their messages to, always, include courseid. Raw creation of events via message_sent::create() missing other[courseid] leads to coding exception since now (there shouldn't be any legacy use, as far as they are always created via create_from_ids() when sending a message. Updated upgrade.txt notes a little bit, added references the 3.6 final deprecation issue (MDL-55449) and covered with unit tests. --- lib/classes/event/message_sent.php | 10 ++++- lib/classes/message/manager.php | 3 ++ lib/messagelib.php | 2 + lib/upgrade.txt | 8 +++- message/tests/events_test.php | 60 +++++++++++++++++++++++++++++- 5 files changed, 78 insertions(+), 5 deletions(-) diff --git a/lib/classes/event/message_sent.php b/lib/classes/event/message_sent.php index 0ea9a15f097..e3309191361 100644 --- a/lib/classes/event/message_sent.php +++ b/lib/classes/event/message_sent.php @@ -44,10 +44,11 @@ defined('MOODLE_INTERNAL') || die(); class message_sent extends base { /** * Create event using ids. + * @todo MDL-55449 Make $courseid mandatory in Moodle 3.6 * @param int $userfromid * @param int $usertoid * @param int $messageid - * @param int|null $courseid + * @param int|null $courseid course id the event is related with. Use SITEID if no relation exists. * @return message_sent */ public static function create_from_ids($userfromid, $usertoid, $messageid, $courseid = null) { @@ -58,8 +59,13 @@ class message_sent extends base { $userfromid = 0; } + // TODO: MDL-55449 Make $courseid mandatory in Moodle 3.6. if (is_null($courseid)) { + // Arrived here with not defined $courseid to associate the event with. + // Let's default to SITEID and perform debugging so devs are aware. MDL-47162. $courseid = SITEID; + debugging('message_sent::create_from_ids() needs a $courseid to be passed, nothing was detected. Please, change ' . + 'the call to include it, using SITEID if the message is unrelated to any real course.', DEBUG_DEVELOPER); } $event = self::create(array( @@ -152,7 +158,7 @@ class message_sent extends base { } if (!isset($this->other['courseid'])) { - debugging('The \'courseid\' value must be set in other.', DEBUG_DEVELOPER); + throw new \coding_exception('The \'courseid\' value must be set in other.'); } } diff --git a/lib/classes/message/manager.php b/lib/classes/message/manager.php index 61f3e6f933c..21e046a8624 100644 --- a/lib/classes/message/manager.php +++ b/lib/classes/message/manager.php @@ -50,6 +50,7 @@ class manager { * * NOTE: to be used from message_send() only. * + * @todo MDL-55449 Drop support for stdClass in Moodle 3.6 * @param \core\message\message $eventdata fully prepared event data for processors * @param \stdClass $savemessage the message saved in 'message' table * @param array $processorlist list of processors for target user @@ -58,11 +59,13 @@ class manager { public static function send_message($eventdata, \stdClass $savemessage, array $processorlist) { global $CFG; + // TODO MDL-55449 Drop support for stdClass in Moodle 3.6. if (!($eventdata instanceof \stdClass) && !($eventdata instanceof message)) { // Not a valid object. throw new \coding_exception('Message should be of type stdClass or \core\message\message'); } + // TODO MDL-55449 Drop support for stdClass in Moodle 3.6. if ($eventdata instanceof \stdClass) { if (!isset($eventdata->courseid)) { $eventdata->courseid = null; diff --git a/lib/messagelib.php b/lib/messagelib.php index a9d4908ac10..34e7c38ae92 100644 --- a/lib/messagelib.php +++ b/lib/messagelib.php @@ -50,6 +50,7 @@ require_once(__DIR__ . '/../message/lib.php'); * Note: processor failure is is not reported as false return value, * earlier versions did not do it consistently either. * + * @todo MDL-55449 Drop support for stdClass in Moodle 3.6 * @category message * @param \core\message\message $eventdata information about the message (component, userfrom, userto, ...) * @return mixed the integer ID of the new message or false if there was a problem with submitted data @@ -57,6 +58,7 @@ require_once(__DIR__ . '/../message/lib.php'); function message_send($eventdata) { global $CFG, $DB; + // TODO MDL-55449 Drop support for stdClass in Moodle 3.6. if ($eventdata instanceof \stdClass) { if (!isset($eventdata->courseid)) { $eventdata->courseid = null; diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 6475461f91b..f557986b1db 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -119,8 +119,12 @@ information provided here is intended especially for developers. * Webservice function mod_assign_get_submissions returns a new field 'gradingstatus' from each submission. * The return signature for the antivirus::scan_file() function has changed. The calling function will now handle removal of infected files from Moodle based on the new integer return value. -* The first parameter $eventdata of \core\manager::send_message() should be \core\message. Use of \stdClass is depecated. -* message_sent::create_from_ids has an additional required parameter $courseid with a default value of SITEID. +* The first parameter $eventdata of both message_send() and \core\message\manager::send_message() should + be \core\message\message. Use of stdClass is deprecated. +* The message_sent event now expects other[courseid] to be always set, exception otherwise. For BC with contrib code, + message_sent::create_from_ids() will show a debugging notice if the \core\message\message being sent is missing + the courseid property, defaulting to SITEID automatically. In Moodle 3.6 (MDL-55449) courseid will be fully mandatory + for all messages sent. === 3.1 === diff --git a/message/tests/events_test.php b/message/tests/events_test.php index a4081a93d8b..4006738c234 100644 --- a/message/tests/events_test.php +++ b/message/tests/events_test.php @@ -202,7 +202,7 @@ class core_message_events_testcase extends advanced_testcase { 'relateduserid' => 2, 'other' => array( 'messageid' => 3, - 'courseid' => 1 + 'courseid' => 4 ) )); @@ -219,8 +219,66 @@ class core_message_events_testcase extends advanced_testcase { $this->assertEventLegacyLogData($expected, $event); $url = new moodle_url('/message/index.php', array('user1' => $event->userid, 'user2' => $event->relateduserid)); $this->assertEquals($url, $event->get_url()); + $this->assertEquals(3, $event->other['messageid']); + $this->assertEquals(4, $event->other['courseid']); } + public function test_mesage_sent_without_other_courseid() { + + // Creating a message_sent event without other[courseid] leads to exception. + $this->expectException('coding_exception'); + $this->expectExceptionMessage('The \'courseid\' value must be set in other'); + + $event = \core\event\message_sent::create(array( + 'userid' => 1, + 'context' => context_system::instance(), + 'relateduserid' => 2, + 'other' => array( + 'messageid' => 3, + ) + )); + } + + public function test_mesage_sent_via_create_from_ids() { + // Containing courseid. + $event = \core\event\message_sent::create_from_ids(1, 2, 3, 4); + + // Trigger and capturing the event. + $sink = $this->redirectEvents(); + $event->trigger(); + $events = $sink->get_events(); + $event = reset($events); + + // Check that the event data is valid. + $this->assertInstanceOf('\core\event\message_sent', $event); + $this->assertEquals(context_system::instance(), $event->get_context()); + $expected = array(SITEID, 'message', 'write', 'index.php?user=1&id=2&history=1#m3', 1); + $this->assertEventLegacyLogData($expected, $event); + $url = new moodle_url('/message/index.php', array('user1' => $event->userid, 'user2' => $event->relateduserid)); + $this->assertEquals($url, $event->get_url()); + $this->assertEquals(3, $event->other['messageid']); + $this->assertEquals(4, $event->other['courseid']); + } + + public function test_mesage_sent_via_create_from_ids_without_other_courseid() { + + // Creating a message_sent event without courseid leads to debugging + SITEID. + // TODO: MDL-55449 Ensure this leads to exception instead of debugging in Moodle 3.6. + $event = \core\event\message_sent::create_from_ids(1, 2, 3); + + // Trigger and capturing the event. + $sink = $this->redirectEvents(); + $event->trigger(); + $events = $sink->get_events(); + $event = reset($events); + + $this->assertDebuggingCalled(); + $this->assertEquals(SITEID, $event->other['courseid']); + } + + + + /** * Test the message viewed event. */