From fdaa738bb5118fdbaf37996cf3359567196a5564 Mon Sep 17 00:00:00 2001
From: Laurent David <lmedavid@gmail.com>
Date: Fri, 4 Aug 2023 12:31:09 +0200
Subject: [PATCH 1/2] MDL-78907 mod_bigbluebuttonbn: Refactor instance class
* Make the constructor private as it should only be instancied through
static methods
* Do not duplicate the code between get_from_instanceid,
get_from_cmid and get_all_instances_in_course
---
mod/bigbluebuttonbn/classes/instance.php | 183 ++++++++++++--------
mod/bigbluebuttonbn/tests/instance_test.php | 21 ++-
mod/bigbluebuttonbn/upgrade.txt | 3 +
3 files changed, 133 insertions(+), 74 deletions(-)
diff --git a/mod/bigbluebuttonbn/classes/instance.php b/mod/bigbluebuttonbn/classes/instance.php
index 4e108b8f46d..cbae0f8ab6f 100644
--- a/mod/bigbluebuttonbn/classes/instance.php
+++ b/mod/bigbluebuttonbn/classes/instance.php
@@ -20,6 +20,7 @@ use cm_info;
use context;
use context_course;
use context_module;
+use core\dml\table;
use mod_bigbluebuttonbn\local\config;
use mod_bigbluebuttonbn\local\helpers\files;
use mod_bigbluebuttonbn\local\helpers\roles;
@@ -64,14 +65,16 @@ class instance {
protected $groupid;
/**
- * instance constructor.
+ * Instance constructor.
+ *
+ * Never called directly. Use self::get_from_instanceid or self::get_from_cmid.
*
* @param cm_info $cm
* @param stdClass $course
* @param stdClass $instancedata
* @param int|null $groupid
*/
- public function __construct(cm_info $cm, stdClass $course, stdClass $instancedata, ?int $groupid = null) {
+ private function __construct(cm_info $cm, stdClass $course, stdClass $instancedata, ?int $groupid = null) {
$this->cm = $cm;
$this->course = $course;
$this->instancedata = $instancedata;
@@ -101,42 +104,7 @@ class instance {
* @return null|self
*/
public static function get_from_instanceid(int $instanceid): ?self {
- global $DB;
-
- $coursetable = new \core\dml\table('course', 'c', 'c');
- $courseselect = $coursetable->get_field_select();
- $coursefrom = $coursetable->get_from_sql();
-
- $cmtable = new \core\dml\table('course_modules', 'cm', 'cm');
- $cmfrom = $cmtable->get_from_sql();
-
- $bbbtable = new \core\dml\table('bigbluebuttonbn', 'bbb', 'b');
- $bbbselect = $bbbtable->get_field_select();
- $bbbfrom = $bbbtable->get_from_sql();
-
- $sql = <<<EOF
- SELECT {$courseselect}, {$bbbselect}
- FROM {$cmfrom}
-INNER JOIN {$coursefrom} ON c.id = cm.course
-INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname
-INNER JOIN {$bbbfrom} ON cm.instance = bbb.id
- WHERE bbb.id = :instanceid
-EOF;
-
- $result = $DB->get_record_sql($sql, [
- 'modname' => 'bigbluebuttonbn',
- 'instanceid' => $instanceid,
- ]);
-
- if (empty($result)) {
- return null;
- }
-
- $course = $coursetable->extract_from_result($result);
- $instancedata = $bbbtable->extract_from_result($result);
- $cm = get_fast_modinfo($course)->instances['bigbluebuttonbn'][$instancedata->id];
-
- return new self($cm, $course, $instancedata);
+ return self::get_instance_info_retriever($instanceid, self::IDTYPE_INSTANCEID);
}
/**
@@ -146,32 +114,72 @@ EOF;
* @return null|self
*/
public static function get_from_cmid(int $cmid): ?self {
+ return self::get_instance_info_retriever($cmid, self::IDTYPE_CMID);
+ }
+
+ /**
+ * Get the instance information from a cmid.
+ */
+ const IDTYPE_CMID = 0;
+ /**
+ * Get the instance information from an id.
+ */
+ const IDTYPE_INSTANCEID = 1;
+
+ /**
+ * Helper to get the instance information from an id.
+ *
+ * Used by self::get_from_id and self::get_cmid.
+ *
+ * @param string $id The id to look for.
+ * @param int $idtype self::IDTYPE_CMID or self::IDTYPE_INSTANCEID
+ * @return null|self
+ * @throws \moodle_exception
+ */
+ private static function get_instance_info_retriever(string $id, int $idtype = self::IDTYPE_INSTANCEID): ?self {
global $DB;
- $coursetable = new \core\dml\table('course', 'c', 'c');
- $courseselect = $coursetable->get_field_select();
- $coursefrom = $coursetable->get_from_sql();
+ if (!in_array($idtype, [self::IDTYPE_CMID, self::IDTYPE_INSTANCEID])) {
+ throw new \moodle_exception('Invalid idtype');
+ }
- $cmtable = new \core\dml\table('course_modules', 'cm', 'cm');
- $cmfrom = $cmtable->get_from_sql();
+ [
+ 'coursetable' => $coursetable,
+ 'courseselect' => $courseselect,
+ 'coursefrom' => $coursefrom,
+ 'cmfrom' => $cmfrom,
+ 'cmselect' => $cmselect,
+ 'bbbtable' => $bbbtable,
+ 'bbbselect' => $bbbselect,
+ 'bbbfrom' => $bbbfrom,
+ ] = self::get_tables_info();
- $bbbtable = new \core\dml\table('bigbluebuttonbn', 'bbb', 'b');
- $bbbselect = $bbbtable->get_field_select();
- $bbbfrom = $bbbtable->get_from_sql();
-
- $sql = <<<EOF
- SELECT {$courseselect}, {$bbbselect}
- FROM {$cmfrom}
-INNER JOIN {$coursefrom} ON c.id = cm.course
-INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname
-INNER JOIN {$bbbfrom} ON cm.instance = bbb.id
- WHERE cm.id = :cmid
-EOF;
-
- $result = $DB->get_record_sql($sql, [
+ $select = implode(', ', [$courseselect, $bbbselect, $cmselect]);
+ $params = [
'modname' => 'bigbluebuttonbn',
- 'cmid' => $cmid,
- ]);
+ 'bbbid' => $id,
+ ];
+ $where = 'bbb.id = :bbbid';
+ $from = <<<EOF
+ {$bbbfrom}
+ INNER JOIN {$cmfrom} ON cm.instance = bbb.id
+ INNER JOIN {$coursefrom} ON c.id = cm.course
+ INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname
+EOF;
+ if ($idtype == self::IDTYPE_CMID) {
+ $params['cmid'] = $id;
+ $where = 'cm.id = :cmid';
+ $from = <<<EOF
+ {$cmfrom}
+ INNER JOIN {$coursefrom} ON c.id = cm.course
+ INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname
+ INNER JOIN {$bbbfrom} ON cm.instance = bbb.id
+EOF;
+ }
+
+ $sql = "SELECT {$select} FROM {$from} WHERE {$where}";
+
+ $result = $DB->get_record_sql($sql, $params);
if (empty($result)) {
return null;
@@ -179,8 +187,13 @@ EOF;
$course = $coursetable->extract_from_result($result);
$instancedata = $bbbtable->extract_from_result($result);
- $cm = get_fast_modinfo($course)->get_cm($cmid);
-
+ if ($idtype == self::IDTYPE_INSTANCEID) {
+ $cmid = $result->cmid;
+ } else {
+ $cmid = $id;
+ }
+ $modinfo = get_fast_modinfo($course);
+ $cm = $modinfo->get_cm($cmid);
return new self($cm, $course, $instancedata);
}
@@ -233,20 +246,19 @@ EOF;
*/
public static function get_all_instances_in_course(int $courseid): array {
global $DB;
+ [
+ 'coursetable' => $coursetable,
+ 'courseselect' => $courseselect,
+ 'coursefrom' => $coursefrom,
+ 'cmfrom' => $cmfrom,
+ 'bbbtable' => $bbbtable,
+ 'bbbselect' => $bbbselect,
+ 'bbbfrom' => $bbbfrom
+ ] = self::get_tables_info();
- $coursetable = new \core\dml\table('course', 'c', 'c');
- $courseselect = $coursetable->get_field_select();
- $coursefrom = $coursetable->get_from_sql();
-
- $cmtable = new \core\dml\table('course_modules', 'cm', 'cm');
- $cmfrom = $cmtable->get_from_sql();
-
- $bbbtable = new \core\dml\table('bigbluebuttonbn', 'bbb', 'b');
- $bbbselect = $bbbtable->get_field_select();
- $bbbfrom = $bbbtable->get_from_sql();
-
+ $selects = implode(', ', [$courseselect, $bbbselect]);
$sql = <<<EOF
- SELECT cm.id as cmid, {$courseselect}, {$bbbselect}
+ SELECT cm.id as cmid, {$selects}
FROM {$cmfrom}
INNER JOIN {$coursefrom} ON c.id = cm.course
INNER JOIN {modules} m ON m.id = cm.module AND m.name = :modname
@@ -269,7 +281,32 @@ EOF;
return $instances;
}
+ // Add an helper method that can be used in both self::get_all_instances_in_course and self::get_all_instances_in_course.
+ // This method will be used to get the additional tables returned from the subplugin.
+ /**
+ * Get the additional tables returned from the subplugin.
+ *
+ * @return array
+ */
+ private static function get_tables_info(): array {
+ $coursetable = new table('course', 'c', 'c');
+ $courseselect = $coursetable->get_field_select();
+ $coursefrom = $coursetable->get_from_sql();
+ $cmtable = new table('course_modules', 'cm', 'cm');
+ $cmselect = $cmtable->get_field_select();
+ $cmfrom = $cmtable->get_from_sql();
+
+ $bbbtable = new table('bigbluebuttonbn', 'bbb', 'b');
+ $bbbselect = $bbbtable->get_field_select();
+ $bbbfrom = $bbbtable->get_from_sql();
+
+ return compact(
+ 'coursetable', 'courseselect', 'coursefrom',
+ 'cmtable', 'cmselect', 'cmfrom',
+ 'bbbtable', 'bbbselect', 'bbbfrom'
+ );
+ }
/**
* Set the current group id of the activity.
*
diff --git a/mod/bigbluebuttonbn/tests/instance_test.php b/mod/bigbluebuttonbn/tests/instance_test.php
index 8fe81cb2277..1d9338276c1 100644
--- a/mod/bigbluebuttonbn/tests/instance_test.php
+++ b/mod/bigbluebuttonbn/tests/instance_test.php
@@ -588,6 +588,26 @@ class instance_test extends advanced_testcase {
$this->assertFalse($instance->is_guest_allowed());
}
+ /**
+ * Test private method get_instance_info_retriever
+ *
+ * @covers ::get_instance_info_retriever
+ */
+ public function test_get_instance_info_retriever() {
+ $this->resetAfterTest();
+ [
+ 'record' => $record,
+ 'cm' => $cm,
+ ] = $this->get_test_instance();
+ $instance = instance::get_from_instanceid($record->id);
+ $instancereflection = new \ReflectionClass($instance);
+ $getinstanceinforetriever = $instancereflection->getMethod('get_instance_info_retriever');
+ $getinstanceinforetriever->setAccessible(true);
+ $this->assertInstanceOf('\mod_bigbluebuttonbn\instance',
+ $getinstanceinforetriever->invoke($instance, $record->id, instance::IDTYPE_INSTANCEID));
+ $this->assertEquals($cm->id, $instance->get_cm_id());
+ }
+
/**
* Test guest access password
*
@@ -601,5 +621,4 @@ class instance_test extends advanced_testcase {
$instance = instance::get_from_instanceid($record->id);
$this->assertNotEmpty($instance->get_guest_access_password());
}
-
}
diff --git a/mod/bigbluebuttonbn/upgrade.txt b/mod/bigbluebuttonbn/upgrade.txt
index 5871a50f02f..4a8da0ee7e7 100644
--- a/mod/bigbluebuttonbn/upgrade.txt
+++ b/mod/bigbluebuttonbn/upgrade.txt
@@ -1,4 +1,7 @@
This files describes API changes in the bigbluebuttonbn code.
+=== 4.3 ===
+* Make instance class constructor private and use the factory method to create instances.
+
=== 4.2 ===
* Simplify bigbluebutton_proxy get_join_url so to pass only parameters that cannot be deduced from the instance.
* Add a new parameter for mod_bigbluebuttonbn\recording::sync_pending_recordings_from_server so sync only 'dismissed' recordings
From 23caba90bdc7e801b9277d8aaf850ff3304c4a90 Mon Sep 17 00:00:00 2001
From: Laurent David <lmedavid@gmail.com>
Date: Sat, 5 Aug 2023 11:02:09 +0200
Subject: [PATCH 2/2] MDL-78907 mod_bigbluebuttonbn: Get cm when necessary
* The constructor of mod_bigbluebuttonbn\instance was provided with a cm_info.
This forced us to retrieve it whenever we created an instance. The issue is that
sometimes we need all the "other" data but not necessarily the cm_info.
So we now lazy load it.
---
mod/bigbluebuttonbn/classes/instance.php | 35 ++++++++++++++----------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/mod/bigbluebuttonbn/classes/instance.php b/mod/bigbluebuttonbn/classes/instance.php
index cbae0f8ab6f..bb348d53be8 100644
--- a/mod/bigbluebuttonbn/classes/instance.php
+++ b/mod/bigbluebuttonbn/classes/instance.php
@@ -64,18 +64,22 @@ class instance {
/** @var int The current groupid if set */
protected $groupid;
+ /** @var int The course module id. */
+ protected $cmid;
+
/**
* Instance constructor.
*
* Never called directly. Use self::get_from_instanceid or self::get_from_cmid.
*
- * @param cm_info $cm
+ * @param int $cmid
* @param stdClass $course
* @param stdClass $instancedata
* @param int|null $groupid
*/
- private function __construct(cm_info $cm, stdClass $course, stdClass $instancedata, ?int $groupid = null) {
- $this->cm = $cm;
+ private function __construct(int $cmid, stdClass $course, stdClass $instancedata, ?int $groupid = null) {
+ $this->cmid = $cmid;
+ $this->cm = null; // This is not retrieved later, whenever we call ::get_cm() it will be retrieved.
$this->course = $course;
$this->instancedata = $instancedata;
$this->groupid = $groupid;
@@ -90,7 +94,7 @@ class instance {
*/
public static function get_group_instance_from_instance(self $originalinstance, int $groupid): ?self {
return new self(
- $originalinstance->get_cm(),
+ $originalinstance->get_cm_id(),
$originalinstance->get_course(),
$originalinstance->get_instance_data(),
$groupid
@@ -192,9 +196,7 @@ EOF;
} else {
$cmid = $id;
}
- $modinfo = get_fast_modinfo($course);
- $cm = $modinfo->get_cm($cmid);
- return new self($cm, $course, $instancedata);
+ return new self($cmid, $course, $instancedata);
}
/**
@@ -275,8 +277,7 @@ EOF;
foreach ($results as $result) {
$course = $coursetable->extract_from_result($result);
$instancedata = $bbbtable->extract_from_result($result);
- $cm = get_fast_modinfo($course)->get_cm($result->cmid);
- $instances[$cm->id] = new self($cm, $course, $instancedata);
+ $instances[$result->cmid] = new self($result->cmid, $course, $instancedata);
}
return $instances;
@@ -378,6 +379,12 @@ EOF;
* @return cm_info
*/
public function get_cm(): cm_info {
+ if ($this->cm === null) {
+ // We do a sort of late binding here as if we call get_cm on a disabled module or in a call stack where
+ // get_cm was already called, we will get an exception or infinite loop.
+ $modinfo = get_fast_modinfo($this->course);
+ $this->cm = $modinfo->get_cm($this->cmid);
+ }
return $this->cm;
}
@@ -387,7 +394,7 @@ EOF;
* @return int
*/
public function get_cm_id(): int {
- return $this->get_cm()->id;
+ return $this->cmid;
}
/**
@@ -1058,7 +1065,7 @@ EOF;
*/
public function get_view_url(): moodle_url {
return new moodle_url('/mod/bigbluebuttonbn/view.php', [
- 'id' => $this->cm->id,
+ 'id' => $this->get_cm()->id,
]);
}
@@ -1070,8 +1077,8 @@ EOF;
public function get_logout_url(): moodle_url {
return new moodle_url('/mod/bigbluebuttonbn/bbb_view.php', [
'action' => 'logout',
- 'id' => $this->cm->id,
- 'courseid' => $this->cm->course // Used to find the course if ever the activity is deleted
+ 'id' => $this->get_cm()->id,
+ 'courseid' => $this->get_cm()->course // Used to find the course if ever the activity is deleted
// while the meeting is running.
]);
}
@@ -1108,7 +1115,7 @@ EOF;
public function get_join_url(): moodle_url {
return new moodle_url('/mod/bigbluebuttonbn/bbb_view.php', [
'action' => 'join',
- 'id' => $this->cm->id,
+ 'id' => $this->get_cm()->id,
'bn' => $this->instancedata->id,
]);
}