From 45046f08e8c5710e86cb0aa41e59395c8a692ad6 Mon Sep 17 00:00:00 2001 From: Stig Lindqvist Date: Sun, 20 Jul 2014 11:46:15 +1200 Subject: [PATCH] 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 --- code/controllers/CMSSiteTreeFilter.php | 60 ++++++++++++++++++-------- code/model/SiteTree.php | 7 +-- code/model/VirtualPage.php | 8 +++- 3 files changed, 53 insertions(+), 22 deletions(-) diff --git a/code/controllers/CMSSiteTreeFilter.php b/code/controllers/CMSSiteTreeFilter.php index 16944856..54423f3b 100644 --- a/code/controllers/CMSSiteTreeFilter.php +++ b/code/controllers/CMSSiteTreeFilter.php @@ -35,7 +35,12 @@ abstract class CMSSiteTreeFilter extends Object { * @var String */ protected $childrenMethod = null; - + + /** + * @var string + */ + protected $numChildrenMethod = 'numChildren'; + /** * Returns a sorted array of all implementators of CMSSiteTreeFilter, suitable for use in a dropdown. * @@ -71,13 +76,21 @@ abstract class CMSSiteTreeFilter extends Object { } /** - * @return String Method on {@link Hierarchy} objects - * which is used to traverse into children relationships. + * Method on {@link Hierarchy} objects which is used to traverse into children relationships. + * + * @return String */ public function getChildrenMethod() { return $this->childrenMethod; } - + + /** + * Method on {@link Hierarchy} objects which is used find the number of children for a parent page + */ + public function getNumChildrenMethod() { + return $this->numChildrenMethod; + } + /** * @return Array Map of Page IDs to their respective ParentID values. */ @@ -101,17 +114,13 @@ abstract class CMSSiteTreeFilter extends Object { } while(!empty($parents)) { - $q = new SQLQuery(); - $q->setSelect(array('"ID"','"ParentID"')) - ->setFrom('"SiteTree"') - ->setWhere('"ID" in ('.implode(',',array_keys($parents)).')'); - + $q = Versioned::get_including_deleted('SiteTree', '"RecordID" in ('.implode(',',array_keys($parents)).')'); + $list = $q->map('ID', 'ParentID'); $parents = array(); - - foreach($q->execute() as $row) { - if ($row['ParentID']) $parents[$row['ParentID']] = true; - $this->_cache_ids[$row['ID']] = true; - $this->_cache_expanded[$row['ID']] = true; + foreach($list as $id => $parentID) { + if ($parentID) $parents[$parentID] = true; + $this->_cache_ids[$id] = true; + $this->_cache_expanded[$id] = true; } } } @@ -202,8 +211,16 @@ abstract class CMSSiteTreeFilter extends Object { * @subpackage content */ class CMSSiteTreeFilter_DeletedPages extends CMSSiteTreeFilter { - + + /** + * @var string + */ protected $childrenMethod = "AllHistoricalChildren"; + + /** + * @var string + */ + protected $numChildrenMethod = 'numHistoricalChildren'; static public function title() { return _t('CMSSiteTreeFilter_DeletedPages.Title', "All pages, including deleted"); @@ -302,8 +319,16 @@ class CMSSiteTreeFilter_StatusDraftPages extends CMSSiteTreeFilter { * @subpackage content */ class CMSSiteTreeFilter_StatusDeletedPages extends CMSSiteTreeFilter { - - protected $childrenMethod = "AllHistoricalChildren"; + + /** + * @var string + */ + protected $childrenMethod = "AllHistoricalChildren"; + + /** + * @var string + */ + protected $numChildrenMethod = 'numHistoricalChildren'; static public function title() { return _t('CMSSiteTreeFilter_StatusDeletedPages.Title', 'Deleted pages'); @@ -318,6 +343,7 @@ class CMSSiteTreeFilter_StatusDeletedPages extends CMSSiteTreeFilter { public function pagesIncluded() { $pages = Versioned::get_including_deleted('SiteTree'); $pages = $this->applyDefaultFilters($pages); + $pages = $pages->filterByCallback(function($page) { // Doesn't exist on either stage or live return $page->IsDeletedFromStage && !$page->ExistsOnLive; diff --git a/code/model/SiteTree.php b/code/model/SiteTree.php index 02c4f0c2..45eabc12 100644 --- a/code/model/SiteTree.php +++ b/code/model/SiteTree.php @@ -2732,13 +2732,14 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid return isset($stack[$level-1]) ? $stack[$level-1] : null; } - + /** * Return the CSS classes to apply to this node in the CMS tree * + * @param string $numChildrenMethod * @return string */ - public function CMSTreeClasses() { + public function CMSTreeClasses($numChildrenMethod="numChildren") { $classes = sprintf('class-%s', $this->class); if($this->HasBrokenFile || $this->HasBrokenLink) { $classes .= " BrokenLink"; @@ -2765,7 +2766,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid if($this->hasExtension('Translatable') && $controller->Locale != Translatable::default_locale() && !$this->isTranslation()) $classes .= " untranslated "; */ - $classes .= $this->markingClasses(); + $classes .= $this->markingClasses($numChildrenMethod); return $classes; } diff --git a/code/model/VirtualPage.php b/code/model/VirtualPage.php index e71627b7..460e66f9 100644 --- a/code/model/VirtualPage.php +++ b/code/model/VirtualPage.php @@ -387,8 +387,12 @@ class VirtualPage extends Page { $this->ImageTracking()->setByIdList($this->CopyContentFrom()->ImageTracking()->column('ID')); } - public function CMSTreeClasses() { - return parent::CMSTreeClasses() . ' VirtualPage-' . $this->CopyContentFrom()->ClassName; + /** + * @param string $numChildrenMethod + * @return string + */ + public function CMSTreeClasses($numChildrenMethod="numChildren") { + return parent::CMSTreeClasses($numChildrenMethod) . ' VirtualPage-' . $this->CopyContentFrom()->ClassName; } /**