From 6dd1f96a6800aed8551762e33286a3edeebcab4e Mon Sep 17 00:00:00 2001 From: Divesh Pahuja Date: Tue, 3 Jan 2023 12:28:31 +0100 Subject: [PATCH] [DataObject\Document] getChildren & getSiblings shouldn't load the listing (#13899) * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves #11737 * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves #11737 * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves #11737 * [DataObject\Document] getChildren & getSiblings shouldn't load the listing - resolves #11737 --- .../src/OrderManager/V7/OrderManager.php | 2 +- .../09_Upgrade_Notes/README.md | 2 + ...ementing_Product_Information_Management.md | 2 +- lib/Navigation/Builder.php | 2 +- models/DataObject/AbstractObject.php | 68 +++++++++---------- models/DataObject/AbstractObject/Dao.php | 6 +- models/Document.php | 13 ++-- models/Document/Hardlink.php | 7 +- models/Document/Hardlink/Wrapper.php | 9 ++- models/Element/Recyclebin/Item.php | 8 +-- models/Element/Service.php | 4 +- phpstan-baseline.neon | 40 ----------- tests/Model/DataObject/ObjectTest.php | 8 +-- tests/Model/Document/DocumentTest.php | 7 +- tests/Model/Element/RecyclebinTest.php | 2 +- 15 files changed, 74 insertions(+), 106 deletions(-) diff --git a/bundles/EcommerceFrameworkBundle/src/OrderManager/V7/OrderManager.php b/bundles/EcommerceFrameworkBundle/src/OrderManager/V7/OrderManager.php index bd52013879b..a7318c1c165 100644 --- 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/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md b/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md index 046149f5093..475e5fa68c9 100644 --- a/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md +++ b/doc/Development_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/doc/Development_Documentation/26_Best_Practice/02_Implementing_Product_Information_Management.md b/doc/Development_Documentation/26_Best_Practice/02_Implementing_Product_Information_Management.md index 18872b812ea..1843b062f5b 100644 --- a/doc/Development_Documentation/26_Best_Practice/02_Implementing_Product_Information_Management.md +++ b/doc/Development_Documentation/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 index 14b8a8bba7c..e2e77bcd678 100644 --- 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 index 137ae8c4cfe..934ae4965db 100644 --- 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 index 89d0407fc5e..d8b81e77948 100644 --- 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 index ce33207b8a8..e6d3b774751 100644 --- 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 index ed65e90e191..d1887adf6df 100644 --- 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 index 43ab8b67705..c3023b7cdc1 100644 --- 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 index 3fa9bd3a5ee..ec744fb0875 100644 --- 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 index c41a8b24633..df1c728856b 100644 --- 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 index 46cdbaf00b0..efa838d2b95 100644 --- 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\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Email.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Folder.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Hardlink.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Link.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Page.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Printcontainer.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Printpage.php - - - - message: "#^Parameter \\#1 \\$children of method Pimcore\\\\Model\\\\Document\\:\\:setChildren\\(\\) expects array\\\\|null, array\\ given\\.$#" - count: 1 - path: models/Document/Hardlink/Wrapper/Snippet.php diff --git a/tests/Model/DataObject/ObjectTest.php b/tests/Model/DataObject/ObjectTest.php index 9b08d72cd72..2e2b4869566 100644 --- 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); diff --git a/tests/Model/Document/DocumentTest.php b/tests/Model/Document/DocumentTest.php index 68882a9c988..f96b0a21e8c 100644 --- a/tests/Model/Document/DocumentTest.php +++ b/tests/Model/Document/DocumentTest.php @@ -19,6 +19,7 @@ use Pimcore\Model\Document\Editable\Input; use Pimcore\Model\Document\Email; use Pimcore\Model\Document\Link; +use Pimcore\Model\Document\Listing; use Pimcore\Model\Document\Page; use Pimcore\Model\Document\PrintAbstract; use Pimcore\Model\Document\Printpage; @@ -343,9 +344,11 @@ public function testSetGetChildren() $parentDoc = TestHelper::createEmptyDocumentPage(); $childDoc = TestHelper::createEmptyDocumentPage('child1-', false); - $parentDoc->setChildren([$childDoc]); + $listing = new Listing(); + $listing->setData([$childDoc]); + $parentDoc->setChildren($listing); - $this->assertSame($parentDoc->getChildren()[0], $childDoc); + $this->assertSame($parentDoc->getChildren()->getDocuments()[0], $childDoc); } public function testDocumentSerialization() diff --git a/tests/Model/Element/RecyclebinTest.php b/tests/Model/Element/RecyclebinTest.php index d2a8ddc6754..632ca4556a1 100644 --- a/tests/Model/Element/RecyclebinTest.php +++ b/tests/Model/Element/RecyclebinTest.php @@ -117,7 +117,7 @@ public function testRecursiveObjectRecycleAndRestore() $recycledContent = unserialize($storage->read($recycledItems->current()->getStoreageFile())); $this->assertEquals($parentId, $recycledContent->getId(), 'Expected recycled parent object ID'); - $this->assertCount(1, $recycledContent->getChildren(DataObject::$types, true), 'Expected recycled child object'); + $this->assertCount(1, $recycledContent->getChildren(DataObject::$types, true)->getData(), 'Expected recycled child object'); //restore deleted items (parent + child) $recycledItems->current()->restore();