diff --git a/docs/en/04_Changelogs/4.3.0.md b/docs/en/04_Changelogs/4.3.0.md index fcaad6995..7068f2ead 100644 --- a/docs/en/04_Changelogs/4.3.0.md +++ b/docs/en/04_Changelogs/4.3.0.md @@ -4,6 +4,7 @@ - `DataList::column()` now returns all values and not just "distinct" values from a column as per the API docs - `DataList`, `ArrayList` and `UnsavedRalationList` all have `columnUnique()` method for fetching distinct column values + - Take care with `stageChildren()` overrides. `Hierarchy::numChildren() ` results will only make use of `stageChildren()` customisations that are applied to the base class and don't include record-specific behaviour. ## Upgrading {#upgrading} diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index b47775085..66d68e578 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -10,7 +10,10 @@ use SilverStripe\ORM\ValidationResult; use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataExtension; +use SilverStripe\ORM\DB; use SilverStripe\Versioned\Versioned; +use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Convert; use Exception; /** @@ -71,6 +74,15 @@ class Hierarchy extends DataExtension */ private static $hide_from_cms_tree = array(); + /** + * Used to enable or disable the prepopulation of the numchildren cache. + * Defaults to true. + * + * @config + * @var boolean + */ + private static $prepopulate_numchildren_cache = true; + /** * Prevent virtual page virtualising these fields * @@ -79,9 +91,17 @@ class Hierarchy extends DataExtension */ private static $non_virtual_fields = [ '_cache_children', - '_cache_numChildren', ]; + /** + * A cache used by numChildren(). + * Clear through {@link flushCache()}. + * version (int)0 means not on this stage. + * + * @var array + */ + protected static $cache_numChildren = []; + public static function get_extra_config($class, $extension, $args) { return array( @@ -271,11 +291,18 @@ class Hierarchy extends DataExtension */ public function numChildren($cache = true) { - // Load if caching + + $baseClass = $this->owner->baseClass(); + $cacheType = 'numChildren'; + $id = $this->owner->ID; + + // cached call if ($cache) { - $numChildren = $this->owner->_cache_numChildren; - if (isset($numChildren)) { - return $numChildren; + if (isset(self::$cache_numChildren[$baseClass][$cacheType][$id])) { + return self::$cache_numChildren[$baseClass][$cacheType][$id]; + } elseif (isset(self::$cache_numChildren[$baseClass][$cacheType]['_complete'])) { + // If the cache is complete and we didn't find our ID in the cache, it means this object is childless. + return 0; } } @@ -284,11 +311,89 @@ class Hierarchy extends DataExtension // Save if caching if ($cache) { - $this->owner->_cache_numChildren = $numChildren; + self::$cache_numChildren[$baseClass][$cacheType][$id] = $numChildren; } + return $numChildren; } + /** + * Pre-populate any appropriate caches prior to rendering a tree. + * This is used to allow for the efficient rendering of tree views, notably in the CMS. + * In the cace of Hierarchy, it caches numChildren values. Other extensions can provide an + * onPrepopulateTreeDataCache(DataList $recordList = null, array $options) methods to hook + * into this event as well. + * + * @param DataList|array $recordList The list of records to prepopulate caches for. Null for all records. + * @param array $options A map of hints about what should be cached. "numChildrenMethod" and + * "childrenMethod" are allowed keys. + */ + public function prepopulateTreeDataCache($recordList = null, array $options = []) + { + if (empty($options['numChildrenMethod']) || $options['numChildrenMethod'] === 'numChildren') { + $idList = is_array($recordList) ? $recordList : + ($recordList instanceof DataList ? $recordList->column('ID') : null); + self::prepopulate_numchildren_cache($this->owner->baseClass(), $idList); + } + + $this->owner->extend('onPrepopulateTreeDataCache', $recordList, $options); + } + + /** + * Pre-populate the cache for Versioned::get_versionnumber_by_stage() for + * a list of record IDs, for more efficient database querying. If $idList + * is null, then every record will be pre-cached. + * + * @param string $class + * @param string $stage + * @param array $idList + */ + public static function prepopulate_numchildren_cache($baseClass, $idList = null) + { + if (!Config::inst()->get(static::class, 'prepopulate_numchildren_cache')) { + return; + } + + /** @var Versioned|DataObject $singleton */ + $dummyObject = DataObject::singleton($baseClass); + $baseTable = $dummyObject->baseTable(); + + $idColumn = Convert::symbol2sql("{$baseTable}.ID"); + + // Get the stageChildren() result of a dummy object and break down into a generic query + $query = $dummyObject->stageChildren(true, true)->dataQuery()->query(); + + // optional ID-list filter + if ($idList) { + // Validate the ID list + foreach ($idList as $id) { + if (!is_numeric($id)) { + user_error( + "Bad ID passed to Versioned::prepopulate_numchildren_cache() in \$idList: " . $id, + E_USER_ERROR + ); + } + } + $query->addWhere(['"ParentID" IN (' . DB::placeholders($idList) . ')' => $idList]); + } + + $query->setOrderBy(null); + + $query->setSelect([ + '"ParentID"', + "COUNT(DISTINCT $idColumn) AS \"NumChildren\"", + ]); + $query->setGroupBy([Convert::symbol2sql("ParentID")]); + + $numChildren = $query->execute()->map(); + self::$cache_numChildren[$baseClass]['numChildren'] = $numChildren; + if (!$idList) { + // If all objects are being cached, mark this cache as complete + // to avoid counting children of childless object. + self::$cache_numChildren[$baseClass]['numChildren']['_complete'] = true; + } + } + /** * Checks if we're on a controller where we should filter. ie. Are we loading the SiteTree? * @@ -309,16 +414,28 @@ class Hierarchy extends DataExtension * * @param bool $showAll Include all of the elements, even those not shown in the menus. Only applicable when * extension is applied to {@link SiteTree}. + * @param bool $skipParentIDFilter Set to true to supress the ParentID and ID where statements. * @return DataList */ - public function stageChildren($showAll = false) + public function stageChildren($showAll = false, $skipParentIDFilter = false) { $hideFromHierarchy = $this->owner->config()->hide_from_hierarchy; $hideFromCMSTree = $this->owner->config()->hide_from_cms_tree; $baseClass = $this->owner->baseClass(); - $staged = DataObject::get($baseClass) - ->filter('ParentID', (int)$this->owner->ID) - ->exclude('ID', (int)$this->owner->ID); + $baseTable = $this->owner->baseTable(); + $staged = DataObject::get($baseClass)->where(sprintf( + '%s.%s <> %s.%s', + Convert::symbol2sql($baseTable), + Convert::symbol2sql("ParentID"), + Convert::symbol2sql($baseTable), + Convert::symbol2sql("ID") + )); + + if (!$skipParentIDFilter) { + // There's no filtering by ID if we don't have an ID. + $staged = $staged->filter('ParentID', (int)$this->owner->ID); + } + if ($hideFromHierarchy) { $staged = $staged->exclude('ClassName', $hideFromHierarchy); } @@ -439,6 +556,6 @@ class Hierarchy extends DataExtension public function flushCache() { $this->owner->_cache_children = null; - $this->owner->_cache_numChildren = null; + self::$cache_numChildren = []; } } diff --git a/tests/php/ORM/HierarchyCachingTest.php b/tests/php/ORM/HierarchyCachingTest.php new file mode 100644 index 000000000..311e38897 --- /dev/null +++ b/tests/php/ORM/HierarchyCachingTest.php @@ -0,0 +1,149 @@ +flushCache(); + } + + public static function setUpBeforeClass() + { + parent::setUpBeforeClass(); + HideTestObject::config()->update( + 'hide_from_hierarchy', + [ HideTestSubObject::class ] + ); + } + + public function cacheNumChildrenDataProvider() + { + return [ + [TestObject::class, 'obj1', false, 0, 'childless object should have a numChildren of 0'], + [TestObject::class, 'obj1', true, 0, 'childless object should have a numChildren of 0 when cache'], + [TestObject::class, 'obj2', false, 2, 'Root object numChildren should count direct children'], + [TestObject::class, 'obj2', true, 2, 'Root object numChildren should count direct children when cache'], + [TestObject::class, 'obj3a', false, 2, 'Sub object numChildren should count direct children'], + [TestObject::class, 'obj3a', true, 2, 'Sub object numChildren should count direct children when cache'], + [TestObject::class, 'obj3d', false, 0, 'Childess Sub object numChildren should be 0'], + [TestObject::class, 'obj3d', true, 0, 'Childess Sub object numChildren should be 0 when cache'], + [HideTestObject::class, 'obj4', false, 1, 'Hidden object should not be included in count'], + [HideTestObject::class, 'obj4', true, 1, 'Hidden object should not be included in couunt when cache'] + ]; + } + + + /** + * @dataProvider cacheNumChildrenDataProvider + */ + public function testNumChildrenCache($className, $identifier, $cache, $expected, $message) + { + $node = $this->objFromFixture($className, $identifier); + + $actual = $node->numChildren($cache); + + $this->assertEquals($expected, $actual, $message); + + if ($cache) { + // When caching is eanbled, try re-accessing the numChildren value to make sure it doesn't change. + $actual = $node->numChildren($cache); + $this->assertEquals($expected, $actual, $message); + } + } + + public function prepopulateCacheNumChildrenDataProvider() + { + return [ + [ + TestObject::class, [], + 'obj1', false, 0, 'childless object should have a numChildren of 0' + ], + [ + TestObject::class, [], + 'obj1', true, 0, 'childless object should have a numChildren of 0 when cache' + ], + [ + TestObject::class, [2], + 'obj1', false, 0, 'childless object should have a numChildren of 0' + ], + [ + TestObject::class, [2], + 'obj1', true, 0, 'childless object should have a numChildren of 0 when cache' + ], + [ + TestObject::class, [], + 'obj2', false, 2, 'Root object numChildren should count direct children' + ], + [ + TestObject::class, [], + 'obj2', true, 2, 'Root object numChildren should count direct children when cache' + ], + [ + TestObject::class, [2], + 'obj2', false, 2, 'Root object numChildren should count direct children' + ], + [ + TestObject::class, [2], + 'obj2', true, 2, 'Root object numChildren should count direct children when cache' + ], + [ + HideTestObject::class, [], + 'obj4', false, 1, 'Hidden object should not be included in count' + ], + [ + HideTestObject::class, [], + 'obj4', true, 1, 'Hidden object should not be included in count when cache' + ], + [ + HideTestObject::class, [2], + 'obj4', false, 1, 'Hidden object should not be included in count' + ], + [ + HideTestObject::class, [2], + 'obj4', true, 1, 'Hidden object should not be included in count when cache' + ] + ]; + } + + /** + * @dataProvider prepopulateCacheNumChildrenDataProvider + */ + public function testPrepopulatedNumChildrenCache( + $className, + $idList, + $identifier, + $cache, + $expected, + $message + ) { + DataObject::singleton($className)->prepopulateTreeDataCache($idList, ['numChildrenMethod' => 'numChildren']); + $node = $this->objFromFixture($className, $identifier); + + $actual = $node->numChildren($cache); + + $this->assertEquals($expected, $actual, $message); + } +}