MDL-41219 course: Make properties of course_modinfo read-only

added phpdocs and unittests

diff --git a/lib/modinfolib.php b/lib/modinfolib.php
index a3af70c..402cb7e 100644 (file)
--- a/lib/modinfolib.php
+++ b/lib/modinfolib.php
@@ -39,69 +39,140 @@ if (!defined('MAX_MODINFO_CACHE_SIZE')) {
  *
  * This includes information about the course-modules and the sections on the course. It can also
  * include dynamic data that has been updated for the current user.
+ *
+ * Use {@link get_fast_modinfo()} to retrieve the instance of the object for particular course
+ * and particular user.
+ *
+ * @property-read int $courseid Course ID
+ * @property-read int $userid User ID
+ * @property-read array $sections Array from section number (e.g. 0) to array of course-module IDs in that
+ *     section; this only includes sections that contain at least one course-module
+ * @property-read cm_info[] $cms Array from course-module instance to cm_info object within this course, in
+ *     order of appearance
+ * @property-read cm_info[][] $instances Array from string (modname) => int (instance id) => cm_info object
+ * @property-read array $groups Groups that the current user belongs to. Calculated on the first request.
+ *     Is an array of grouping id => array of group id => group id. Includes grouping id 0 for 'all groups'
  */
-class course_modinfo extends stdClass {
-    // For convenience we store the course object here as it is needed in other parts of code
+class course_modinfo {
+    /**
+     * For convenience we store the course object here as it is needed in other parts of code
+     * @var stdClass
+     */
     private $course;
-    // Array of section data from cache
-    private $sectioninfo;
-
-    // Existing data fields
-    ///////////////////////
-
-    // These are public for backward compatibility. Note: it is not possible to retain BC
-    // using PHP magic get methods because behaviour is different with regard to empty().
 
     /**
-     * Course ID
-     * @var int
-     * @deprecated For new code, use get_course_id instead.
+     * Array of section data from cache
+     * @var section_info[]
      */
-    public $courseid;
+    private $sectioninfo;
 
     /**
      * User ID
      * @var int
-     * @deprecated For new code, use get_user_id instead.
      */
-    public $userid;
+    private $userid;
 
     /**
      * Array from int (section num, e.g. 0) => array of int (course-module id); this list only
      * includes sections that actually contain at least one course-module
      * @var array
-     * @deprecated For new code, use get_sections instead
      */
-    public $sections;
+    private $sections;
 
     /**
      * Array from int (cm id) => cm_info object
-     * @var array
-     * @deprecated For new code, use get_cms or get_cm instead.
+     * @var cm_info[]
      */
-    public $cms;
+    private $cms;
 
     /**
      * Array from string (modname) => int (instance id) => cm_info object
-     * @var array
-     * @deprecated For new code, use get_instances or get_instances_of instead.
+     * @var cm_info[][]
      */
-    public $instances;
+    private $instances;
 
     /**
      * Groups that the current user belongs to. This value is usually not available (set to null)
      * unless the course has activities set to groupmembersonly. When set, it is an array of
      * grouping id => array of group id => group id. Includes grouping id 0 for 'all groups'.
+     * @var int[][]
+     */
+    private $groups;
+
+    /**
+     * List of class read-only properties and their getter methods.
+     * Used by magic functions __get(), __isset(), __empty()
      * @var array
-     * @deprecated Don't use this! For new code, use get_groups.
      */
-    public $groups;
+    private static $standardproperties = array(
+        'courseid' => 'get_course_id',
+        'userid' => 'get_user_id',
+        'sections' => 'get_sections',
+        'cms' => 'get_cms',
+        'instances' => 'get_instances',
+        'groups' => 'get_groups_all',
+    );
 
-    // Get methods for data
-    ///////////////////////
+    /**
+     * Magic method getter
+     *
+     * @param string $name
+     * @return mixed
+     */
+    public function __get($name) {
+        if (isset(self::$standardproperties[$name])) {
+            $method = self::$standardproperties[$name];
+            return $this->$method();
+        } else {
+            debugging('Invalid course_modinfo property accessed: '.$name);
+            return null;
+        }
+    }
 
     /**
-     * @return object Moodle course object that was used to construct this data
+     * Magic method for function isset()
+     *
+     * @param string $name
+     * @return bool
+     */
+    public function __isset($name) {
+        if (isset(self::$standardproperties[$name])) {
+            $value = $this->__get($name);
+            return isset($value);
+        }
+        return false;
+    }
+
+    /**
+     * Magic method for function empty()
+     *
+     * @param string $name
+     * @return bool
+     */
+    public function __empty($name) {
+        if (isset(self::$standardproperties[$name])) {
+            $value = $this->__get($name);
+            return empty($value);
+        }
+        return true;
+    }
+
+    /**
+     * Magic method setter
+     *
+     * Will display the developer warning when trying to set/overwrite existing property.
+     *
+     * @param string $name
+     * @param mixed $value
+     */
+    public function __set($name, $value) {
+        debugging("It is not allowed to set the property course_modinfo::\${$name}", DEBUG_DEVELOPER);
+    }
+
+    /**
+     * Returns course object that was used in the first get_fast_modinfo() call.
+     *
+     * @return stdClass
      */
     public function get_course() {
         return $this->course;
@@ -111,7 +182,7 @@ class course_modinfo extends stdClass {
      * @return int Course ID
      */
     public function get_course_id() {
-        return $this->courseid;
+        return $this->course->id;
     }
 
     /**
@@ -130,7 +201,7 @@ class course_modinfo extends stdClass {
     }
 
     /**
-     * @return array Array from course-module instance to cm_info object within this course, in
+     * @return cm_info[] Array from course-module instance to cm_info object within this course, in
      *   order of appearance
      */
     public function get_cms() {
@@ -152,7 +223,7 @@ class course_modinfo extends stdClass {
 
     /**
      * Obtains all module instances on this course.
-     * @return array Array from module name => array from instance id => cm_info
+     * @return cm_info[][] Array from module name => array from instance id => cm_info
      */
     public function get_instances() {
         return $this->instances;
@@ -179,7 +250,7 @@ class course_modinfo extends stdClass {
     /**
      * Obtains all instances of a particular module on this course.
      * @param $modname Name of module (not full frankenstyle) e.g. 'label'
-     * @return array Array from instance id => cm_info for modules on this course; empty if none
+     * @return cm_info[] Array from instance id => cm_info for modules on this course; empty if none
      */
     public function get_instances_of($modname) {
         if (empty($this->instances[$modname])) {
@@ -189,29 +260,38 @@ class course_modinfo extends stdClass {
     }
 
     /**
-     * Returns groups that the current user belongs to on the course. Note: If not already
-     * available, this may make a database query.
-     * @param int $groupingid Grouping ID or 0 (default) for all groups
-     * @return array Array of int (group id) => int (same group id again); empty array if none
+     * Groups that the current user belongs to organised by grouping id. Calculated on the first request.
+     * @return int[][] array of grouping id => array of group id => group id. Includes grouping id 0 for 'all groups'
      */
-    public function get_groups($groupingid=0) {
+    private function get_groups_all() {
         if (is_null($this->groups)) {
             // NOTE: Performance could be improved here. The system caches user groups
             // in $USER->groupmember[$courseid] => array of groupid=>groupid. Unfortunately this
             // structure does not include grouping information. It probably could be changed to
             // do so, without a significant performance hit on login, thus saving this one query
             // each request.
-            $this->groups = groups_get_user_groups($this->courseid, $this->userid);
+            $this->groups = groups_get_user_groups($this->course->id, $this->userid);
         }
-        if (!isset($this->groups[$groupingid])) {
+        return $this->groups;
+    }
+
+    /**
+     * Returns groups that the current user belongs to on the course. Note: If not already
+     * available, this may make a database query.
+     * @param int $groupingid Grouping ID or 0 (default) for all groups
+     * @return int[] Array of int (group id) => int (same group id again); empty array if none
+     */
+    public function get_groups($groupingid = 0) {
+        $allgroups = $this->get_groups_all();
+        if (!isset($allgroups[$groupingid])) {
             return array();
         }
-        return $this->groups[$groupingid];
+        return $allgroups[$groupingid];
     }
 
     /**
      * Gets all sections as array from section number => data about section.
-     * @return array Array of section_info objects organised by section number
+     * @return section_info[] Array of section_info objects organised by section number
      */
     public function get_section_info_all() {
         return $this->sectioninfo;
@@ -255,7 +335,6 @@ class course_modinfo extends stdClass {
         }
 
         // Set initial values
-        $this->courseid = $course->id;
         $this->userid = $userid;
         $this->sections = array();
         $this->cms = array();
@@ -315,7 +394,7 @@ class course_modinfo extends stdClass {
         }
 
         // Loop through each piece of module data, constructing it
-        $modexists = array();
+        static $modexists = array();
         foreach ($info as $mod) {
             if (empty($mod->name)) {
                 // something is wrong here
@@ -323,11 +402,11 @@ class course_modinfo extends stdClass {
             }
 
             // Skip modules which don't exist
-            if (empty($modexists[$mod->mod])) {
-                if (!file_exists("$CFG->dirroot/mod/$mod->mod/lib.php")) {
-                    continue;
-                }
-                $modexists[$mod->mod] = true;
+            if (!array_key_exists($mod->mod, $modexists)) {
+                $modexists[$mod->mod] = file_exists("$CFG->dirroot/mod/$mod->mod/lib.php");
+            }
+            if (!$modexists[$mod->mod]) {
+                continue;
             }
 
             // Construct info for this module

diff --git a/lib/tests/modinfolib_test.php b/lib/tests/modinfolib_test.php
index afb66ac..92ac122 100644 (file)
--- a/lib/tests/modinfolib_test.php
+++ b/lib/tests/modinfolib_test.php
@@ -267,6 +267,88 @@ class core_modinfolib_testcase extends advanced_testcase {
         set_config('enablecompletion', $oldcfgenablecompletion);
     }
 
+    public function test_course_modinfo_properties() {
+        global $USER, $DB;
+
+        $this->resetAfterTest();
+        $this->setAdminUser();
+
+        // Generate the course and some modules. Make one section hidden.
+        $course = $this->getDataGenerator()->create_course(
+                array('format' => 'topics',
+                    'numsections' => 3),
+                array('createsections' => true));
+        $DB->execute('UPDATE {course_sections} SET visible = 0 WHERE course = ? and section = ?',
+                array($course->id, 3));
+        $coursecontext = context_course::instance($course->id);
+        $forum0 = $this->getDataGenerator()->create_module('forum',
+                array('course' => $course->id), array('section' => 0));
+        $assign0 = $this->getDataGenerator()->create_module('assign',
+                array('course' => $course->id), array('section' => 0, 'visible' => 0));
+        $forum1 = $this->getDataGenerator()->create_module('forum',
+                array('course' => $course->id), array('section' => 1));
+        $assign1 = $this->getDataGenerator()->create_module('assign',
+                array('course' => $course->id), array('section' => 1));
+        $page1 = $this->getDataGenerator()->create_module('page',
+                array('course' => $course->id), array('section' => 1));
+        $page3 = $this->getDataGenerator()->create_module('page',
+                array('course' => $course->id), array('section' => 3));
+
+        $modinfo = get_fast_modinfo($course->id);
+
+        $this->assertEquals(array($forum0->cmid, $assign0->cmid, $forum1->cmid, $assign1->cmid, $page1->cmid, $page3->cmid),
+                array_keys($modinfo->cms));
+        $this->assertEquals($course->id, $modinfo->courseid);
+        $this->assertEquals($USER->id, $modinfo->userid);
+        $this->assertEquals(array(0 => array($forum0->cmid, $assign0->cmid),
+            1 => array($forum1->cmid, $assign1->cmid, $page1->cmid), 3 => array($page3->cmid)), $modinfo->sections);
+        $this->assertEquals(array('forum', 'assign', 'page'), array_keys($modinfo->instances));
+        $this->assertEquals(array($assign0->id, $assign1->id), array_keys($modinfo->instances['assign']));
+        $this->assertEquals(array($forum0->id, $forum1->id), array_keys($modinfo->instances['forum']));
+        $this->assertEquals(array($page1->id, $page3->id), array_keys($modinfo->instances['page']));
+        $this->assertEquals(groups_get_user_groups($course->id), $modinfo->groups);
+        $this->assertEquals(array(0 => array($forum0->cmid, $assign0->cmid),
+            1 => array($forum1->cmid, $assign1->cmid, $page1->cmid),
+            3 => array($page3->cmid)), $modinfo->get_sections());
+        $this->assertEquals(array(0, 1, 2, 3), array_keys($modinfo->get_section_info_all()));
+        $this->assertEquals($forum0->cmid . ',' . $assign0->cmid, $modinfo->get_section_info(0)->sequence);
+        $this->assertEquals($forum1->cmid . ',' . $assign1->cmid . ',' . $page1->cmid, $modinfo->get_section_info(1)->sequence);
+        $this->assertEquals('', $modinfo->get_section_info(2)->sequence);
+        $this->assertEquals($page3->cmid, $modinfo->get_section_info(3)->sequence);
+        $this->assertEquals($course->id, $modinfo->get_course()->id);
+        $this->assertEquals(array('assign', 'forum', 'page'),
+                array_keys($modinfo->get_used_module_names()));
+        $this->assertEquals(array('assign', 'forum', 'page'),
+                array_keys($modinfo->get_used_module_names(true)));
+        // Admin can see hidden modules/sections.
+        $this->assertTrue($modinfo->cms[$assign0->cmid]->uservisible);
+        $this->assertTrue($modinfo->get_section_info(3)->uservisible);
+
+        // Get modinfo for non-current user (without capability to view hidden activities/sections).
+        $user = $this->getDataGenerator()->create_user();
+        $modinfo = get_fast_modinfo($course->id, $user->id);
+        $this->assertEquals($user->id, $modinfo->userid);
+        $this->assertFalse($modinfo->cms[$assign0->cmid]->uservisible);
+        $this->assertFalse($modinfo->get_section_info(3)->uservisible);
+
+        // Attempt to access and set non-existing field.
+        $this->assertTrue(empty($modinfo->somefield));
+        $this->assertFalse(isset($modinfo->somefield));
+        $modinfo->somefield;
+        $this->assertDebuggingCalled();
+        $modinfo->somefield = 'Some value';
+        $this->assertDebuggingCalled();
+        $this->assertEmpty($modinfo->somefield);
+        $this->assertDebuggingCalled();
+
+        // Attempt to overwrite existing field.
+        $this->assertFalse(empty($modinfo->cms));
+        $this->assertTrue(isset($modinfo->cms));
+        $modinfo->cms = 'Illegal overwriting';
+        $this->assertDebuggingCalled();
+        $this->assertNotEquals('Illegal overwriting', $modinfo->cms);
+    }
+
     /**
      * Test is_user_access_restricted_by_group()
      *

diff --git a/lib/upgrade.txt b/lib/upgrade.txt
index b2f99ab..6ecf4d1 100644 (file)
--- a/lib/upgrade.txt
+++ b/lib/upgrade.txt
@@ -26,6 +26,7 @@ information provided here is intended especially for developers.
   to report progress during long operations. Related to this, zip_archive now supports an estimated_count()
   function that returns an approximate number of entries in the zip faster than the count() function.
 * Class cm_info no longer extends stdClass. All properties are read-only and calculated on first request only.
+* Class course_modinfo no longer extends stdClass. All properties are read-only.
 
 DEPRECATIONS:
 Various previously deprecated functions have now been altered to throw DEBUG_DEVELOPER debugging notices

diff --git a/mod/forum/index.php b/mod/forum/index.php
index 3658a3f..96e1363 100644 (file)
--- a/mod/forum/index.php
+++ b/mod/forum/index.php
@@ -138,11 +138,7 @@ $generalforums  = array();
 $learningforums = array();
 $modinfo = get_fast_modinfo($course);
 
-if (!isset($modinfo->instances['forum'])) {
-    $modinfo->instances['forum'] = array();
-}
-
-foreach ($modinfo->instances['forum'] as $forumid=>$cm) {
+foreach ($modinfo->get_instances_of('forum') as $forumid=>$cm) {
     if (!$cm->uservisible or !isset($forums[$forumid])) {
         continue;
     }
@@ -176,7 +172,7 @@ if (!is_null($subscribe)) {
         redirect(new moodle_url('/mod/forum/index.php', array('id' => $id)), get_string('subscribeenrolledonly', 'forum'));
     }
     // Can proceed now, the user is not guest and is enrolled
-    foreach ($modinfo->instances['forum'] as $forumid=>$cm) {
+    foreach ($modinfo->get_instances_of('forum') as $forumid=>$cm) {
         $forum = $forums[$forumid];
         $modcontext = context_module::instance($cm->id);
         $cansub = false;

diff --git a/rss/file.php b/rss/file.php
index 0ff00d5..6d7dbc0 100644 (file)
--- a/rss/file.php
+++ b/rss/file.php
@@ -79,11 +79,7 @@ if ($token==="$inttoken") {
     if ($course = $DB->get_record('course', array('id' => $courseid))) {
         $modinfo = get_fast_modinfo($course);
 
-        if (!isset($modinfo->instances[$componentname])) {
-            $modinfo->instances[$componentname] = array();
-        }
-
-        foreach ($modinfo->instances[$componentname] as $modinstanceid=>$cm) {
+        foreach ($modinfo->get_instances_of($componentname) as $modinstanceid=>$cm) {
             if ($modinstanceid==$instanceid) {
                 $context = context_module::instance($cm->id, IGNORE_MISSING);
                 break;