From 5b6a39e71a61fd595a913788c412b8e249611bff Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Fri, 16 Jun 2017 15:01:33 +1200 Subject: [PATCH] TreeDropDown performance boost. --- forms/TreeDropdownField.php | 70 +++++++++++++++++++++++++++++----- model/Hierarchy.php | 71 ++++++++++++++++++++++++++++++----- tests/model/HierarchyTest.php | 4 +- 3 files changed, 124 insertions(+), 21 deletions(-) diff --git a/forms/TreeDropdownField.php b/forms/TreeDropdownField.php index d3a7cbc8c..5fbee3e47 100644 --- a/forms/TreeDropdownField.php +++ b/forms/TreeDropdownField.php @@ -107,6 +107,13 @@ class TreeDropdownField extends FormField { $this->childrenMethod = 'ChildFolders'; $this->numChildrenMethod = 'numChildFolders'; } + else { + // AllChildren should be used as a default because + // if you use AllChildrenIncludingDeleted instead, the filters will be much more demanding + // this is because instead of working with DataList you will be working with ArrayList + // AllChildren should be always used when dealing with more pages than the node leaf limit + $this->childrenMethod = 'AllChildren'; + } $this->addExtraClass('single'); @@ -303,16 +310,57 @@ class TreeDropdownField extends FormField { if ($this->filterCallback || $this->search != "" ) $obj->setMarkingFilterFunction(array($this, "filterMarking")); - $obj->markPartialTree($nodeCountThreshold = 30, $context = null, - $this->childrenMethod, $this->numChildrenMethod); + // prepare a filter by ids, this is very useful when we have thousands of child pages under common parent + $filteredIds = ($this->search != '' && !is_null($this->searchIds) && is_array($this->searchIds)) + ? array_keys($this->searchIds) : array(); + + // collect IDs of currently selected pages (this includes hierarchy path) + $forceValue = $request->requestVar('forceValue'); + if (!is_null($forceValue) || $this->value) { + $fieldValue = (!is_null($forceValue) ? $forceValue : $this->value); + + if (($values = preg_split('/,\s*/', $fieldValue)) && count($values)) { + foreach ($values as $value) { + if (!$value || $value == 'unchanged') { + continue; + } + + $selectedObject = $this->objectForKey($value); + + // add all pages along the hierarchy path + $stack = $selectedObject->parentStack(); + foreach ($stack as $stackItem) { + $filteredIds[] = $stackItem->ID; + } + } + } + } + + // main node leaf limit configuration + $nodeCountThreshold = 30; + $obj->markPartialTree( + $nodeCountThreshold, $context = null, + $this->childrenMethod, $this->numChildrenMethod, $filteredIds + ); // allow to pass values to be selected within the ajax request - if( isset($_REQUEST['forceValue']) || $this->value ) { - $forceValue = ( isset($_REQUEST['forceValue']) ? $_REQUEST['forceValue'] : $this->value); - if(($values = preg_split('/,\s*/', $forceValue)) && count($values)) foreach($values as $value) { - if(!$value || $value == 'unchanged') continue; + if (!is_null($forceValue) || $this->value) { + $fieldValue = (!is_null($forceValue) ? $forceValue : $this->value); - $obj->markToExpose($this->objectForKey($value)); + if (($values = preg_split('/,\s*/', $fieldValue)) && count($values)) { + foreach ($values as $value) { + if (!$value || $value == 'unchanged') { + continue; + } + + $obj->markToExpose( + $this->objectForKey($value), + $this->childrenMethod, + $this->numChildrenMethod, + $nodeCountThreshold, + $filteredIds + ); + } } } @@ -337,7 +385,7 @@ class TreeDropdownField extends FormField { // Skip the check if we're filtering the tree, since its not clear how many children will // match the filter criteria until they're queried (and matched up with previously marked nodes). $nodeThresholdLeaf = Config::inst()->get('Hierarchy', 'node_threshold_leaf'); - if($nodeThresholdLeaf && !$this->filterCallback && !$this->search) { + if($nodeThresholdLeaf && !$this->filterCallback && !$this->search && count($filteredIds) == 0) { $className = $this->sourceObject; $nodeCountCallback = function($parent, $numChildren) use($className, $nodeThresholdLeaf) { if($className == 'SiteTree' && $parent->ID && $numChildren > $nodeThresholdLeaf) { @@ -361,7 +409,8 @@ class TreeDropdownField extends FormField { $this->numChildrenMethod, true, // root call null, - $nodeCountCallback + $nodeCountCallback, + $filteredIds ); return substr(trim($html), 4, -5); } else { @@ -374,7 +423,8 @@ class TreeDropdownField extends FormField { $this->numChildrenMethod, true, // root call null, - $nodeCountCallback + $nodeCountCallback, + $filteredIds ); return $html; } diff --git a/model/Hierarchy.php b/model/Hierarchy.php index c42b94be5..b853f473e 100644 --- a/model/Hierarchy.php +++ b/model/Hierarchy.php @@ -118,6 +118,7 @@ class Hierarchy extends DataExtension { * overload this function * @param bool $limitToMarked Display only marked children * @param string $childrenMethod The name of the method used to get children from each object + * @param string $numChildrenMethod The name of the instance method to call to count the object's children * @param bool $rootCall Set to true for this first call, and then to false for calls inside * the recursion. You should not change this. * @param int $nodeCountThreshold See {@link self::$node_threshold_total} @@ -125,12 +126,14 @@ class Hierarchy extends DataExtension { * intercept the query. Useful e.g. to avoid excessive children listings * (Arguments: $parent, $numChildren) * + * @param array $filteredIds list of ids that will be used to filter list items (reduce the result set before processing) + * * @return string */ public function getChildrenAsUL($attributes = "", $titleEval = '"
  • " . $child->Title', $extraArg = null, $limitToMarked = false, $childrenMethod = "AllChildrenIncludingDeleted", $numChildrenMethod = "numChildren", $rootCall = true, - $nodeCountThreshold = null, $nodeCountCallback = null) { + $nodeCountThreshold = null, $nodeCountCallback = null, array $filteredIds = array()) { if(!is_numeric($nodeCountThreshold)) { $nodeCountThreshold = Config::inst()->get('Hierarchy', 'node_threshold_total'); @@ -149,6 +152,18 @@ class Hierarchy extends DataExtension { if($this->owner->hasMethod($childrenMethod)) { $children = $this->owner->$childrenMethod($extraArg); + + // apply filter before any further processing to limit excessive amount of items + // filter is applied only after limit has been reached + if (count($filteredIds) > 0 && $nodeCountThreshold && $children->count() > $nodeCountThreshold) { + $children = $children->filter(array('ID' => $filteredIds)); + } + + // too many children, display nothing + if ($nodeCountThreshold && $children->count() > $nodeCountThreshold) { + $children = null; + } + } else { user_error(sprintf("Can't find the method '%s' on class '%s' for getting tree children", $childrenMethod, get_class($this->owner)), E_USER_ERROR); @@ -188,7 +203,7 @@ class Hierarchy extends DataExtension { $child->markClosed(); } else { $output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, - $childrenMethod, $numChildrenMethod, false, $nodeCountThreshold); + $childrenMethod, $numChildrenMethod, false, $nodeCountThreshold, null, $filteredIds); } } elseif($child->isTreeOpened()) { // Since we're not loading children, don't mark it as open either @@ -216,10 +231,14 @@ class Hierarchy extends DataExtension { * {@link isExpanded()} and {@link isMarked()} on individual nodes. * * @param int $nodeCountThreshold See {@link getChildrenAsUL()} + * @param mixed $context + * @param string $childrenMethod The name of the instance method to call to get the object's list of children + * @param string $numChildrenMethod The name of the instance method to call to count the object's children + * @param array $filteredIds list of ids that will be used to filter list items (reduce the result set before processing) * @return int The actual number of nodes marked. */ public function markPartialTree($nodeCountThreshold = 30, $context = null, - $childrenMethod = "AllChildrenIncludingDeleted", $numChildrenMethod = "numChildren") { + $childrenMethod = "AllChildrenIncludingDeleted", $numChildrenMethod = "numChildren", array $filteredIds = array()) { if(!is_numeric($nodeCountThreshold)) $nodeCountThreshold = 30; @@ -228,7 +247,19 @@ class Hierarchy extends DataExtension { // foreach can't handle an ever-growing $nodes list while(list($id, $node) = each($this->markedNodes)) { - $children = $this->markChildren($node, $context, $childrenMethod, $numChildrenMethod); + // first determine the number of children, if it's too high the tree view can't be used + // so will will not even bother to display the subtree + // this covers two cases: + // either no search filter is in use or search filter is in use but it's too broad + $numberOfChildren = $node->$numChildrenMethod(); + if ($nodeCountThreshold && $numberOfChildren > $nodeCountThreshold + && (count($filteredIds) == 0 || count($filteredIds) > $nodeCountThreshold)) { + break; + } + + $children = $this->markChildren( + $node, $context, $childrenMethod, $numChildrenMethod, $nodeCountThreshold, $filteredIds + ); if($nodeCountThreshold && sizeof($this->markedNodes) > $nodeCountThreshold) { // Undo marking children as opened since they're lazy loaded if($children) foreach($children as $child) $child->markClosed(); @@ -299,12 +330,20 @@ class Hierarchy extends DataExtension { * @param mixed $context * @param string $childrenMethod The name of the instance method to call to get the object's list of children * @param string $numChildrenMethod The name of the instance method to call to count the object's children + * @param int $nodeCountThreshold + * @param array $filteredIds list of ids that will be used to filter list items (reduce the result set before processing) * @return DataList */ public function markChildren($node, $context = null, $childrenMethod = "AllChildrenIncludingDeleted", - $numChildrenMethod = "numChildren") { + $numChildrenMethod = "numChildren", $nodeCountThreshold = null, array $filteredIds = array()) { + if($node->hasMethod($childrenMethod)) { $children = $node->$childrenMethod($context); + + // apply filter before any further processing + if (count($filteredIds) > 0 && $nodeCountThreshold && $children->count() > $nodeCountThreshold) { + $children = $children->filter(array('ID' => $filteredIds)); + } } else { user_error(sprintf("Can't find the method '%s' on class '%s' for getting tree children", $childrenMethod, get_class($node)), E_USER_ERROR); @@ -375,11 +414,18 @@ class Hierarchy extends DataExtension { * * @param int $id ID of parent node * @param bool $open If this is true, mark the parent node as opened + * @param string $childrenMethod The name of the instance method to call to get the object's list of children + * @param string $numChildrenMethod The name of the instance method to call to count the object's children + * @param int $nodeCountThreshold + * @param array $filteredIds list of ids that will be used to filter list items (reduce the result set before processing) * @return bool */ - public function markById($id, $open = false) { + public function markById($id, $open = false, $childrenMethod = "AllChildrenIncludingDeleted", + $numChildrenMethod = "numChildren", $nodeCountThreshold = null, array $filteredIds = array()) { if(isset($this->markedNodes[$id])) { - $this->markChildren($this->markedNodes[$id]); + $this->markChildren( + $this->markedNodes[$id], null, $childrenMethod, $numChildrenMethod, $nodeCountThreshold, $filteredIds + ); if($open) { $this->markedNodes[$id]->markOpened(); } @@ -393,12 +439,19 @@ class Hierarchy extends DataExtension { * Expose the given object in the tree, by marking this page and all it ancestors. * * @param DataObject $childObj + * @param string $childrenMethod The name of the instance method to call to get the object's list of children + * @param string $numChildrenMethod The name of the instance method to call to count the object's children + * @param int $nodeCountThreshold + * @param array $filteredIds list of ids that will be used to filter list items (reduce the result set before processing) */ - public function markToExpose($childObj) { + public function markToExpose($childObj, $childrenMethod = "AllChildrenIncludingDeleted", + $numChildrenMethod = "numChildren", $nodeCountThreshold = null, array $filteredIds = array()) { if(is_object($childObj)){ $stack = array_reverse($childObj->parentStack()); foreach($stack as $stackItem) { - $this->markById($stackItem->ID, true); + $this->markById( + $stackItem->ID, true, $childrenMethod, $numChildrenMethod, $nodeCountThreshold, $filteredIds + ); } } } diff --git a/tests/model/HierarchyTest.php b/tests/model/HierarchyTest.php index cc580ec61..b712c5df6 100644 --- a/tests/model/HierarchyTest.php +++ b/tests/model/HierarchyTest.php @@ -455,7 +455,7 @@ EOT; $obj2aa->delete(); $obj2ab->delete(); // Don't pre-load all children - $nodeCountThreshold = 1; + $nodeCountThreshold = 3; $childrenMethod = 'AllChildren'; $numChildrenMethod = 'numChildren'; @@ -500,7 +500,7 @@ EOT; $obj2aa->delete(); $obj2ab->delete(); // Don't pre-load all children - $nodeCountThreshold = 1; + $nodeCountThreshold = 3; $childrenMethod = 'AllChildrenIncludingDeleted'; $numChildrenMethod = 'numHistoricalChildren';