Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

FIX Respect tree node limits, fix search result node display

- Renamed $minNodeCount to more accurate $nodeCountThreshold
- The $minNodeCount attribute wasn't properly respected
during actual querying, so SilverStripe would always traverse
the entire tree (and load all objects into memory),
before then marking nodes as "unexpanded", which prevents
them from actually being rendered.
- Fixes nodes on search results to be expanded by default
- Fixes nodes on search results to correctly ajax-expand
  • Loading branch information...
commit dd6f33ab37de4499efe24f1894ba1a104a55df04 1 parent 6931073
@chillu chillu authored
View
6 admin/code/LeftAndMain.php
@@ -691,7 +691,7 @@ public function SiteTreeAsUL() {
* @return String Nested unordered list with links to each page
*/
public function getSiteTreeFor($className, $rootID = null, $childrenMethod = null, $numChildrenMethod = null,
- $filterFunction = null, $minNodeCount = 30) {
+ $filterFunction = null, $nodeCountThreshold = 30) {
// Filter criteria
$params = $this->request->getVar('q');
@@ -719,7 +719,7 @@ public function getSiteTreeFor($className, $rootID = null, $childrenMethod = nul
// Mark the nodes of the tree to return
if ($filterFunction) $obj->setMarkingFilterFunction($filterFunction);
- $obj->markPartialTree($minNodeCount, $this, $childrenMethod, $numChildrenMethod);
+ $obj->markPartialTree($nodeCountThreshold, $this, $childrenMethod, $numChildrenMethod);
// Ensure current page is exposed
if($p = $this->currentPage()) $obj->markToExpose($p);
@@ -744,7 +744,7 @@ public function getSiteTreeFor($className, $rootID = null, $childrenMethod = nul
true,
$childrenMethod,
$numChildrenMethod,
- $minNodeCount
+ $nodeCountThreshold
);
// Wrap the root if needs be.
View
63 model/Hierarchy.php
@@ -74,12 +74,17 @@ public function validate(ValidationResult $validationResult) {
* @param string $childrenMethod The name of the method used to get children from each object
* @param boolean $rootCall Set to true for this first call, and then to false for calls inside the recursion. You
* should not change this.
- * @param int $minNodeCount
+ * @param int $nodeCountThreshold The lower bounds for the amount of nodes to mark. If set, the logic will expand
+ * nodes until it eaches at least this number, and then stops. Root nodes will always
+ * show regardless of this settting. Further nodes can be lazy-loaded via ajax.
+ * This isn't a hard limit. Example: On a value of 10, with 20 root nodes, each having
+ * 30 children, the actual node count will be 50 (all root nodes plus first expanded child).
+ *
* @return string
*/
public function getChildrenAsUL($attributes = "", $titleEval = '"<li>" . $child->Title', $extraArg = null,
$limitToMarked = false, $childrenMethod = "AllChildrenIncludingDeleted",
- $numChildrenMethod = "numChildren", $rootCall = true, $minNodeCount = 30) {
+ $numChildrenMethod = "numChildren", $rootCall = true, $nodeCountThreshold = 30) {
if($limitToMarked && $rootCall) {
$this->markingFinished($numChildrenMethod);
@@ -103,9 +108,25 @@ public function getChildrenAsUL($attributes = "", $titleEval = '"<li>" . $child-
if(!$limitToMarked || $child->isMarked()) {
$foundAChild = true;
$output .= (is_callable($titleEval)) ? $titleEval($child) : eval("return $titleEval;");
- $output .= "\n" .
- $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, $childrenMethod,
- $numChildrenMethod, false, $minNodeCount) . "</li>\n";
+ $output .= "\n";
+
+
+ $numChildren = $child->$numChildrenMethod();
+ if(
+ // Always traverse into opened nodes (they might be exposed as parents of search results)
+ $child->isExpanded()
+ // Only traverse into children if we haven't reached the maximum node count already.
+ // Otherwise, the remaining nodes are lazy loaded via ajax.
+ && $child->isMarked()
+ ) {
+ $output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, $childrenMethod,
+ $numChildrenMethod, false, $nodeCountThreshold);
+ }
+ elseif($child->isTreeOpened()) {
+ // Since we're not loading children, don't mark it as open either
+ $child->markClosed();
+ }
+ $output .= "</li>\n";
}
}
@@ -125,21 +146,23 @@ public function getChildrenAsUL($attributes = "", $titleEval = '"<li>" . $child-
* This method returns the number of nodes marked. After this method is called other methods
* can check isExpanded() and isMarked() on individual nodes.
*
- * @param int $minNodeCount The minimum amount of nodes to mark.
+ * @param int $nodeCountThreshold See {@link getChildrenAsUL()}
* @return int The actual number of nodes marked.
*/
- public function markPartialTree($minNodeCount = 30, $context = null,
+ public function markPartialTree($nodeCountThreshold = 30, $context = null,
$childrenMethod = "AllChildrenIncludingDeleted", $numChildrenMethod = "numChildren") {
- if(!is_numeric($minNodeCount)) $minNodeCount = 30;
+ if(!is_numeric($nodeCountThreshold)) $nodeCountThreshold = 30;
$this->markedNodes = array($this->owner->ID => $this->owner);
$this->owner->markUnexpanded();
// foreach can't handle an ever-growing $nodes list
while(list($id, $node) = each($this->markedNodes)) {
- $this->markChildren($node, $context, $childrenMethod, $numChildrenMethod);
- if($minNodeCount && sizeof($this->markedNodes) >= $minNodeCount) {
+ $children = $this->markChildren($node, $context, $childrenMethod, $numChildrenMethod);
+ if($nodeCountThreshold && sizeof($this->markedNodes) > $nodeCountThreshold) {
+ // Undo marking children as opened since they're lazy loaded
+ if($children) foreach($children as $child) $child->markClosed();
break;
}
}
@@ -200,6 +223,7 @@ public function markingFilterMatches($node) {
/**
* Mark all children of the given node that match the marking filter.
* @param DataObject $node Parent node.
+ * @return DataList
*/
public function markChildren($node, $context = null, $childrenMethod = "AllChildrenIncludingDeleted",
$numChildrenMethod = "numChildren") {
@@ -213,7 +237,13 @@ public function markChildren($node, $context = null, $childrenMethod = "AllChild
$node->markExpanded();
if($children) {
foreach($children as $child) {
- if(!$this->markingFilter || $this->markingFilterMatches($child)) {
+ $markingMatches = $this->markingFilterMatches($child);
+ // Filtered results should always show opened, since actual matches
+ // might be hidden by non-matching parent nodes.
+ if($this->markingFilter && $markingMatches) {
+ $child->markOpened();
+ }
+ if(!$this->markingFilter || $markingMatches) {
if($child->$numChildrenMethod()) {
$child->markUnexpanded();
} else {
@@ -223,6 +253,8 @@ public function markChildren($node, $context = null, $childrenMethod = "AllChild
}
}
}
+
+ return $children;
}
/**
@@ -350,6 +382,15 @@ public function markOpened() {
self::$marked[ClassInfo::baseDataClass($this->owner->class)][$this->owner->ID] = true;
self::$treeOpened[ClassInfo::baseDataClass($this->owner->class)][$this->owner->ID] = true;
}
+
+ /**
+ * Mark this DataObject's tree as closed.
+ */
+ public function markClosed() {
+ if(isset(self::$treeOpened[ClassInfo::baseDataClass($this->owner->class)][$this->owner->ID])) {
+ unset(self::$treeOpened[ClassInfo::baseDataClass($this->owner->class)][$this->owner->ID]);
+ }
+ }
/**
* Check if this DataObject is marked.
View
234 tests/model/HierarchyTest.php
@@ -180,6 +180,238 @@ public function testBreadcrumbs() {
$this->assertEquals('Obj 2 &raquo; Obj 2a &raquo; Obj 2aa', $obj2aa->getBreadcrumbs());
}
+ public function testGetChildrenAsUL() {
+ $obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1');
+ $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+ $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+ $obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa');
+
+ $nodeCountThreshold = 30;
+
+ $root = new HierarchyTest_Object();
+ $root->markPartialTree($nodeCountThreshold);
+ $html = $root->getChildrenAsUL(
+ "",
+ '"<li id=\"" . $child->ID . "\">" . $child->Title',
+ null,
+ false,
+ "AllChildrenIncludingDeleted",
+ "numChildren",
+ true, // rootCall
+ $nodeCountThreshold
+ );
+ $parser = new CSSContentParser($html);
+ $node2 = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2,
+ 'Contains root elements'
+ );
+ $node2a = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]' .
+ '/ul/li[@id="' . $obj2a->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2a,
+ 'Contains child elements (in correct nesting)'
+ );
+ $node2aa = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]' .
+ '/ul/li[@id="' . $obj2a->ID . '"]' .
+ '/ul/li[@id="' . $obj2aa->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2aa,
+ 'Contains grandchild elements (in correct nesting)'
+ );
+ }
+
+ public function testGetChildrenAsULMinNodeCount() {
+ $obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1');
+ $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+ $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+
+ // Set low enough that it should be fulfilled by root only elements
+ $nodeCountThreshold = 3;
+
+ $root = new HierarchyTest_Object();
+ $root->markPartialTree($nodeCountThreshold);
+ $html = $root->getChildrenAsUL(
+ "",
+ '"<li id=\"" . $child->ID . "\">" . $child->Title',
+ null,
+ false,
+ "AllChildrenIncludingDeleted",
+ "numChildren",
+ true,
+ $nodeCountThreshold
+ );
+ $parser = new CSSContentParser($html);
+ $node1 = $parser->getByXpath(
+ '//ul/li[@id="' . $obj1->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node1,
+ 'Contains root elements'
+ );
+ $node2 = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2,
+ 'Contains root elements'
+ );
+ $node2a = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]' .
+ '/ul/li[@id="' . $obj2a->ID . '"]'
+ );
+ $this->assertFalse(
+ (bool)$node2a,
+ 'Does not contains child elements because they exceed minNodeCount'
+ );
+ }
+
+ public function testGetChildrenAsULMinNodeCountWithMarkToExpose() {
+ $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+ $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+ $obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa');
+
+ // Set low enough that it should be fulfilled by root only elements
+ $nodeCountThreshold = 3;
+
+ $root = new HierarchyTest_Object();
+ $root->markPartialTree($nodeCountThreshold);
+
+ // Mark certain node which should be included regardless of minNodeCount restrictions
+ $root->markToExpose($obj2aa);
+
+ $html = $root->getChildrenAsUL(
+ "",
+ '"<li id=\"" . $child->ID . "\">" . $child->Title',
+ null,
+ false,
+ "AllChildrenIncludingDeleted",
+ "numChildren",
+ true,
+ $nodeCountThreshold
+ );
+ $parser = new CSSContentParser($html);
+ $node2 = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2,
+ 'Contains root elements'
+ );
+ $node2aa = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]' .
+ '/ul/li[@id="' . $obj2a->ID . '"]' .
+ '/ul/li[@id="' . $obj2aa->ID . '"]'
+ );
+ $this->assertTrue((bool)$node2aa);
+ }
+
+ public function testGetChildrenAsULMinNodeCountWithFilters() {
+ $obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1');
+ $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+ $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+ $obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa');
+
+ // Set low enough that it should fit all search matches without lazy loading
+ $nodeCountThreshold = 3;
+
+ $root = new HierarchyTest_Object();
+
+ // Includes nodes by filter regardless of minNodeCount restrictions
+ $root->setMarkingFilterFunction(function($record) use($obj2, $obj2a, $obj2aa) {
+ // Results need to include parent hierarchy, even if we just want to
+ // match the innermost node.
+ // var_dump($record->Title);
+ // var_dump(in_array($record->ID, array($obj2->ID, $obj2a->ID, $obj2aa->ID)));
+ return in_array($record->ID, array($obj2->ID, $obj2a->ID, $obj2aa->ID));
+ });
+ $root->markPartialTree($nodeCountThreshold);
+
+ $html = $root->getChildrenAsUL(
+ "",
+ '"<li id=\"" . $child->ID . "\">" . $child->Title',
+ null,
+ true, // limit to marked
+ "AllChildrenIncludingDeleted",
+ "numChildren",
+ true,
+ $nodeCountThreshold
+ );
+ $parser = new CSSContentParser($html);
+ $node1 = $parser->getByXpath(
+ '//ul/li[@id="' . $obj1->ID . '"]'
+ );
+ $this->assertFalse(
+ (bool)$node1,
+ 'Does not contain root elements which dont match the filter'
+ );
+ $node2aa = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]' .
+ '/ul/li[@id="' . $obj2a->ID . '"]' .
+ '/ul/li[@id="' . $obj2aa->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2aa,
+ 'Contains non-root elements which match the filter'
+ );
+ }
+
+ public function testGetChildrenAsULHardLimitsNodes() {
+ $obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1');
+ $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+ $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+ $obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa');
+
+ // Set low enough that it should fit all search matches without lazy loading
+ $nodeCountThreshold = 3;
+
+ $root = new HierarchyTest_Object();
+
+ // Includes nodes by filter regardless of minNodeCount restrictions
+ $root->setMarkingFilterFunction(function($record) use($obj2, $obj2a, $obj2aa) {
+ // Results need to include parent hierarchy, even if we just want to
+ // match the innermost node.
+ // var_dump($record->Title);
+ // var_dump(in_array($record->ID, array($obj2->ID, $obj2a->ID, $obj2aa->ID)));
+ return in_array($record->ID, array($obj2->ID, $obj2a->ID, $obj2aa->ID));
+ });
+ $root->markPartialTree($nodeCountThreshold);
+
+ $html = $root->getChildrenAsUL(
+ "",
+ '"<li id=\"" . $child->ID . "\">" . $child->Title',
+ null,
+ true, // limit to marked
+ "AllChildrenIncludingDeleted",
+ "numChildren",
+ true,
+ $nodeCountThreshold
+ );
+ $parser = new CSSContentParser($html);
+ $node1 = $parser->getByXpath(
+ '//ul/li[@id="' . $obj1->ID . '"]'
+ );
+ $this->assertFalse(
+ (bool)$node1,
+ 'Does not contain root elements which dont match the filter'
+ );
+ $node2aa = $parser->getByXpath(
+ '//ul/li[@id="' . $obj2->ID . '"]' .
+ '/ul/li[@id="' . $obj2a->ID . '"]' .
+ '/ul/li[@id="' . $obj2aa->ID . '"]'
+ );
+ $this->assertTrue(
+ (bool)$node2aa,
+ 'Contains non-root elements which match the filter'
+ );
+ }
+
}
class HierarchyTest_Object extends DataObject implements TestOnly {
@@ -191,4 +423,4 @@ class HierarchyTest_Object extends DataObject implements TestOnly {
'Hierarchy',
"Versioned('Stage', 'Live')",
);
-}
+}
Please sign in to comment.
Something went wrong with that request. Please try again.