Merge pull request #10077 from chrometoasters/pulls/6127-hierarchy-query-class-fix

FIX Use owner class when querying for stage and live children
2024-10-22 14:05:37 +02:00 · 2021-10-03 15:13:13 +13:00 · 2021-10-03 15:13:13 +13:00 · 8b634d45cd
commit 8b634d45cd
parent 5379306e98 38fe326262
9 changed files with 248 additions and 26 deletions
--- 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);
--- 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;
@ -28,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).
     *
@ -284,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
@ -320,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.
     *
@ -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([
--- 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()
@ -52,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);
@ -83,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);
@ -137,11 +141,42 @@ 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);

-        // 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);
@ -179,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);
@ -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);
@ -272,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',
--- 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
--- 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);
        }
--- 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 */
@ -219,7 +258,7 @@ class HierarchyTest extends SapphireTest
        $this->assertEquals('Obj 2 &raquo; Obj 2a &raquo; Obj 2aa', $obj2aa->getBreadcrumbs());
    }

-    public function testNoHideFromHeirarchy()
+    public function testNoHideFromHierarchy()
    {
        /** @var HierarchyTest\HideTestObject $obj4 */
        $obj4 = $this->objFromFixture(HierarchyTest\HideTestObject::class, 'obj4');
@ -232,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',
--- 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
--- a/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestObject.php
+++ b/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestObject.php
@ -0,0 +1,30 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\HierarchyTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Versioned\Versioned;
+
+/**
+ * @mixin Versioned
+ */
+class HierarchyOnSubclassTestObject extends DataObject implements TestOnly
+{
+    private static $table_name = 'HierarchyOnSubclassTest_Object';
+
+    private static $db = [
+        'Title' => 'Varchar'
+    ];
+
+    private static $extensions = [
+        Versioned::class,
+    ];
+
+    private static $default_sort = 'Title ASC';
+
+    public function cmstreeclasses()
+    {
+        return $this->markingClasses();
+    }
+}
--- a/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestSubObject.php
+++ b/tests/php/ORM/HierarchyTest/HierarchyOnSubclassTestSubObject.php
@ -0,0 +1,18 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\HierarchyTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\Hierarchy\Hierarchy;
+
+/**
+ * @mixin Hierarchy
+ */
+class HierarchyOnSubclassTestSubObject extends HierarchyOnSubclassTestObject implements TestOnly
+{
+    private static $table_name = 'HierarchyOnSubclassTest_SubObject';
+
+    private static $extensions = [
+        Hierarchy::class,
+    ];
+}