From 20d891228f2fb3c95cd6ed2ee0252d3dcd9fe40f Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Tue, 2 Nov 2010 18:38:50 +0000 Subject: [PATCH] MDL-24256 question category editing was messed up. 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 | 237 +++++++++++++++++------------------- question/category_class.php | 8 +- 2 files changed, 112 insertions(+), 133 deletions(-) diff --git a/lib/listlib.php b/lib/listlib.php index ce70893d42e50..7a3979bf2d94f 100644 --- a/lib/listlib.php +++ b/lib/listlib.php @@ -44,53 +44,43 @@ * @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 @@ function moodle_list($type='ul', $attributes='', $editable = false, $pageurl=nul * * @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 @@ function to_html($indent=0, $extraargs=array()) { * @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 @@ function find_item($id, $suppresserror = false) { } } 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 @@ function find_item($id, $suppresserror = false) { 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 @@ function set_parent(&$parent) { * @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 @@ function list_from_records($paged = false, $offset = 0) { 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 @@ function display_page_numbers() { * @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 @@ function get_items_peers($itemid) { * * @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 @@ function get_child_ids() { * @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 @@ function move_item_up_down($direction, $id) { $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 @@ function reorder_peers($peers) { * @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 @@ function move_item_left($id) { * * @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 @@ function move_item_right($id) { * @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 @@ function process_actions($left, $right, $moveup, $movedown) { * @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 @@ function item_is_first_on_page($itemid) { * @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 @@ function item_is_last_on_page($itemid) { * @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 @@ function list_item($item, &$parent, $attributes='', $display = true) { * 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 @@ function item_html($extraargs = array()) { * 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 @@ function to_html($indent=0, $extraargs = array()) { return $this->item_html($extraargs).' '.(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 @@ function set_icon_html($first, $last, &$lastitem) { } } - function image_icon($action, $url, $icon) { + public function image_icon($action, $url, $icon) { global $OUTPUT; return ' ' . s($action). ' '; } - function image_spacer() { + public function image_spacer() { global $OUTPUT; return ''; } @@ -619,20 +602,18 @@ function image_spacer() { * @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; } } diff --git a/question/category_class.php b/question/category_class.php index 0ebabc0151236..ac5903fac5448 100644 --- a/question/category_class.php +++ b/question/category_class.php @@ -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 @@ public function process_actions($left, $right, $moveup, $movedown, $moveupcontex } 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 @@ public function set_icon_html($first, $last, &$lastitem){ 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 @@ public function item_html($extraargs = array()){ return $item; } - }