BUG Fix CMSMain::getList to correctly respect filter result

Fixes #1064
CMSSiteTreeFilter refactored to allow SS_List of filtered pages to be returned

code/controllers/CMSMain.php

@@ -697,35 +697,40 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr
 		return $this->renderWith($this->getTemplatesWithSuffix('_ListView'));
 	}
 	
+	/**
+	 * Safely reconstruct a selected filter from a given set of query parameters
+	 *
+	 * @param array $params Query parameters to use
+	 * @return CMSSiteTreeFilter The filter class, or null if none present
+	 * @throws InvalidArgumentException if invalid filter class is passed.
+	 */
+	protected function getQueryFilter($params) {
+		if(empty($params['FilterClass'])) return null;
+		$filterClass = $params['FilterClass'];
+		if(!is_subclass_of($filterClass, 'CMSSiteTreeFilter')) {
+			throw new InvalidArgumentException("Invalid filter class passed: {$filterClass}");
+		}
+		return $filterClass::create($params);
+	}
+
 	/**
 	 * Returns the pages meet a certain criteria as {@see CMSSiteTreeFilter} or the subpages of a parent page
 	 * defaulting to no filter and show all pages in first level.
 	 * Doubles as search results, if any search parameters are set through {@link SearchForm()}.
 	 * 
-	 * @param Array $params Search filter criteria
-	 * @param Int $parentID Optional parent node to filter on (can't be combined with other search criteria)
+	 * @param array $params Search filter criteria
+	 * @param int $parentID Optional parent node to filter on (can't be combined with other search criteria)
 	 * @return SS_List
-	 * @throws Exception if invalid filter class is passed.
+	 * @throws InvalidArgumentException if invalid filter class is passed.
 	 */
-	public function getList($params, $parentID = 0) {
-		$list = new DataList($this->stat('tree_class'));
-		$filter = null;
-		$ids = array();
-		if(isset($params['FilterClass']) && $filterClass = $params['FilterClass']){
-			if(!is_subclass_of($filterClass, 'CMSSiteTreeFilter')) {
-				throw new Exception(sprintf('Invalid filter class passed: %s', $filterClass));
-			}
-			$filter = new $filterClass($params);
-			$filterOn = true;
-			foreach($pages=$filter->pagesIncluded() as $pageMap){
-				$ids[] = $pageMap['ID'];
-			}
-			if(count($ids)) $list = $list->where('"'.$this->stat('tree_class').'"."ID" IN ('.implode(",", $ids).')');
+	public function getList($params = array(), $parentID = 0) {
+		if($filter = $this->getQueryFilter($params)) {
+			return $filter->getFilteredPages();
 		} else {
-			$list = $list->filter("ParentID", is_numeric($parentID) ? $parentID : 0);
+			$list = DataList::create($this->stat('tree_class'));
+			$parentID = is_numeric($parentID) ? $parentID : 0;
+			return $list->filter("ParentID", $parentID);
 		}
-
-		return $list;
 	}
 	
 	public function ListViewForm() {

code/controllers/CMSSiteTreeFilter.php

@@ -92,9 +92,19 @@ abstract class CMSSiteTreeFilter extends Object {
 	}
 
 	/**
-	 * @return Array Map of Page IDs to their respective ParentID values.
+	 * Gets the list of filtered pages
+	 *
+	 * @see {@link SiteTree::getStatusFlags()}
+	 * @return SS_List
 	 */
-	public function pagesIncluded() {}
+	abstract public function getFilteredPages();
+
+	/**
+	 * @return array Map of Page IDs to their respective ParentID values.
+	 */
+	public function pagesIncluded() {
+		return $this->mapIDs($this->getFilteredPages());
+	}
 	
 	/**
 	 * Populate the IDs of the pages returned by pagesIncluded(), also including
@@ -231,15 +241,15 @@ class CMSSIteTreeFilter_PublishedPages extends CMSSiteTreeFilter {
 	 * Filters out all pages who's status who's status that doesn't exist on live
 	 *
 	 * @see {@link SiteTree::getStatusFlags()}
-	 * @return array
+	 * @return SS_List
 	 */
-	public function pagesIncluded() {
+	public function getFilteredPages() {
 		$pages = Versioned::get_including_deleted('SiteTree');
 		$pages = $this->applyDefaultFilters($pages);
 		$pages = $pages->filterByCallback(function($page) {
 			return $page->ExistsOnLive;
 		});
-		return $this->mapIDs($pages);
+		return $pages;
 	}
 }
 
@@ -267,10 +277,10 @@ class CMSSiteTreeFilter_DeletedPages extends CMSSiteTreeFilter {
 		return _t('CMSSiteTreeFilter_DeletedPages.Title', "All pages, including deleted");
 	}
 	
-	public function pagesIncluded() {
+	public function getFilteredPages() {
 		$pages = Versioned::get_including_deleted('SiteTree');
 		$pages = $this->applyDefaultFilters($pages);
-		return $this->mapIDs($pages);
+		return $pages;
 	}
 }
 
@@ -286,12 +296,12 @@ class CMSSiteTreeFilter_ChangedPages extends CMSSiteTreeFilter {
 		return _t('CMSSiteTreeFilter_ChangedPages.Title', "Changed pages");
 	}
 	
-	public function pagesIncluded() {
+	public function getFilteredPages() {
 		$pages = Versioned::get_by_stage('SiteTree', 'Stage');
 		$pages = $this->applyDefaultFilters($pages)
 			->leftJoin('SiteTree_Live', '"SiteTree_Live"."ID" = "SiteTree"."ID"')
 			->where('"SiteTree"."Version" > "SiteTree_Live"."Version"');
-		return $this->mapIDs($pages);
+		return $pages;
 	}	
 }
 
@@ -310,17 +320,16 @@ class CMSSiteTreeFilter_StatusRemovedFromDraftPages extends CMSSiteTreeFilter {
 	/**
 	 * Filters out all pages who's status is set to "Removed from draft".
 	 * 
-	 * @see {@link SiteTree::getStatusFlags()}
-	 * @return array
+	 * @return SS_List
 	 */
-	public function pagesIncluded() {
+	public function getFilteredPages() {
 		$pages = Versioned::get_including_deleted('SiteTree');
 		$pages = $this->applyDefaultFilters($pages);
 		$pages = $pages->filterByCallback(function($page) {
 			// If page is removed from stage but not live
 			return $page->IsDeletedFromStage && $page->ExistsOnLive;
 		});
-		return $this->mapIDs($pages);
+		return $pages;
 	}	
 }
 
@@ -340,16 +349,16 @@ class CMSSiteTreeFilter_StatusDraftPages extends CMSSiteTreeFilter {
 	 * Filters out all pages who's status is set to "Draft".
 	 * 
 	 * @see {@link SiteTree::getStatusFlags()}
-	 * @return array
+	 * @return SS_List
 	 */
-	public function pagesIncluded() {
+	public function getFilteredPages() {
 		$pages = Versioned::get_by_stage('SiteTree', 'Stage');
 		$pages = $this->applyDefaultFilters($pages);
 		$pages = $pages->filterByCallback(function($page) {
 			// If page exists on stage but not on live
 			return (!$page->IsDeletedFromStage && $page->IsAddedToStage);
 		});
-		return $this->mapIDs($pages);
+		return $pages;
 	}	
 }
 
@@ -379,9 +388,9 @@ class CMSSiteTreeFilter_StatusDeletedPages extends CMSSiteTreeFilter {
 	 * Filters out all pages who's status is set to "Deleted".
 	 * 
 	 * @see {@link SiteTree::getStatusFlags()}
-	 * @return array
+	 * @return SS_List
 	 */
-	public function pagesIncluded() {
+	public function getFilteredPages() {
 		$pages = Versioned::get_including_deleted('SiteTree');
 		$pages = $this->applyDefaultFilters($pages);
 
@@ -389,7 +398,7 @@ class CMSSiteTreeFilter_StatusDeletedPages extends CMSSiteTreeFilter {
 			// Doesn't exist on either stage or live
 			return $page->IsDeletedFromStage && !$page->ExistsOnLive;
 		});
-		return $this->mapIDs($pages);
+		return $pages;
 	}	
 }
 
@@ -407,13 +416,12 @@ class CMSSiteTreeFilter_Search extends CMSSiteTreeFilter {
 	 * Retun an array of maps containing the keys, 'ID' and 'ParentID' for each page to be displayed
 	 * in the search.
 	 * 
-	 * @return Array
+	 * @return SS_List
 	 */
-	public function pagesIncluded() {
-
+	public function getFilteredPages() {
 		// Filter default records
 		$pages = Versioned::get_by_stage('SiteTree', 'Stage');
 		$pages = $this->applyDefaultFilters($pages);
-		return $this->mapIDs($pages);
+		return $pages;
 	}
 }

tests/controller/CMSMainTest.php

@@ -349,6 +349,92 @@ class CMSMainTest extends FunctionalTest {
 			$this->assertEquals($controller->getResponse()->getStatusCode(), 302);
 		}
 	}
+	
+	/**
+	 * Tests filtering in {@see CMSMain::getList()}
+	 */
+	public function testGetList() {
+		$controller = new CMSMain();
+
+		// Test all pages (stage)
+		$pages = $controller->getList()->sort('Title');
+		$this->assertEquals(28, $pages->count());
+		$this->assertEquals(
+			array('Home', 'Page 1', 'Page 10', 'Page 11', 'Page 12'),
+			$pages->Limit(5)->column('Title')
+		);
+
+		// Change state of tree
+		$page1 = $this->objFromFixture('Page', 'page1');
+		$page3 = $this->objFromFixture('Page', 'page3');
+		$page11 = $this->objFromFixture('Page', 'page11');
+		$page12 = $this->objFromFixture('Page', 'page12');
+		// Deleted
+		$page1->doUnpublish();
+		$page1->delete();
+		// Live and draft
+		$page11->publish('Stage', 'Live');
+		// Live only
+		$page12->publish('Stage', 'Live');
+		$page12->delete();
+
+		// Re-test all pages (stage)
+		$pages = $controller->getList()->sort('Title');
+		$this->assertEquals(26, $pages->count());
+		$this->assertEquals(
+			array('Home', 'Page 10', 'Page 11', 'Page 13', 'Page 14'),
+			$pages->Limit(5)->column('Title')
+		);
+
+		// Test deleted page filter
+		$params = array(
+			'FilterClass' => 'CMSSiteTreeFilter_StatusDeletedPages'
+		);
+		$pages = $controller->getList($params);
+		$this->assertEquals(1, $pages->count());
+		$this->assertEquals(
+			array('Page 1'),
+			$pages->column('Title')
+		);
+
+		// Test live, but not on draft filter
+		$params = array(
+			'FilterClass' => 'CMSSiteTreeFilter_StatusRemovedFromDraftPages'
+		);
+		$pages = $controller->getList($params);
+		$this->assertEquals(1, $pages->count());
+		$this->assertEquals(
+			array('Page 12'),
+			$pages->column('Title')
+		);
+
+		// Test live pages filter
+		$params = array(
+			'FilterClass' => 'CMSSIteTreeFilter_PublishedPages'
+		);
+		$pages = $controller->getList($params);
+		$this->assertEquals(2, $pages->count());
+		$this->assertEquals(
+			array('Page 11', 'Page 12'),
+			$pages->column('Title')
+		);
+
+		// Test that parentID is ignored when filtering
+		$pages = $controller->getList($params, $page3->ID);
+		$this->assertEquals(2, $pages->count());
+		$this->assertEquals(
+			array('Page 11', 'Page 12'),
+			$pages->column('Title')
+		);
+
+		// Test that parentID is respected when not filtering
+		$pages = $controller->getList(array(), $page3->ID);
+		$this->assertEquals(2, $pages->count());
+		$this->assertEquals(
+			array('Page 3.1', 'Page 3.2'),
+			$pages->column('Title')
+		);
+	}
 }
 
 class CMSMainTest_ClassA extends Page implements TestOnly {