From 1e5414eac7ba9800b0848da2422f4d7033d562fc Mon Sep 17 00:00:00 2001 From: Michal Kleiner Date: Mon, 30 Aug 2021 19:02:03 +1200 Subject: [PATCH 1/3] FIX Use correct ancestor class when querying for stage and live children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The right ancestor class that has the Hierarchy extension applied needs to be used since it doesn’t have to be the class directly extending from DataObject, which means the ID and ParentID columns might be in different tables. --- src/ORM/Hierarchy/Hierarchy.php | 44 +++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index 9760155f0..b482bc025 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -4,6 +4,8 @@ namespace SilverStripe\ORM\Hierarchy; use SilverStripe\Admin\LeftAndMain; use SilverStripe\Control\Controller; +use SilverStripe\Core\ClassInfo; +use SilverStripe\Core\Extensible; use SilverStripe\ORM\DataList; use SilverStripe\ORM\SS_List; use SilverStripe\ORM\ValidationResult; @@ -333,7 +335,7 @@ class Hierarchy extends DataExtension 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); + self::prepopulate_numchildren_cache($this->getHierarchyBaseClass(), $idList); } $this->owner->extend('onPrepopulateTreeDataCache', $recordList, $options); @@ -407,25 +409,47 @@ class Hierarchy extends DataExtension && in_array($controller->getAction(), ["treeview", "listview", "getsubtree"]); } + /** + * Find the first class in the inheritance chain that has Hierarchy extension applied + * + * @return string + */ + private function getHierarchyBaseClass(): string + { + $ancestry = ClassInfo::ancestry($this->owner); + $ancestorClass = array_shift($ancestry); + while ($ancestorClass && !Extensible::has_extension($ancestorClass, self::class)) { + $ancestorClass = array_shift($ancestry); + } + + return $ancestorClass; + } + /** * Return children in the stage site. * * @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. + * @param bool $skipParentIDFilter Set to true to suppress the ParentID and ID where statements. * @return DataList */ 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(); - $baseTable = $this->owner->baseTable(); - $staged = DataObject::get($baseClass)->where(sprintf( + /** @var DataObject|Hierarchy $owner */ + $owner = $this->owner; + $hideFromHierarchy = $owner->config()->hide_from_hierarchy; + $hideFromCMSTree = $owner->config()->hide_from_cms_tree; + $class = $this->getHierarchyBaseClass(); + + $schema = DataObject::getSchema(); + $tableForParentID = $schema->tableForField($class, 'ParentID'); + $tableForID = $schema->tableForField($class, 'ID'); + + $staged = DataObject::get($class)->where(sprintf( '%s.%s <> %s.%s', - Convert::symbol2sql($baseTable), + Convert::symbol2sql($tableForParentID), Convert::symbol2sql("ParentID"), - Convert::symbol2sql($baseTable), + Convert::symbol2sql($tableForID), Convert::symbol2sql("ID") )); @@ -466,7 +490,7 @@ class Hierarchy extends DataExtension $hideFromHierarchy = $owner->config()->hide_from_hierarchy; $hideFromCMSTree = $owner->config()->hide_from_cms_tree; - $children = DataObject::get($owner->baseClass()) + $children = DataObject::get($this->getHierarchyBaseClass()) ->filter('ParentID', (int)$owner->ID) ->exclude('ID', (int)$owner->ID) ->setDataQueryParam([ From 7226d7fab6e3e10566892f5a2222d57de3266eae Mon Sep 17 00:00:00 2001 From: Michal Kleiner Date: Sat, 25 Sep 2021 00:17:25 +1200 Subject: [PATCH 2/3] ENH Add tests for Hierarchy extension when applied to a subclass --- tests/php/Forms/TreeDropdownFieldTest.php | 70 ++++++++++++++++++- tests/php/Forms/TreeDropdownFieldTest.yml | 26 +++++++ tests/php/ORM/HierarchyTest.php | 39 +++++++++++ tests/php/ORM/HierarchyTest.yml | 17 +++++ .../HierarchyOnSubclassTestObject.php | 30 ++++++++ .../HierarchyOnSubclassTestSubObject.php | 18 +++++ 6 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestObject.php create mode 100644 tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestSubObject.php diff --git a/tests/php/Forms/TreeDropdownFieldTest.php b/tests/php/Forms/TreeDropdownFieldTest.php index ec349fa0b..17de5c8d3 100644 --- a/tests/php/Forms/TreeDropdownFieldTest.php +++ b/tests/php/Forms/TreeDropdownFieldTest.php @@ -12,6 +12,8 @@ use SilverStripe\Forms\FieldList; use SilverStripe\Forms\Form; use SilverStripe\Forms\TreeDropdownField; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestObject; +use SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject; use SilverStripe\ORM\Tests\HierarchyTest\TestObject; class TreeDropdownFieldTest extends SapphireTest @@ -20,7 +22,9 @@ class TreeDropdownFieldTest extends SapphireTest protected static $fixture_file = 'TreeDropdownFieldTest.yml'; protected static $extra_dataobjects = [ - TestObject::class + TestObject::class, + HierarchyOnSubclassTestObject::class, + HierarchyOnSubclassTestSubObject::class, ]; public function testSchemaStateDefaults() @@ -137,6 +141,37 @@ class TreeDropdownFieldTest extends SapphireTest $this->assertEquals($expectedNodeIDs, $actualNodeIDs); } + public function testTreeSearchJsonFlatlistWithLowNodeThresholdUsingSubObject() + { + // Initialise our TreeDropDownField + $field = new TreeDropdownField('TestTree', 'Test tree - Hierarchy on subclass', HierarchyOnSubclassTestSubObject::class); + $field->config()->set('node_threshold_total', 2); + + // Search for all Test object matching our criteria + $request = new HTTPRequest( + 'GET', + 'url', + ['search' => 'SubObject', 'format' => 'json', 'flatList' => '1'] + ); + $request->setSession(new Session([])); + $response = $field->tree($request); + $tree = json_decode($response->getBody(), true); + $actualNodeIDs = array_column($tree['children'], 'id'); + + // Get the list of expected node IDs from the YML Fixture + $expectedNodeIDs = array_map( + function ($key) { + return $this->objFromFixture(HierarchyOnSubclassTestSubObject::class, $key)->ID; + }, + ['four', 'fourB', 'fourA2'] // Those are the identifiers of the object we expect our search to find + ); + + sort($actualNodeIDs); + sort($expectedNodeIDs); + + $this->assertEquals($expectedNodeIDs, $actualNodeIDs); + } + public function testTreeSearch() { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); @@ -233,6 +268,39 @@ class TreeDropdownFieldTest extends SapphireTest ); } + public function testTreeSearchUsingSubObject() + { + $field = new TreeDropdownField('TestTree', 'Test tree', HierarchyOnSubclassTestSubObject::class); + + // case-insensitive search against keyword 'SubObject' for objects that have Hierarchy extension + // applied to a class that doesn't directly inherit from DataObject + $request = new HTTPRequest('GET', 'url', ['search' => 'SubObject']); + $request->setSession(new Session([])); + $response = $field->tree($request); + $tree = $response->getBody(); + + $subObject1 = $this->objFromFixture(HierarchyOnSubclassTestSubObject::class, 'four'); + $subObject1ChildB = $this->objFromFixture(HierarchyOnSubclassTestSubObject::class, 'fourB'); + + $parser = new CSSContentParser($tree); + $cssPath = 'ul.tree li#selector-TestTree-' . $subObject1->ID . ' li#selector-TestTree-' . $subObject1ChildB->ID . ' a'; + $firstResult = $parser->getBySelector($cssPath); + $this->assertEquals( + $subObject1ChildB->Name, + (string)$firstResult[0], + $subObject1ChildB->Name . ' is found, nested under ' . $subObject1->Name + ); + + // other objects which don't contain the keyword 'SubObject' are not returned in search results + $subObject2 = $this->objFromFixture(HierarchyOnSubclassTestSubObject::class, 'five'); + $cssPath = 'ul.tree li#selector-TestTree-' . $subObject2->ID . ' a'; + $noResult = $parser->getBySelector($cssPath); + $this->assertEmpty( + $noResult, + $subObject2 . ' is not found' + ); + } + public function testReadonly() { $field = new TreeDropdownField('TestTree', 'Test tree', File::class); diff --git a/tests/php/Forms/TreeDropdownFieldTest.yml b/tests/php/Forms/TreeDropdownFieldTest.yml index 2207cc300..66ced1fc9 100644 --- a/tests/php/Forms/TreeDropdownFieldTest.yml +++ b/tests/php/Forms/TreeDropdownFieldTest.yml @@ -62,3 +62,29 @@ SilverStripe\ORM\Tests\HierarchyTest\TestObject: ParentID: =>SilverStripe\ORM\Tests\HierarchyTest\TestObject.twoA three: Title: Three MatchSearchCriteria +SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject: + four: + Title: Four SubObject + fourA: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.four + Title: Child A of Four + fourB: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.four + Title: Child B of Four SubObject + fourA1: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.fourA + Title: Child 1 of Child A of Four + fourA2: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.fourA + Title: Child 2 of Child A of Four SubObject + fourB1: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.fourB + Title: Child 1 of Child B of Four + fourB2: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.fourB + Title: Child 2 of Child B of Four + fourB3: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.fourB + Title: Child 3 of Child B of Four + five: + Title: Five diff --git a/tests/php/ORM/HierarchyTest.php b/tests/php/ORM/HierarchyTest.php index f4b859d99..f22a313ad 100644 --- a/tests/php/ORM/HierarchyTest.php +++ b/tests/php/ORM/HierarchyTest.php @@ -14,6 +14,8 @@ class HierarchyTest extends SapphireTest HierarchyTest\TestObject::class, HierarchyTest\HideTestObject::class, HierarchyTest\HideTestSubObject::class, + HierarchyTest\HierarchyOnSubclassTestObject::class, + HierarchyTest\HierarchyOnSubclassTestSubObject::class, ]; public static function getExtraDataObjects() @@ -145,6 +147,43 @@ class HierarchyTest extends SapphireTest ); } + public function testNumChildrenHierarchyOnSubclass() + { + /** @var HierarchyTest\HierarchyOnSubclassTestObject $obj5 */ + $obj5 = $this->objFromFixture(HierarchyTest\HierarchyOnSubclassTestObject::class, 'obj5'); + + $this->assertFalse( + $obj5->hasMethod('numChildren'), + 'numChildren() cannot be called on object without Hierarchy extension' + ); + + /** @var HierarchyTest\HierarchyOnSubclassTestSubObject $obj5a */ + $obj5a = $this->objFromFixture(HierarchyTest\HierarchyOnSubclassTestSubObject::class, 'obj5a'); + /** @var HierarchyTest\HierarchyOnSubclassTestSubObject $obj5b */ + $obj5b = $this->objFromFixture(HierarchyTest\HierarchyOnSubclassTestSubObject::class, 'obj5b'); + + $this->assertEquals(2, $obj5a->numChildren()); + $this->assertEquals(1, $obj5b->numChildren()); + + $obj5bChild2 = new HierarchyTest\HierarchyOnSubclassTestSubObject(); + $obj5bChild2->ParentID = $obj5b->ID; + $obj5bChild2->write(); + $this->assertEquals( + $obj5b->numChildren(false), + 2, + 'numChildren() caching can be disabled through method parameter' + ); + $obj5bChild3 = new HierarchyTest\HierarchyOnSubclassTestSubObject(); + $obj5bChild3->ParentID = $obj5b->ID; + $obj5bChild3->write(); + $obj5b->flushCache(); + $this->assertEquals( + $obj5b->numChildren(), + 3, + 'numChildren() caching can be disabled by flushCache()' + ); + } + public function testLoadDescendantIDListIntoArray() { /** @var HierarchyTest\TestObject $obj2 */ diff --git a/tests/php/ORM/HierarchyTest.yml b/tests/php/ORM/HierarchyTest.yml index cd2e66576..a8a9d8c3b 100644 --- a/tests/php/ORM/HierarchyTest.yml +++ b/tests/php/ORM/HierarchyTest.yml @@ -53,3 +53,20 @@ SilverStripe\ORM\Tests\HierarchyTest\HideTestObject: SilverStripe\ORM\Tests\HierarchyTest\HideTestSubObject: obj4b: Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HideTestObject.obj4 +SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestObject: + obj5: + Title: Obj 5 +SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject: + obj5a: + Title: Obj 5a + obj5b: + Title: Obj 5b + obj5aa: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.obj5a + Title: Obj 5aa + obj5ab: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.obj5a + Title: Obj 5ab + obj5ba: + Parent: =>SilverStripe\ORM\Tests\HierarchyTest\HierarchyOnSubclassTestSubObject.obj5b + Title: Obj 5ba diff --git a/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestObject.php b/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestObject.php new file mode 100644 index 000000000..48dfdd9a1 --- /dev/null +++ b/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestObject.php @@ -0,0 +1,30 @@ + 'Varchar' + ]; + + private static $extensions = [ + Versioned::class, + ]; + + private static $default_sort = 'Title ASC'; + + public function cmstreeclasses() + { + return $this->markingClasses(); + } +} diff --git a/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestSubObject.php b/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestSubObject.php new file mode 100644 index 000000000..427474942 --- /dev/null +++ b/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestSubObject.php @@ -0,0 +1,18 @@ + Date: Fri, 24 Sep 2021 22:29:43 +1200 Subject: [PATCH 3/3] MNT Fix minor typos --- src/Forms/TreeDropdownField.php | 2 +- src/ORM/Hierarchy/Hierarchy.php | 6 +++--- tests/php/Forms/TreeDropdownFieldTest.php | 10 +++++----- tests/php/ORM/HierarchyCachingTest.php | 8 ++++---- tests/php/ORM/HierarchyTest.php | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/Forms/TreeDropdownField.php b/src/Forms/TreeDropdownField.php index b27dae2e5..a6f21ec14 100644 --- a/src/Forms/TreeDropdownField.php +++ b/src/Forms/TreeDropdownField.php @@ -501,7 +501,7 @@ class TreeDropdownField extends FormField // Begin marking $markingSet->markPartialTree(); - // Explicitely mark our search results if necessary + // Explicitly mark our search results if necessary foreach ($this->searchIds as $id => $marked) { if ($marked) { $object = $this->objectForKey($id); diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index b482bc025..6f6d2e84c 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -30,7 +30,7 @@ class Hierarchy extends DataExtension { /** * The lower bounds for the amount of nodes to mark. If set, the logic will expand nodes until it reaches at least - * this number, and then stops. Root nodes will always show regardless of this settting. Further nodes can be + * this number, and then stops. Root nodes will always show regardless of this setting. 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). * @@ -286,7 +286,7 @@ class Hierarchy extends DataExtension /** * Return the number of direct children. By default, values are cached after the first invocation. Can be - * augumented by {@link augmentNumChildrenCountQuery()}. + * augmented by {@link augmentNumChildrenCountQuery()}. * * @param bool $cache Whether to retrieve values from cache * @return int @@ -322,7 +322,7 @@ class Hierarchy extends DataExtension /** * 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 + * In the case 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. * diff --git a/tests/php/Forms/TreeDropdownFieldTest.php b/tests/php/Forms/TreeDropdownFieldTest.php index 17de5c8d3..3d78c7ab3 100644 --- a/tests/php/Forms/TreeDropdownFieldTest.php +++ b/tests/php/Forms/TreeDropdownFieldTest.php @@ -56,7 +56,7 @@ class TreeDropdownFieldTest extends SapphireTest { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); - // case insensitive search against keyword 'sub' for folders + // case-insensitive search against keyword 'sub' for folders $request = new HTTPRequest('GET', 'url', ['search'=>'sub', 'format' => 'json']); $request->setSession(new Session([])); $response = $field->tree($request); @@ -87,7 +87,7 @@ class TreeDropdownFieldTest extends SapphireTest { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); - // case insensitive search against keyword 'sub' for folders + // case-insensitive search against keyword 'sub' for folders $request = new HTTPRequest('GET', 'url', ['search'=>'sub', 'format' => 'json', 'flatList' => '1']); $request->setSession(new Session([])); $response = $field->tree($request); @@ -176,7 +176,7 @@ class TreeDropdownFieldTest extends SapphireTest { $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); - // case insensitive search against keyword 'sub' for folders + // case-insensitive search against keyword 'sub' for folders $request = new HTTPRequest('GET', 'url', ['search'=>'sub']); $request->setSession(new Session([])); $response = $field->tree($request); @@ -214,7 +214,7 @@ class TreeDropdownFieldTest extends SapphireTest $field = new TreeDropdownField('TestTree', 'Test tree', File::class); - // case insensitive search against keyword 'sub' for files + // case-insensitive search against keyword 'sub' for files $request = new HTTPRequest('GET', 'url', ['search'=>'sub']); $request->setSession(new Session([])); $response = $field->tree($request); @@ -340,7 +340,7 @@ class TreeDropdownFieldTest extends SapphireTest $treeBaseID = $this->idFromFixture(Folder::class, 'folder1'); $field = new TreeDropdownField('TestTree', 'Test tree', Folder::class); - // getSchemaDataDefaults needs the field to be attach to a form + // getSchemaDataDefaults needs the field to be attached to a form new Form( null, 'mock', diff --git a/tests/php/ORM/HierarchyCachingTest.php b/tests/php/ORM/HierarchyCachingTest.php index 7f101fb50..2d3de8dfb 100644 --- a/tests/php/ORM/HierarchyCachingTest.php +++ b/tests/php/ORM/HierarchyCachingTest.php @@ -48,10 +48,10 @@ class HierachyCacheTest extends SapphireTest [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'], + [TestObject::class, 'obj3d', false, 0, 'Childless Sub object numChildren should be 0'], + [TestObject::class, 'obj3d', true, 0, 'Childless 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'] + [HideTestObject::class, 'obj4', true, 1, 'Hidden object should not be included in count when cache'] ]; } @@ -68,7 +68,7 @@ class HierachyCacheTest extends SapphireTest $this->assertEquals($expected, $actual, $message); if ($cache) { - // When caching is eanbled, try re-accessing the numChildren value to make sure it doesn't change. + // When caching is enabled, try re-accessing the numChildren value to make sure it doesn't change. $actual = $node->numChildren($cache); $this->assertEquals($expected, $actual, $message); } diff --git a/tests/php/ORM/HierarchyTest.php b/tests/php/ORM/HierarchyTest.php index f22a313ad..6d4676377 100644 --- a/tests/php/ORM/HierarchyTest.php +++ b/tests/php/ORM/HierarchyTest.php @@ -258,7 +258,7 @@ class HierarchyTest extends SapphireTest $this->assertEquals('Obj 2 » Obj 2a » Obj 2aa', $obj2aa->getBreadcrumbs()); } - public function testNoHideFromHeirarchy() + public function testNoHideFromHierarchy() { /** @var HierarchyTest\HideTestObject $obj4 */ $obj4 = $this->objFromFixture(HierarchyTest\HideTestObject::class, 'obj4'); @@ -271,7 +271,7 @@ class HierarchyTest extends SapphireTest $this->assertEquals($obj4->liveChildren()->Count(), 2); } - public function testHideFromHeirarchy() + public function testHideFromHierarchy() { HierarchyTest\HideTestObject::config()->update( 'hide_from_hierarchy',