BUG: CMS tree filters doesn't count the correct number of children for deleted pages

This is a bug that combines Hierarchy, Versioned and LeftAndMain admins and CMSSiteTreeFilters.

This bug can be reproduced by having a large site tree with enough deleted pages in it so it doesn't
pre load all the children pages when initially opening an admin. Filter by either 'All pages including deleted'
or 'Deleted pages'. For CMS users it will look like deleted pages are gone.

The solution involves a couple of smaller fixes in both CMS and framework modules.

1) Ensure that 'numHistoricalChildren' are used instead of 'numChildren' when dealing with deleted pages
2) LeftAndMain::currentPage() deletes all the 'marking' cache previously built up by Hierarchy::markPartialTree()
3) Use Versioned::get_included_deleted() instead of raw DB queries against the DataObject tables when calculating parents in CMSSiteTreeFilter

admin/code/LeftAndMain.php

@@ -799,7 +799,12 @@ class LeftAndMain extends Controller implements PermissionProvider {
 			? $filter->getChildrenMethod() 
 			: 'AllChildrenIncludingDeleted';
 
-		if(!$numChildrenMethod) $numChildrenMethod = 'numChildren';
+		if(!$numChildrenMethod) {
+			$numChildrenMethod = 'numChildren';
+			if($filter && $filter->getNumChildrenMethod()) {
+				$numChildrenMethod = $filter->getNumChildrenMethod();
+			}
+		}
 		if(!$filterFunction) $filterFunction = ($filter) ? array($filter, 'isPageIncluded') : null;
 
 		// Get the tree root
@@ -812,7 +817,10 @@ class LeftAndMain extends Controller implements PermissionProvider {
 		$obj->markPartialTree($nodeCountThreshold, $this, $childrenMethod, $numChildrenMethod);
 		
 		// Ensure current page is exposed
-		if($p = $this->currentPage()) $obj->markToExpose($p);
+		// This call flushes the Hierarchy::$marked cache when the current node is deleted
+		// @see CMSMain::getRecord()
+		// This will make it impossible to show children under a deleted parent page
+		// if($p = $this->currentPage()) $obj->markToExpose($p);
 		
 		// NOTE: SiteTree/CMSMain coupling :-(
 		if(class_exists('SiteTree')) {
@@ -823,9 +831,10 @@ class LeftAndMain extends Controller implements PermissionProvider {
 		// getChildrenAsUL is a flexible and complex way of traversing the tree
 		$controller = $this;
 		$recordController = ($this->stat('tree_class') == 'SiteTree') ?  singleton('CMSPageEditController') : $this;
-		$titleFn = function(&$child) use(&$controller, &$recordController) {
+		$titleFn = function(&$child, $numChildrenMethod) use(&$controller, &$recordController) {
 			$link = Controller::join_links($recordController->Link("show"), $child->ID);
-			return LeftAndMain_TreeNode::create($child, $link, $controller->isCurrentPage($child))->forTemplate();
+			$node = LeftAndMain_TreeNode::create($child, $link, $controller->isCurrentPage($child), $numChildrenMethod);
+			return $node->forTemplate();
 		};
 		
 		// Limit the amount of nodes shown for performance reasons.
@@ -1866,10 +1875,22 @@ class LeftAndMain_TreeNode extends ViewableData {
 	 */
 	protected $isCurrent;
 
-	public function __construct($obj, $link = null, $isCurrent = false) {
+	/**
+	 * @var string
+	 */
+	protected $numChildrenMethod;
+
+	/**
+	 * @param $obj
+	 * @param null $link
+	 * @param bool $isCurrent
+	 * @param $numChildrenMethod
+	 */
+	public function __construct($obj, $link = null, $isCurrent = false, $numChildrenMethod='numChildren') {
 		$this->obj = $obj;
 		$this->link = $link;
 		$this->isCurrent = $isCurrent;
+		$this->numChildrenMethod = $numChildrenMethod;
 	}
 
 	/**
@@ -1890,7 +1911,7 @@ class LeftAndMain_TreeNode extends ViewableData {
 	}
 
 	public function getClasses() {
-		$classes = $this->obj->CMSTreeClasses();
+		$classes = $this->obj->CMSTreeClasses($this->numChildrenMethod);
 		if($this->isCurrent) $classes .= " current";
 		$flags = $this->obj->hasMethod('getStatusFlags') ? $this->obj->getStatusFlags() : false;
 		if ($flags) {

model/Hierarchy.php

@@ -115,7 +115,8 @@ class Hierarchy extends DataExtension {
 		if($limitToMarked && $rootCall) {
 			$this->markingFinished($numChildrenMethod);
 		}
-		
+
+
 		if($nodeCountCallback) {
 			$nodeCountWarning = $nodeCountCallback($this->owner, $this->owner->$numChildrenMethod());	
 			if($nodeCountWarning) return $nodeCountWarning;
@@ -130,6 +131,7 @@ class Hierarchy extends DataExtension {
 		}
 
 		if($children) {
+
 			if($attributes) {
 				$attributes = " $attributes";
 			}
@@ -139,10 +141,15 @@ class Hierarchy extends DataExtension {
 			foreach($children as $child) {
 				if(!$limitToMarked || $child->isMarked()) {
 					$foundAChild = true;
-					$output .= (is_callable($titleEval)) ? $titleEval($child) : eval("return $titleEval;");
+					if(is_callable($titleEval)) {
+						$output .= $titleEval($child, $numChildrenMethod);
+					} else {
+						$output .= eval("return $titleEval;");
+					}
 					$output .= "\n";
 
 					$numChildren = $child->$numChildrenMethod();
+
 					if(
 						// Always traverse into opened nodes (they might be exposed as parents of search results)
 						$child->isExpanded()
@@ -158,7 +165,7 @@ class Hierarchy extends DataExtension {
 						} else {
 							$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();
@@ -256,7 +263,7 @@ class Hierarchy extends DataExtension {
 			return call_user_func($func, $node);
 		}
 	}
-	
+
 	/**
 	 * Mark all children of the given node that match the marking filter.
 	 * @param DataObject $node Parent node.
@@ -276,11 +283,6 @@ class Hierarchy extends DataExtension {
 			foreach($children as $child) {
 				$markingMatches = $this->markingFilterMatches($child);
 				if($markingMatches) {
-					// Filtered results should always show opened, since actual matches
-					// might be hidden by non-matching parent nodes.
-					if($this->markingFilter) {
-						$child->markOpened();
-					}
 					if($child->$numChildrenMethod()) {
 						$child->markUnexpanded();
 					} else {
@@ -315,14 +317,14 @@ class Hierarchy extends DataExtension {
 	 * 
 	 * @return string
 	 */
-	public function markingClasses() {
+	public function markingClasses($numChildrenMethod="numChildren") {
 		$classes = '';
 		if(!$this->isExpanded()) {
 			$classes .= " unexpanded";
 		}
 		
 		// Set jstree open state, or mark it as a leaf (closed) if there are no children
-		if(!$this->numChildren()) {
+		if(!$this->$numChildrenMethod()) {
 			$classes .= " jstree-leaf closed";
 		} elseif($this->isTreeOpened()) {
 			$classes .= " jstree-open";

tests/model/HierarchyTest.php

@@ -391,6 +391,97 @@ class HierarchyTest extends SapphireTest {
 		);
 	}
 
+	/**
+	 * This test checks that deleted ('archived') child pages don't set a css class on the parent
+	 * node that makes it look like it has children
+	 */
+	public function testGetChildrenAsULNodeDeletedOnLive() {
+		$obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+		$obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+		$obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa');
+		$obj2ab = $this->objFromFixture('HierarchyTest_Object', 'obj2b');
+
+		// delete all children under obj2
+		$obj2a->delete();
+		$obj2aa->delete();
+		$obj2ab->delete();
+		// Don't pre-load all children
+		$nodeCountThreshold = 1;
+
+		$childrenMethod = 'AllChildren';
+		$numChildrenMethod = 'numChildren';
+
+		$root = new HierarchyTest_Object();
+		$root->markPartialTree($nodeCountThreshold, null, $childrenMethod, $numChildrenMethod);
+
+		// As in LeftAndMain::getSiteTreeFor() but simpler and more to the point for testing purposes
+		$titleFn = function(&$child, $numChildrenMethod="") {
+			return '<li class="' . $child->markingClasses($numChildrenMethod).
+				'" id="' . $child->ID . '">"' . $child->Title;
+		};
+
+		$html = $root->getChildrenAsUL(
+			"",
+			$titleFn,
+			null,
+			true, // limit to marked
+			$childrenMethod,
+			$numChildrenMethod,
+			true,
+			$nodeCountThreshold
+		);
+
+		// Get the class attribute from the $obj2 node in the sitetree, class 'jstree-leaf' means it's a leaf node
+		$nodeClass = $this->getNodeClassFromTree($html, $obj2);
+		$this->assertEquals('jstree-leaf closed', $nodeClass, 'object2 should not have children in the sitetree');
+	}
+
+	/**
+	 * This test checks that deleted ('archived') child pages _do_ set a css class on the parent
+	 * node that makes it look like it has children when getting all children including deleted
+	 */
+	public function testGetChildrenAsULNodeDeletedOnStage() {
+		$obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
+		$obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a');
+		$obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa');
+		$obj2ab = $this->objFromFixture('HierarchyTest_Object', 'obj2b');
+
+		// delete all children under obj2
+		$obj2a->delete();
+		$obj2aa->delete();
+		$obj2ab->delete();
+		// Don't pre-load all children
+		$nodeCountThreshold = 1;
+
+		$childrenMethod = 'AllChildrenIncludingDeleted';
+		$numChildrenMethod = 'numHistoricalChildren';
+
+		$root = new HierarchyTest_Object();
+		$root->markPartialTree($nodeCountThreshold, null, $childrenMethod, $numChildrenMethod);
+
+		// As in LeftAndMain::getSiteTreeFor() but simpler and more to the point for testing purposes
+		$titleFn = function(&$child, $numChildrenMethod="") {
+			return '<li class="' . $child->markingClasses($numChildrenMethod).
+				'" id="' . $child->ID . '">"' . $child->Title;
+		};
+
+		$html = $root->getChildrenAsUL(
+			"",
+			$titleFn,
+			null,
+			true, // limit to marked
+			$childrenMethod,
+			$numChildrenMethod,
+			true,
+			$nodeCountThreshold
+		);
+
+		// Get the class attribute from the $obj2 node in the sitetree
+		$nodeClass = $this->getNodeClassFromTree($html, $obj2);
+		// Object2 can now be expanded
+		$this->assertEquals('unexpanded jstree-closed closed', $nodeClass, 'obj2 should have children in the sitetree');
+	}
+
 	/**
 	 * @param String $html  [description]
 	 * @param array $nodes Breadcrumb path as array
@@ -417,6 +508,25 @@ class HierarchyTest extends SapphireTest {
 		self::assertThat((bool)$match, self::isFalse(), $message);	
 	}
 
+	/**
+	 * Get the HTML class attribute from a node in the sitetree
+	 *
+	 * @param $html
+	 * @param $node
+	 * @return string
+	 */
+	protected function getNodeClassFromTree($html, $node) {
+		$parser = new CSSContentParser($html);
+		$xpath = '//ul/li[@id="' . $node->ID . '"]';
+		$object = $parser->getByXpath($xpath);
+
+		foreach($object[0]->attributes() as $key => $attr) {
+			if($key == 'class') {
+				return (string)$attr;
+			}
+		}
+		return '';
+	}
 }
 
 class HierarchyTest_Object extends DataObject implements TestOnly {
@@ -428,4 +538,8 @@ class HierarchyTest_Object extends DataObject implements TestOnly {
 		'Hierarchy',
 		"Versioned('Stage', 'Live')",
 	);
-}
+
+	public function cmstreeclasses() {
+		return $this->markingClasses();
+	}
+}