MDL-24256 question category editing was messed up.
authorTim Hunt <T.J.Hunt@open.ac.uk>
Tue, 2 Nov 2010 18:38:50 +0000 (18:38 +0000)
committerTim Hunt <T.J.Hunt@open.ac.uk>
Tue, 2 Nov 2010 18:38:50 +0000 (18:38 +0000)
The unerlying problem seemed to be too many uses of pass-by-reference in listlib, where it was not necessary.
In investigating this code, I ended up doing a fair bit of cleaning up. Apologies that it leads to an unclear changeset.

lib/listlib.php
question/category_class.php

index ce70893..7a3979b 100644 (file)
@@ -44,53 +44,43 @@ defined('MOODLE_INTERNAL') || die();
  * @copyright Jamie Pratt
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class moodle_list {
-    var $attributes;
-    var $listitemclassname = 'list_item';
-    /**
-     * An array of $listitemclassname objects.
-     * @var array
-     */
-    var $items = array();
-    /**
-     * ol / ul
-     * @var string
-     */
-    var $type;
-    /**
-     * @var list_item or derived class
-     */
-    var $parentitem = null;
-    var $table;
-    var $fieldnamesparent = 'parent';
-    /**
-     * Records from db, only used in top level list.
-     * @var array
-     */
-    var $records = array();
+abstract class moodle_list {
+    public $attributes;
+    public $listitemclassname = 'list_item';
 
-    var $editable;
+    /** @var array of $listitemclassname objects. */
+    public $items = array();
 
-    /**
-     * Key is child id, value is parent.
-     * @var array
-     */
-    var $childparent;
+    /** @var string 'ol' or 'ul'. */
+    public $type;
+
+    /** @var list_item or derived class. */
+    public $parentitem = null;
+    public $table;
+    public $fieldnamesparent = 'parent';
+
+    /** @var array Records from db, only used in top level list. */
+    public $records = array();
+
+    public $editable;
+
+    /** @var array keys are child ids, values are parents. */
+    public $childparent;
 
 //------------------------------------------------------
 //vars used for pagination.
-    var $page = 0;// 0 means no pagination
-    var $firstitem = 1;
-    var $lastitem = 999999;
-    var $pagecount;
-    var $paged = false;
-    var $offset = 0;
+    public $page = 0; // 0 means no pagination
+    public $firstitem = 1;
+    public $lastitem = 999999;
+    public $pagecount;
+    public $paged = false;
+    public $offset = 0;
 //------------------------------------------------------
-    var $pageurl;
-    var $pageparamname;
+    public $pageurl;
+    public $pageparamname;
 
     /**
-     * Constructor function
+     * Constructor.
      *
      * @param string $type
      * @param string $attributes
@@ -99,9 +89,8 @@ class moodle_list {
      * @param integer $page if 0 no pagination. (These three params only used in top level list.)
      * @param string $pageparamname name of url param that is used for passing page no
      * @param integer $itemsperpage no of top level items.
-     * @return moodle_list
      */
-    function moodle_list($type='ul', $attributes='', $editable = false, $pageurl=null, $page = 0, $pageparamname = 'page', $itemsperpage = 20) {
+    public function __construct($type='ul', $attributes='', $editable = false, $pageurl=null, $page = 0, $pageparamname = 'page', $itemsperpage = 20) {
         global $PAGE;
 
         $this->editable = $editable;
@@ -123,7 +112,7 @@ class moodle_list {
      *
      * @param integer $indent depth of indentation.
      */
-    function to_html($indent=0, $extraargs=array()) {
+    public function to_html($indent=0, $extraargs=array()) {
         if (count($this->items)) {
             $tabs = str_repeat("\t", $indent);
             $first = true;
@@ -165,7 +154,7 @@ class moodle_list {
      * @param boolean $suppresserror error if not item found?
      * @return list_item *copy* or null if item is not found
      */
-    function find_item($id, $suppresserror = false) {
+    public function find_item($id, $suppresserror = false) {
         if (isset($this->items)) {
             foreach ($this->items as $key => $child) {
                 if ($child->id == $id) {
@@ -173,7 +162,7 @@ class moodle_list {
                 }
             }
             foreach (array_keys($this->items) as $key) {
-                $thischild =& $this->items[$key];
+                $thischild = $this->items[$key];
                 $ref = $thischild->children->find_item($id, true);//error always reported at top level
                 if ($ref !== null) {
                     return $ref;
@@ -187,12 +176,12 @@ class moodle_list {
         return null;
     }
 
-    function add_item(&$item) {
-        $this->items[] =& $item;
+    public function add_item($item) {
+        $this->items[] = $item;
     }
 
-    function set_parent(&$parent) {
-        $this->parentitem =& $parent;
+    public function set_parent($parent) {
+        $this->parentitem = $parent;
     }
 
     /**
@@ -206,7 +195,7 @@ class moodle_list {
      * @return array(boolean, integer) whether there is more than one page, $offset + how many toplevel items where there in this list.
      *
      */
-    function list_from_records($paged = false, $offset = 0) {
+    public function list_from_records($paged = false, $offset = 0) {
         $this->paged = $paged;
         $this->offset = $offset;
         $this->get_records();
@@ -222,44 +211,48 @@ class moodle_list {
         foreach ($records as $record) {
             $this->childparent[$record->id] = $record->parent;
         }
+
         //create top level list items and they're responsible for creating their children
         foreach ($records as $record) {
-            if (!array_key_exists($record->parent, $this->childparent)) {
-                //if this record is not a child of another record then
-
-                $inpage = ($itemiter >= $this->firstitem && $itemiter <= $this->lastitem);
-                //make list item for top level for all items
-                //we need the info about the top level items for reordering peers.
-                if ($this->parentitem!==null) {
-                    $newattributes = $this->parentitem->attributes;
-                } else {
-                    $newattributes = '';
+            if (array_key_exists($record->parent, $this->childparent)) {
+                // This record is a child of another record, so it will be dealt
+                // with by a call to list_item::create_children, not here.
+                continue;
+            }
 
-                }
-                $this->items[$itemiter] = new $this->listitemclassname($record, $this, $newattributes, $inpage);
-                if ($inpage) {
-                    $this->items[$itemiter]->create_children($records, $this->childparent, $record->id);
-                } else {
-                    //don't recurse down the tree for items that are not on this page
-                    $this->paged = true;
-                }
-                $itemiter++;
+            $inpage = $itemiter >= $this->firstitem && $itemiter <= $this->lastitem;
+
+            // Make list item for top level for all items
+            // we need the info about the top level items for reordering peers.
+            if ($this->parentitem !== null) {
+                $newattributes = $this->parentitem->attributes;
+            } else {
+                $newattributes = '';
             }
+
+            $this->items[$itemiter] = new $this->listitemclassname($record, $this, $newattributes, $inpage);
+
+            if ($inpage) {
+                $this->items[$itemiter]->create_children($records, $this->childparent, $record->id);
+            } else {
+                // Don't recurse down the tree for items that are not on this page
+                $this->paged = true;
+            }
+
+            $itemiter++;
         }
         return array($this->paged, $itemiter);
     }
 
     /**
      * Should be overriden to return an array of records of list items.
-     *
      */
-    function get_records() {
-    }
+    public abstract function get_records();
 
     /**
      * display list of page numbers for navigation
      */
-    function display_page_numbers() {
+    public function display_page_numbers() {
         $html = '';
         $topcount = count($this->items);
         $this->pagecount = (integer) ceil(($topcount + $this->offset)/ QUESTION_PAGE_LENGTH );
@@ -285,7 +278,7 @@ class moodle_list {
      * @param    int itemid - if given, restrict records to those with this parent id.
      * @return   array peer ids
      */
-    function get_items_peers($itemid) {
+    public function get_items_peers($itemid) {
         $itemref = $this->find_item($itemid);
         $peerids = $itemref->parentlist->get_child_ids();
         return $peerids;
@@ -296,7 +289,7 @@ class moodle_list {
      *
      * @return   array peer ids
      */
-    function get_child_ids() {
+    public function get_child_ids() {
         $childids = array();
         foreach ($this->items as $child) {
            $childids[] = $child->id;
@@ -310,7 +303,7 @@ class moodle_list {
      * @param string $direction up / down
      * @param integer $id
      */
-    function move_item_up_down($direction, $id) {
+    public function move_item_up_down($direction, $id) {
         $peers = $this->get_items_peers($id);
         $itemkey = array_search($id, $peers);
         switch ($direction) {
@@ -337,7 +330,7 @@ class moodle_list {
         $this->reorder_peers($peers);
     }
 
-    function reorder_peers($peers) {
+    public function reorder_peers($peers) {
         global $DB;
         foreach ($peers as $key => $peer) {
             $DB->set_field($this->table, "sortorder", $key, array("id"=>$peer));
@@ -348,7 +341,7 @@ class moodle_list {
      * @param integer $id an item index.
      * @return object the item that used to be the parent of the item moved.
      */
-    function move_item_left($id) {
+    public function move_item_left($id) {
         global $DB;
 
         $item = $this->find_item($id);
@@ -374,7 +367,7 @@ class moodle_list {
      *
      * @param integer $id
      */
-    function move_item_right($id) {
+    public function move_item_right($id) {
         global $DB;
 
         $peers = $this->get_items_peers($id);
@@ -403,7 +396,7 @@ class moodle_list {
      * @param integer $movedown id of item to move down
      * @return unknown
      */
-    function process_actions($left, $right, $moveup, $movedown) {
+    public function process_actions($left, $right, $moveup, $movedown) {
         //should this action be processed by this list object?
         if (!(array_key_exists($left, $this->records) || array_key_exists($right, $this->records) || array_key_exists($moveup, $this->records) || array_key_exists($movedown, $this->records))) {
             return false;
@@ -448,7 +441,7 @@ class moodle_list {
      * @return boolean Is the item with the given id the first top-level item on
      * the current page?
      */
-    function item_is_first_on_page($itemid) {
+    public function item_is_first_on_page($itemid) {
         return $this->page && isset($this->items[$this->firstitem]) &&
                 $itemid == $this->items[$this->firstitem]->id;
     }
@@ -458,7 +451,7 @@ class moodle_list {
      * @return boolean Is the item with the given id the last top-level item on
      * the current page?
      */
-    function item_is_last_on_page($itemid) {
+    public function item_is_last_on_page($itemid) {
         return $this->page && isset($this->items[$this->lastitem]) &&
                 $itemid == $this->items[$this->lastitem]->id;
     }
@@ -469,46 +462,36 @@ class moodle_list {
  * @copyright Jamie Pratt
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class list_item {
-    /**
-     * id of record, used if list is editable
-     * @var integer
-     */
-    var $id;
-    /**
-     * name of this item, used if list is editable
-     * @var string
-     */
-    var $name;
-    /**
-     * The object or string representing this item.
-     * @var mixed
-     */
-    var $item;
-    var $fieldnamesname = 'name';
-    var $attributes;
-    var $display;
-    var $icons = array();
-    /**
-     * @var moodle_list
-     */
-    var $parentlist;
-    /**
-     * Set if there are any children of this listitem.
-     * @var moodle_list
-     */
-    var $children;
+abstract class list_item {
+    /** @var integer id of record, used if list is editable. */
+    public $id;
+
+    /** @var string name of this item, used if list is editable. */
+    public $name;
+
+    /** @var mixed The object or string representing this item. */
+    public $item;
+    public $fieldnamesname = 'name';
+    public $attributes;
+    public $display;
+    public $icons = array();
+
+    /** @var moodle_list */
+    public $parentlist;
+
+    /** @var moodle_list Set if there are any children of this listitem. */
+    public $children;
 
     /**
      * Constructor
      * @param mixed $item fragment of html for list item or record
-     * @param object &$parent reference to parent of this item
+     * @param object $parent reference to parent of this item
      * @param string $attributes attributes for li tag
      * @param boolean $display whether this item is displayed. Some items may be loaded so we have a complete
      *                              structure in memory to work with for actions but are not displayed.
      * @return list_item
      */
-    function list_item($item, &$parent, $attributes='', $display = true) {
+    public function __construct($item, $parent, $attributes = '', $display = true) {
         $this->item = $item;
         if (is_object($this->item)) {
             $this->id = $this->item->id;
@@ -526,7 +509,7 @@ class list_item {
      * Output the html just for this item. Called by to_html which adds html for children.
      *
      */
-    function item_html($extraargs = array()) {
+    public function item_html($extraargs = array()) {
         if (is_string($this->item)) {
             $html = $this->item;
         } elseif (is_object($this->item)) {
@@ -545,7 +528,7 @@ class list_item {
      *                            may be used by sub class.
      * @return string html
      */
-    function to_html($indent=0, $extraargs = array()) {
+    public function to_html($indent = 0, $extraargs = array()) {
         if (!$this->display) {
             return '';
         }
@@ -559,14 +542,14 @@ class list_item {
         return $this->item_html($extraargs).'&nbsp;'.(join($this->icons, '')).(($childrenhtml !='')?("\n".$childrenhtml):'');
     }
 
-    function set_icon_html($first, $last, &$lastitem) {
+    public function set_icon_html($first, $last, $lastitem) {
         global $CFG;
         $strmoveup = get_string('moveup');
         $strmovedown = get_string('movedown');
         $strmoveleft = get_string('maketoplevelitem', 'question');
 
         if (isset($this->parentlist->parentitem)) {
-            $parentitem =& $this->parentlist->parentitem;
+            $parentitem = $this->parentlist->parentitem;
             if (isset($parentitem->parentlist->parentitem)) {
                 $action = get_string('makechildof', 'question', $parentitem->parentlist->parentitem->name);
             } else {
@@ -601,13 +584,13 @@ class list_item {
         }
     }
 
-    function image_icon($action, $url, $icon) {
+    public function image_icon($action, $url, $icon) {
         global $OUTPUT;
         return '<a title="' . s($action) .'" href="'.$url.'">
                 <img src="' . $OUTPUT->pix_url('t/'.$icon) . '" class="iconsmall" alt="' . s($action). '" /></a> ';
     }
 
-    function image_spacer() {
+    public function image_spacer() {
         global $OUTPUT;
         return '<img src="' . $OUTPUT->pix_url('spacer') . '" class="iconsmall" alt="" />';
     }
@@ -619,20 +602,18 @@ class list_item {
      * @param array $children
      * @param integer $thisrecordid
      */
-    function create_children(&$records, &$children, $thisrecordid) {
+    public function create_children(&$records, &$children, $thisrecordid) {
         //keys where value is $thisrecordid
         $thischildren = array_keys($children, $thisrecordid);
-        if (count($thischildren)) {
-            foreach ($thischildren as $child) {
-                $thisclass = get_class($this);
-                $newlistitem = new $thisclass($records[$child], $this->children, $this->attributes);
-                $this->children->add_item($newlistitem);
-                $newlistitem->create_children($records, $children, $records[$child]->id);
-            }
+        foreach ($thischildren as $child) {
+            $thisclass = get_class($this);
+            $newlistitem = new $thisclass($records[$child], $this->children, $this->attributes);
+            $this->children->add_item($newlistitem);
+            $newlistitem->create_children($records, $children, $records[$child]->id);
         }
     }
 
-    function set_parent(&$parent) {
-        $this->parentlist =& $parent;
+    public function set_parent($parent) {
+        $this->parentlist = $parent;
     }
 }
index 0ebabc0..ac5903f 100644 (file)
@@ -30,8 +30,8 @@ class question_category_list extends moodle_list {
     public $context = null;
     public $sortby = 'parent, sortorder, name';
 
-    public function question_category_list($type='ul', $attributes='', $editable = false, $pageurl=null, $page = 0, $pageparamname = 'page', $itemsperpage = 20, $context = null){
-        parent::moodle_list('ul', '', $editable, $pageurl, $page, 'cpage', $itemsperpage);
+    public function __construct($type='ul', $attributes='', $editable = false, $pageurl=null, $page = 0, $pageparamname = 'page', $itemsperpage = 20, $context = null){
+        parent::__construct('ul', '', $editable, $pageurl, $page, 'cpage', $itemsperpage);
         $this->context = $context;
     }
 
@@ -59,8 +59,6 @@ class question_category_list extends moodle_list {
 }
 
 class question_category_list_item extends list_item {
-
-
     public function set_icon_html($first, $last, &$lastitem){
         global $CFG;
         $category = $this->item;
@@ -79,6 +77,7 @@ class question_category_list_item extends list_item {
                 get_string('shareincontext', 'question', print_context_name($this->parentlist->lastlist->context)), $url, 'up');
         }
     }
+
     public function item_html($extraargs = array()){
         global $CFG, $OUTPUT;
         $str = $extraargs['str'];
@@ -100,7 +99,6 @@ class question_category_list_item extends list_item {
 
         return $item;
     }
-
 }