[DataObject\Document] getChildren & getSiblings shouldn't load the li…

…sting (pimcore#13899) * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves pimcore#11737 * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves pimcore#11737 * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves pimcore#11737 * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves pimcore#11737
michalananapps · Jan 3, 2023 · 6dd1f96 · 6dd1f96
1 parent 4e55d6e
commit 6dd1f96
Show file tree

Hide file tree

Showing 15 changed files with 74 additions and 106 deletions.
diff --git a/bundles/EcommerceFrameworkBundle/src/OrderManager/V7/OrderManager.php b/bundles/EcommerceFrameworkBundle/src/OrderManager/V7/OrderManager.php
@@ -626,7 +626,7 @@ protected function cleanupZombieOrderItems(AbstractOrder $order)
         }
 
         $orderItemChildren = $order->getChildren();
-        foreach ($orderItemChildren ?: [] as $orderItemChild) {
+        foreach ($orderItemChildren as $orderItemChild) {
             if ($orderItemChild instanceof AbstractOrderItem) {
                 if (!in_array($orderItemChild->getId(), $validItemIds)) {
                     if (!$orderItemChild->getDependencies()->getRequiredBy(null, 1)) {

diff --git a/...evelopment_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md b/...evelopment_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
@@ -120,6 +120,8 @@ Please make sure to set your preferred storage location ***before*** migration.
 - [Ecommerce] Elasticsearch 7 support was removed
 - [Ecommerce] Config option `es_client_params` in `index_service` was removed 
 - [ClassSavedInterface] Removed `method_exists` bc layer. Please add the corresponding `ClassSavedInterface` interface to your custom field definitions. For more details check the 10.6.0 patch notes.
+- [DataObjects\Documents] **BC Break**: Calling `getChildren/getSiblings` on `AbstractObject` or `Document` now returns unloaded listing. If the list is not traveresed immediately, then it is required to call `load()` explicitily.
+  Also, `setChildren` now accepts `Listing` as first parameter instead of array.
 - [Admin] Removed `adminer` as built-in database management tool.
 
 ## 10.6.0

diff --git a/...ocumentation/26_Best_Practice/02_Implementing_Product_Information_Management.md b/...ocumentation/26_Best_Practice/02_Implementing_Product_Information_Management.md
@@ -95,7 +95,7 @@ class AbstractProduct extends DataObject\Concrete
     public function getVariants(): array
     {
         $variantType = self::OBJECT_TYPE_VARIANT; //variant
-        $variants = $this->getChildren(array($variantType));
+        $variants = $this->getChildren(array($variantType))->load();
         return $variants;
     }
 }

diff --git a/lib/Navigation/Builder.php b/lib/Navigation/Builder.php
@@ -330,7 +330,7 @@ protected function getChildren(Document $parentDocument): array
     {
         // the intention of this function is mainly to be overridden in order to customize the behavior of the navigation
         // e.g. for custom filtering and other very specific use-cases
-        return $parentDocument->getChildren();
+        return $parentDocument->getChildren()->load();
     }
 
     /**

diff --git a/models/DataObject/AbstractObject.php b/models/DataObject/AbstractObject.php
@@ -466,12 +466,12 @@ protected static function typeMatch(AbstractObject $object): bool
     }
 
     /**
-     * @param array $objectTypes
-     * @param bool $includingUnpublished
-     *
-     * @return DataObject[]
+     * Returns a list of the children objects
      */
-    public function getChildren(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER], bool $includingUnpublished = false): array
+    public function getChildren(
+        array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER],
+        bool $includingUnpublished = false
+    ): Listing
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
 
@@ -483,10 +483,11 @@ public function getChildren(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self
                 $list->setOrderKey($this->getChildrenSortBy());
                 $list->setOrder($this->getChildrenSortOrder());
                 $list->setObjectTypes($objectTypes);
-                $this->children[$cacheKey] = $list->load();
-                $this->hasChildren[$cacheKey] = (bool) count($this->children[$cacheKey]);
+                $this->children[$cacheKey] = $list;
             } else {
-                $this->children[$cacheKey] = [];
+                $list = new Listing();
+                $list->setObjects([]);
+                $this->children[$cacheKey] = $list;
                 $this->hasChildren[$cacheKey] = false;
             }
         }
@@ -497,12 +498,11 @@ public function getChildren(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self
     /**
      * Quick test if there are children
      *
-     * @param array $objectTypes
-     * @param bool|null $includingUnpublished
-     *
-     * @return bool
      */
-    public function hasChildren(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER], bool $includingUnpublished = null): bool
+    public function hasChildren(
+        array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER],
+        bool $includingUnpublished = null
+    ): bool
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
 
@@ -514,14 +514,12 @@ public function hasChildren(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self
     }
 
     /**
-     * Get a list of the sibling documents
-     *
-     * @param array $objectTypes
-     * @param bool $includingUnpublished
-     *
-     * @return array
+     * Get a list of the sibling objects
      */
-    public function getSiblings(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER], bool $includingUnpublished = false): array
+    public function getSiblings(
+        array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER],
+        bool $includingUnpublished = false
+    ): Listing
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
 
@@ -536,10 +534,11 @@ public function getSiblings(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self
                 $list->setOrderKey('key');
                 $list->setObjectTypes($objectTypes);
                 $list->setOrder('asc');
-                $this->siblings[$cacheKey] = $list->load();
-                $this->hasSiblings[$cacheKey] = (bool) count($this->siblings[$cacheKey]);
+                $this->siblings[$cacheKey] = $list;
             } else {
-                $this->siblings[$cacheKey] = [];
+                $list = new Listing();
+                $list->setObjects([]);
+                $this->siblings[$cacheKey] = $list;
                 $this->hasSiblings[$cacheKey] = false;
             }
         }
@@ -550,12 +549,11 @@ public function getSiblings(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self
     /**
      * Returns true if the object has at least one sibling
      *
-     * @param array $objectTypes
-     * @param bool|null $includingUnpublished
-     *
-     * @return bool
      */
-    public function hasSiblings(array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER], bool $includingUnpublished = null): bool
+    public function hasSiblings(
+        array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER],
+        bool $includingUnpublished = null
+    ): bool
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
 
@@ -1011,23 +1009,23 @@ public function setChildrenSortBy(?string $childrenSortBy)
     }
 
     /**
-     * @param DataObject[]|null $children
-     * @param array $objectTypes
-     * @param bool $includingUnpublished
-     *
      * @return $this
      */
-    public function setChildren(?array $children, array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER], bool $includingUnpublished = false): static
+    public function setChildren(
+        ?Listing $children,
+        array $objectTypes = [self::OBJECT_TYPE_OBJECT, self::OBJECT_TYPE_VARIANT, self::OBJECT_TYPE_FOLDER],
+        bool $includingUnpublished = false
+    ): static
     {
         if ($children === null) {
             // unset all cached children
             $this->children = [];
             $this->hasChildren = [];
-        } elseif (is_array($children)) {
+        } else {
             //default cache key
             $cacheKey = $this->getListingCacheKey([$objectTypes, $includingUnpublished]);
             $this->children[$cacheKey] = $children;
-            $this->hasChildren[$cacheKey] = (bool) count($children);
+            $this->hasChildren[$cacheKey] = (bool) $children->count();
         }
 
         return $this;

diff --git a/models/DataObject/AbstractObject/Dao.php b/models/DataObject/AbstractObject/Dao.php
@@ -366,7 +366,11 @@ public function hasSiblings(array $objectTypes = [DataObject::OBJECT_TYPE_OBJECT
             $sql .= ' AND published = 1';
         }
 
-        $sql .= " AND `type` IN ('" . implode("','", $objectTypes) . "') LIMIT 1";
+        if (!empty($objectTypes)) {
+            $sql .= " AND `type` IN ('" . implode("','", $objectTypes) . "')";
+        }
+
+        $sql .= ' LIMIT 1';
 
         $c = $this->db->fetchOne($sql, $params);
 

diff --git a/models/Document.php b/models/Document.php
@@ -559,18 +559,18 @@ public function clearDependentCache(array $additionalTags = [])
     /**
      * set the children of the document
      *
-     * @param Document[]|null $children
+     * @param listing|null $children
      * @param bool $includingUnpublished
      *
      * @return $this
      */
-    public function setChildren(?array $children, bool $includingUnpublished = false): static
+    public function setChildren(?listing $children, bool $includingUnpublished = false): static
     {
         if ($children === null) {
             // unset all cached children
             $this->hasChildren = [];
             $this->children = [];
-        } elseif (is_array($children)) {
+        } else {
             $cacheKey = $this->getListingCacheKey([$includingUnpublished]);
             $this->children[$cacheKey] = $children;
             $this->hasChildren[$cacheKey] = (bool) count($children);
@@ -582,11 +582,8 @@ public function setChildren(?array $children, bool $includingUnpublished = false
     /**
      * Get a list of the children (not recursivly)
      *
-     * @param bool $includingUnpublished
-     *
-     * @return self[]
      */
-    public function getChildren(bool $includingUnpublished = false): array
+    public function getChildren(bool $includingUnpublished = false): Listing
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
 
@@ -597,7 +594,7 @@ public function getChildren(bool $includingUnpublished = false): array
                 $list->setCondition('parentId = ?', $this->getId());
                 $list->setOrderKey('index');
                 $list->setOrder('asc');
-                $this->children[$cacheKey] = $list->load();
+                $this->children[$cacheKey] = $list;
             } else {
                 $this->children[$cacheKey] = [];
             }

diff --git a/models/Document/Hardlink.php b/models/Document/Hardlink.php
@@ -167,7 +167,7 @@ public function getProperties(): array
     /**
      * {@inheritdoc}
      */
-    public function getChildren(bool $includingUnpublished = false): array
+    public function getChildren(bool $includingUnpublished = false): Listing
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
         if (!isset($this->children[$cacheKey])) {
@@ -183,11 +183,12 @@ public function getChildren(bool $includingUnpublished = false): array
                 }
             }
 
-            $children = array_merge($sourceChildren, $children);
+            $children->setData(array_merge($sourceChildren, $children->load()));
+
             $this->setChildren($children, $includingUnpublished);
         }
 
-        return $this->children[$cacheKey] ?? [];
+        return $this->children[$cacheKey];
     }
 
     /**

diff --git a/models/Document/Hardlink/Wrapper.php b/models/Document/Hardlink/Wrapper.php
@@ -17,6 +17,7 @@
 namespace Pimcore\Model\Document\Hardlink;
 
 use Pimcore\Model\Document;
+use Pimcore\Model\Document\Listing;
 
 /**
  * @internal
@@ -127,9 +128,9 @@ public function getProperty(string $name, bool $asContainer = false): mixed
     /**
      * @param bool $includingUnpublished
      *
-     * @return Document[]
+     * @return listing
      */
-    public function getChildren(bool $includingUnpublished = false): array
+    public function getChildren(bool $includingUnpublished = false): Listing
     {
         $cacheKey = $this->getListingCacheKey(func_get_args());
         if (!isset($this->children[$cacheKey])) {
@@ -147,7 +148,9 @@ public function getChildren(bool $includingUnpublished = false): array
                 }
             }
 
-            $this->setChildren($children, $includingUnpublished);
+            $listing = new Listing;
+            $listing->setData($children);
+            $this->setChildren($listing, $includingUnpublished);
         }
 
         return $this->children[$cacheKey];

diff --git a/models/Element/Recyclebin/Item.php b/models/Element/Recyclebin/Item.php
@@ -290,11 +290,9 @@ protected function doRecursiveRestore(Element\ElementInterface $element)
             } else {
                 $children = $element->getChildren();
             }
-            if (is_array($children)) {
-                foreach ($children as $child) {
-                    $child->setParentId($element->getId());
-                    $this->doRecursiveRestore($child);
-                }
+            foreach ($children as $child) {
+                $child->setParentId($element->getId());
+                $this->doRecursiveRestore($child);
             }
         }
     }

diff --git a/models/Element/Service.php b/models/Element/Service.php
@@ -671,7 +671,9 @@ protected function updateChildren(DataObject|Document|Asset\Folder $target, Elem
         }
         if (!$found) {
             $newElement = Element\Service::getElementById($new->getType(), $new->getId());
-            $target->setChildren(array_merge($target->getChildren(), [$newElement]));
+            $listing = $target->getChildren();
+            $listing->setData(array_merge($listing->getData(), [$newElement]));
+            $target->setChildren($listing);
         }
     }
 

diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -149,43 +149,3 @@ parameters:
 			message: "#^Left side of && is always true\\.$#"
 			count: 2
 			path: models/DataObject/ClassDefinition/Data/ManyToOneRelation.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Email.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Folder.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Hardlink.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Link.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Page.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Printcontainer.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Printpage.php
-
-		-
-			message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\<Pimcore\\\\Model\\\\Document\\>\\|null, array\\<int, Pimcore\\\\Model\\\\Document\\\\Hardlink\\\\Wrapper\\\\WrapperInterface\\> given\\.$#"
-			count: 1
-			path: models/Document/Hardlink/Wrapper/Snippet.php
diff --git a/tests/Model/DataObject/ObjectTest.php b/tests/Model/DataObject/ObjectTest.php
@@ -100,14 +100,14 @@ public function testCacheUnpublishedChildren()
         $firstChild->save();
 
         //without unpublished flag
-        $child = $parent->getChildren();
+        $child = $parent->getChildren()->load();
         $this->assertEquals(0, count($child), 'Expected no child');
 
         $hasChild = $parent->hasChildren();
         $this->assertFalse($hasChild, 'hasChild property should be false');
 
         //with unpublished flag
-        $child = $parent->getChildren([], true);
+        $child = $parent->getChildren([], true)->load();
         $this->assertEquals(1, count($child), 'Expected 1 child');
 
         $hasChild = $parent->hasChildren([], true);
@@ -134,14 +134,14 @@ public function testCacheUnpublishedSiblings()
         $secondChild->save();
 
         //without unpublished flag
-        $sibling = $firstChild->getSiblings();
+        $sibling = $firstChild->getSiblings()->load();
         $this->assertEquals(0, count($sibling), 'Expected no sibling');
 
         $hasSibling = $firstChild->hasSiblings();
         $this->assertFalse($hasSibling, 'hasSiblings property should be false');
 
         //with unpublished flag
-        $sibling = $firstChild->getSiblings([], true);
+        $sibling = $firstChild->getSiblings([], true)->load();
         $this->assertEquals(1, count($sibling), 'Expected 1 sibling');
 
         $hasSibling = $firstChild->hasSiblings([], true);