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

@@ -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.');
         }
     }
 

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;

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;

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.
+* The first parameter $eventdata of both message_send() and \core\message\manager::send_message() should
-* message_sent::create_from_ids has an additional required parameter $courseid with a default value of SITEID.
+  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 ===
 

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.
      */