From 1735fd6f075c7a9373475555c4b99d41c329b77a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Danae=CC=88=20Miller-Clendon?= Date: Fri, 26 Feb 2021 14:15:49 +1300 Subject: [PATCH 1/9] NEW: Stages differ recursive. --- src/RecursiveStagesService.php | 171 ++++++++++++++++++ src/Versioned.php | 14 ++ tests/php/RecursiveStagesServiceTest.php | 92 ++++++++++ tests/php/RecursiveStagesServiceTest.yml | 24 +++ .../ChildObject.php | 36 ++++ .../ColumnObject.php | 66 +++++++ .../GroupObject.php | 66 +++++++ .../PrimaryObject.php | 56 ++++++ 8 files changed, 525 insertions(+) create mode 100644 src/RecursiveStagesService.php create mode 100644 tests/php/RecursiveStagesServiceTest.php create mode 100644 tests/php/RecursiveStagesServiceTest.yml create mode 100644 tests/php/RecursiveStagesServiceTest/ChildObject.php create mode 100644 tests/php/RecursiveStagesServiceTest/ColumnObject.php create mode 100644 tests/php/RecursiveStagesServiceTest/GroupObject.php create mode 100644 tests/php/RecursiveStagesServiceTest/PrimaryObject.php diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php new file mode 100644 index 00000000..bd691b93 --- /dev/null +++ b/src/RecursiveStagesService.php @@ -0,0 +1,171 @@ +exists()) { + return false; + } + + $items = [$object]; + + // compare existing content + while ($item = array_shift($items)) { + if ($this->checkNeedPublishingItem($item)) { + return true; + } + + $relatedObjects = $this->findOwnedObjects($item, $mode); + $items = array_merge($items, $relatedObjects); + } + + // compare deleted content + $draftIdentifiers = $this->findOwnedIdentifiers($object, $mode, Versioned::DRAFT); + $liveIdentifiers = $this->findOwnedIdentifiers($object, $mode, Versioned::LIVE); + + return $draftIdentifiers !== $liveIdentifiers; + } + + /** + * Find all identifiers for owned objects + * + * @param DataObject $object + * @param string $mode + * @param string $stage + * @return array + */ + protected function findOwnedIdentifiers(DataObject $object, string $mode, string $stage): array + { + $ids = Versioned::withVersionedMode(function () use ($object, $mode, $stage): array { + Versioned::set_stage($stage); + + $object = DataObject::get_by_id($object->ClassName, $object->ID); + + if ($object === null) { + return []; + } + + $items = [$object]; + $ids = []; + + while ($object = array_shift($items)) { + $ids[] = implode('_', [$object->baseClass(), $object->ID]); + $relatedObjects = $this->findOwnedObjects($object, $mode); + $items = array_merge($items, $relatedObjects); + } + + return $ids; + }); + + sort($ids, SORT_STRING); + + return array_values($ids); + } + + /** + * This lookup will attempt to find "Strongly owned" objects + * such objects are unable to exist without the current object + * We will use "cascade_duplicates" setting for this purpose as we can assume that if an object needs to be + * duplicated along with the owner object, it uses the strong ownership relation + * + * "Weakly owned" objects could be looked up via "owns" setting + * Such objects can exist even without the owner objects as they are often used as shared objects + * managed independently of their owners + * + * @param DataObject $object + * @param string $mode + * @return array + */ + protected function findOwnedObjects(DataObject $object, string $mode): array + { + $ownershipType = $mode === self::OWNERSHIP_WEAK + ? 'owns' + : 'cascade_duplicates'; + + $relations = (array) $object->config()->get($ownershipType); + $relations = array_unique($relations); + $result = []; + + foreach ($relations as $relation) { + $relation = (string) $relation; + + if (!$relation) { + continue; + } + + $relationData = $object->{$relation}(); + + if ($relationData instanceof DataObject) { + if (!$relationData->exists()) { + continue; + } + + $result[] = $relationData; + + continue; + } + + if (!$relationData instanceof SS_List) { + continue; + } + + foreach ($relationData as $relatedObject) { + if (!$relatedObject instanceof DataObject || !$relatedObject->exists()) { + continue; + } + + $result[] = $relatedObject; + } + } + + return $result; + } + + /** + * @param DataObject|Versioned $item + * @return bool + */ + protected function checkNeedPublishingItem(DataObject $item): bool + { + if ($item->hasExtension(Versioned::class)) { + /** @var $item Versioned */ + return !$item->isPublished() || $item->stagesDiffer(); + } + + return false; + } +} diff --git a/src/Versioned.php b/src/Versioned.php index 6f28c515..d623b657 100644 --- a/src/Versioned.php +++ b/src/Versioned.php @@ -1997,6 +1997,20 @@ public function stagesDiffer() return (bool) $stagesDiffer; } + /** + * Determine if content differs on stages including nested objects + * $mode determines which relation will be used to traverse the ownership tree + * "strong" will use "cascade_duplicates" + * "weak" will use "owns" + * + * @param string $mode "strong" or "weak" + * @return bool + */ + public function stagesDifferRecursive(string $mode = RecursiveStagesService::OWNERSHIP_STRONG): bool + { + return RecursiveStagesService::singleton()->stagesDifferRecursive($this->owner, $mode); + } + /** * @param string $filter * @param string $sort diff --git a/tests/php/RecursiveStagesServiceTest.php b/tests/php/RecursiveStagesServiceTest.php new file mode 100644 index 00000000..b9cc43cf --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest.php @@ -0,0 +1,92 @@ + [ + Versioned::class, + ], + ColumnObject::class => [ + Versioned::class, + ], + GroupObject::class => [ + Versioned::class, + ], + ChildObject::class => [ + Versioned::class, + ], + ]; + + /** + * @param string $class + * @param string $identifier + * @param bool $delete + * @throws ValidationException + * @dataProvider objectsProvider + */ + public function testStageDiffersRecursive(string $class, string $identifier, bool $delete): void + { + /** @var PrimaryObject|Versioned $primaryItem */ + $primaryItem = $this->objFromFixture(PrimaryObject::class, 'primary-object-1'); + $primaryItem->publishRecursive(); + + $this->assertFalse($primaryItem->stagesDifferRecursive()); + + $record = $this->objFromFixture($class, $identifier); + + if ($delete) { + $record->delete(); + } else { + $record->Title = 'New Title'; + $record->write(); + } + + $this->assertTrue($primaryItem->stagesDifferRecursive()); + } + + public function testStageDiffersRecursiveWithInvalidObject(): void + { + /** @var PrimaryObject|Versioned $primaryItem */ + $primaryItem = PrimaryObject::create(); + + $this->assertFalse($primaryItem->stagesDifferRecursive()); + } + + public function objectsProvider(): array + { + return [ + [PrimaryObject::class, 'primary-object-1', false], + [ColumnObject::class, 'column-1', false], + [ColumnObject::class, 'column-1', true], + [GroupObject::class, 'group-1', false], + [GroupObject::class, 'group-1', true], + [ChildObject::class, 'child-object-1', false], + [ChildObject::class, 'child-object-1', true], + ]; + } +} diff --git a/tests/php/RecursiveStagesServiceTest.yml b/tests/php/RecursiveStagesServiceTest.yml new file mode 100644 index 00000000..6e35a701 --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest.yml @@ -0,0 +1,24 @@ +# site-config-1 +# -> primary-object-1 (top level publish object) +# --> column-1 +# ---> group-1 +# ----> child-object-1 + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\PrimaryObject: + primary-object-1: + Title: PrimaryObject1 + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject: + column-1: + Title: Column1 + PrimaryObject: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\PrimaryObject.primary-object-1 + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject: + group-1: + Title: Group1 + Column: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject.column-1 + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ChildObject: + child-object-1: + Title: Item1 + Group: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject.group-1 diff --git a/tests/php/RecursiveStagesServiceTest/ChildObject.php b/tests/php/RecursiveStagesServiceTest/ChildObject.php new file mode 100644 index 00000000..6eabe6d6 --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/ChildObject.php @@ -0,0 +1,36 @@ + 'Varchar(255)', + ]; + + /** + * @var array + */ + private static $has_one = [ + 'Group' => GroupObject::class, + ]; +} diff --git a/tests/php/RecursiveStagesServiceTest/ColumnObject.php b/tests/php/RecursiveStagesServiceTest/ColumnObject.php new file mode 100644 index 00000000..ce6c2878 --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/ColumnObject.php @@ -0,0 +1,66 @@ + 'Varchar(255)', + ]; + + /** + * @var array + */ + private static $has_one = [ + 'PrimaryObject' => PrimaryObject::class, + ]; + + /** + * @var array + */ + private static $has_many = [ + 'Groups' => GroupObject::class, + ]; + + /** + * @var array + */ + private static $owns = [ + 'Groups', + ]; + + /** + * @var array + */ + private static $cascade_duplicates = [ + 'Groups', + ]; + + /** + * @var array + */ + private static $cascade_deletes = [ + 'Groups', + ]; +} diff --git a/tests/php/RecursiveStagesServiceTest/GroupObject.php b/tests/php/RecursiveStagesServiceTest/GroupObject.php new file mode 100644 index 00000000..4bac1eee --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/GroupObject.php @@ -0,0 +1,66 @@ + 'Varchar(255)', + ]; + + /** + * @var array + */ + private static $has_one = [ + 'Column' => ColumnObject::class, + ]; + + /** + * @var array + */ + private static $has_many = [ + 'Children' => ChildObject::class, + ]; + + /** + * @var array + */ + private static $owns = [ + 'Children', + ]; + + /** + * @var array + */ + private static $cascade_duplicates = [ + 'Children', + ]; + + /** + * @var array + */ + private static $cascade_deletes = [ + 'Children', + ]; +} diff --git a/tests/php/RecursiveStagesServiceTest/PrimaryObject.php b/tests/php/RecursiveStagesServiceTest/PrimaryObject.php new file mode 100644 index 00000000..741a756c --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/PrimaryObject.php @@ -0,0 +1,56 @@ + 'Varchar(255)', + ]; + + /** + * @var array + */ + private static $has_many = [ + 'Columns' => ColumnObject::class, + ]; + + /** + * @var array + */ + private static $owns = [ + 'Columns', + ]; + + /** + * @var array + */ + private static $cascade_duplicates = [ + 'Columns', + ]; + + /** + * @var array + */ + private static $cascade_deletes = [ + 'Columns', + ]; +} From dd2674fa064dae94ec0bf4077f4886234d55a31a Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Tue, 6 Apr 2021 14:59:28 +1200 Subject: [PATCH 2/9] Update src/RecursiveStagesService.php Co-authored-by: Maxime Rainville --- src/RecursiveStagesService.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php index bd691b93..8c6276f0 100644 --- a/src/RecursiveStagesService.php +++ b/src/RecursiveStagesService.php @@ -8,11 +8,7 @@ use SilverStripe\ORM\SS_List; /** - * Class RecursiveStagesService - * * Functionality for detecting the need of publishing nested objects owned by common parent / ancestor object - * - * @package SilverStripe\Versioned */ class RecursiveStagesService { From 97d1ac66fadaf93613e48d8c46919723c7893604 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Wed, 7 Apr 2021 10:30:47 +1200 Subject: [PATCH 3/9] PR fixes. --- _config/versionedownership.yml | 6 ++++++ src/RecursiveStagesInterface.php | 24 ++++++++++++++++++++++++ src/RecursiveStagesService.php | 3 +-- src/Versioned.php | 6 +++++- 4 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 src/RecursiveStagesInterface.php diff --git a/_config/versionedownership.yml b/_config/versionedownership.yml index 9f973967..948f0c7f 100644 --- a/_config/versionedownership.yml +++ b/_config/versionedownership.yml @@ -12,3 +12,9 @@ OnlyIf: SilverStripe\Admin\LeftAndMain: extensions: RecursivePublishableHandler: SilverStripe\Versioned\RecursivePublishableHandler +--- +Name: versionedrecursivestages +--- +SilverStripe\Core\Injector\Injector: + SilverStripe\Versioned\RecursiveStagesInterface: + class: SilverStripe\Versioned\RecursiveStagesService diff --git a/src/RecursiveStagesInterface.php b/src/RecursiveStagesInterface.php new file mode 100644 index 00000000..caea445d --- /dev/null +++ b/src/RecursiveStagesInterface.php @@ -0,0 +1,24 @@ +stagesDifferRecursive($this->owner, $mode); + /** @var RecursiveStagesInterface $service */ + $service = Injector::inst()->get(RecursiveStagesInterface::class); + + return $service->stagesDifferRecursive($this->owner, $mode); } /** From d2efe5acba12462d816ee6db4ca28f6c9b2ca06d Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Tue, 17 Oct 2023 12:35:29 +1300 Subject: [PATCH 4/9] PR fixes. --- _config/versionedownership.yml | 10 +- _config/versionedstate.yml | 1 + src/RecursiveStagesExtension.php | 34 +++ src/RecursiveStagesInterface.php | 6 +- src/RecursiveStagesService.php | 193 ++++++++++-------- src/Versioned.php | 11 +- tests/php/RecursiveStagesServiceTest.php | 111 +++++++--- tests/php/RecursiveStagesServiceTest.yml | 18 +- .../ChildObject.php | 18 +- .../ColumnObject.php | 38 +--- .../GroupObject.php | 38 +--- .../PrimaryObject.php | 35 +--- 12 files changed, 267 insertions(+), 246 deletions(-) create mode 100644 src/RecursiveStagesExtension.php diff --git a/_config/versionedownership.yml b/_config/versionedownership.yml index 948f0c7f..a6b43386 100644 --- a/_config/versionedownership.yml +++ b/_config/versionedownership.yml @@ -4,6 +4,10 @@ Name: versionedownership SilverStripe\ORM\DataObject: extensions: RecursivePublishable: SilverStripe\Versioned\RecursivePublishable + +SilverStripe\Core\Injector\Injector: + SilverStripe\Versioned\RecursiveStagesInterface: + class: SilverStripe\Versioned\RecursiveStagesService --- Name: versionedownership-admin OnlyIf: @@ -12,9 +16,3 @@ OnlyIf: SilverStripe\Admin\LeftAndMain: extensions: RecursivePublishableHandler: SilverStripe\Versioned\RecursivePublishableHandler ---- -Name: versionedrecursivestages ---- -SilverStripe\Core\Injector\Injector: - SilverStripe\Versioned\RecursiveStagesInterface: - class: SilverStripe\Versioned\RecursiveStagesService diff --git a/_config/versionedstate.yml b/_config/versionedstate.yml index 1813c8e5..653f5e6f 100644 --- a/_config/versionedstate.yml +++ b/_config/versionedstate.yml @@ -7,6 +7,7 @@ SilverStripe\Control\RequestHandler: SilverStripe\ORM\DataObject: extensions: - SilverStripe\Versioned\VersionedStateExtension + - SilverStripe\Versioned\RecursiveStagesExtension --- Name: versionedstate-test --- diff --git a/src/RecursiveStagesExtension.php b/src/RecursiveStagesExtension.php new file mode 100644 index 00000000..b77df6f8 --- /dev/null +++ b/src/RecursiveStagesExtension.php @@ -0,0 +1,34 @@ +stagesDifferCache = []; + $this->ownedObjectsCache = []; + } + /** - * Strong ownership uses 'owns' configuration to determine relationships + * @return void + * @throws NotFoundExceptionInterface */ - public const OWNERSHIP_STRONG = 'strong'; + public static function reset(): void + { + /** @var RecursiveStagesInterface $service */ + $service = Injector::inst()->get(RecursiveStagesInterface::class); + + if (!$service instanceof RecursiveStagesService) { + // This covers the case where the service is overridden + return; + } + + $service->flushCachedData(); + } /** - * Weak ownership uses 'cascade_duplicates' configuration to determine relationships + * Determine if content differs on stages including nested objects + * This method also supports non-versioned models to allow traversal of hierarchy + * which includes both versioned and non-versioned models + * + * @param DataObject $object + * @return bool + * @throws Exception */ - public const OWNERSHIP_WEAK = 'weak'; + public function stagesDifferRecursive(DataObject $object): bool + { + $cacheKey = $object->getUniqueKey(); + + if (!array_key_exists($cacheKey, $this->stagesDifferCache)) { + $this->stagesDifferCache[$cacheKey] = $this->determineStagesDifferRecursive($object); + } + + return $this->stagesDifferCache[$cacheKey]; + } /** - * Determine if content differs on stages including nested objects + * Execution ownership hierarchy traversal and inspect individual models * * @param DataObject $object - * @param string $mode * @return bool + * @throws Exception */ - public function stagesDifferRecursive(DataObject $object, string $mode): bool + protected function determineStagesDifferRecursive(DataObject $object): bool { if (!$object->exists()) { + // Model hasn't been saved to DB, so we can just bail out as there is nothing to inspect return false; } - $items = [$object]; + $models = [$object]; - // compare existing content - while ($item = array_shift($items)) { - if ($this->checkNeedPublishingItem($item)) { + // Compare existing content (perform full ownership traversal) + while ($model = array_shift($models)) { + if ($this->isModelDirty($model)) { + // Model is dirty, + // we can return here as there is no need to check the rest of the hierarchy return true; } - $relatedObjects = $this->findOwnedObjects($item, $mode); - $items = array_merge($items, $relatedObjects); + // Discover and add owned objects for inspection + $relatedObjects = $this->getOwnedObjects($model); + $models = array_merge($models, $relatedObjects); } - // compare deleted content - $draftIdentifiers = $this->findOwnedIdentifiers($object, $mode, Versioned::DRAFT); - $liveIdentifiers = $this->findOwnedIdentifiers($object, $mode, Versioned::LIVE); + // Compare deleted content, + // this wouldn't be covered in hierarchy traversal as deleted models are no longer present + $draftIdentifiers = $this->getOwnedIdentifiers($object, Versioned::DRAFT); + $liveIdentifiers = $this->getOwnedIdentifiers($object, Versioned::LIVE); return $draftIdentifiers !== $liveIdentifiers; } /** - * Find all identifiers for owned objects + * Get unique identifiers for all owned objects, so we can easily compare them * * @param DataObject $object - * @param string $mode * @param string $stage * @return array + * @throws Exception */ - protected function findOwnedIdentifiers(DataObject $object, string $mode, string $stage): array + protected function getOwnedIdentifiers(DataObject $object, string $stage): array { - $ids = Versioned::withVersionedMode(function () use ($object, $mode, $stage): array { + $identifiers = Versioned::withVersionedMode(function () use ($object, $stage): array { Versioned::set_stage($stage); - $object = DataObject::get_by_id($object->ClassName, $object->ID); + /** @var DataObject $stagedObject */ + $stagedObject = DataObject::get_by_id($object->ClassName, $object->ID); - if ($object === null) { + if ($stagedObject === null) { return []; } - $items = [$object]; - $ids = []; + $models = [$stagedObject]; + $identifiers = []; - while ($object = array_shift($items)) { - $ids[] = implode('_', [$object->baseClass(), $object->ID]); - $relatedObjects = $this->findOwnedObjects($object, $mode); - $items = array_merge($items, $relatedObjects); + while ($model = array_shift($models)) { + // Compose a unique identifier, so we can easily compare models + // Note that we intentionally use base class here, so we can cover the situation where model class changes + $identifiers[] = implode('_', [$model->baseClass(), $model->ID]); + $relatedObjects = $this->getOwnedObjects($model); + $models = array_merge($models, $relatedObjects); } - return $ids; + return $identifiers; }); - sort($ids, SORT_STRING); + sort($identifiers, SORT_STRING); - return array_values($ids); + return array_values($identifiers); } /** - * This lookup will attempt to find "Strongly owned" objects - * such objects are unable to exist without the current object - * We will use "cascade_duplicates" setting for this purpose as we can assume that if an object needs to be - * duplicated along with the owner object, it uses the strong ownership relation - * - * "Weakly owned" objects could be looked up via "owns" setting - * Such objects can exist even without the owner objects as they are often used as shared objects - * managed independently of their owners + * This lookup will attempt to find "owned" objects + * This method uses the 'owns' relation, same as @see RecursivePublishable::publishRecursive() * - * @param DataObject $object - * @param string $mode + * @param DataObject|RecursivePublishable $object * @return array + * @throws Exception */ - protected function findOwnedObjects(DataObject $object, string $mode): array + protected function getOwnedObjects(DataObject $object): array { - $ownershipType = $mode === self::OWNERSHIP_WEAK - ? 'owns' - : 'cascade_duplicates'; - - $relations = (array) $object->config()->get($ownershipType); - $relations = array_unique($relations); - $result = []; - - foreach ($relations as $relation) { - $relation = (string) $relation; - - if (!$relation) { - continue; - } - - $relationData = $object->{$relation}(); - - if ($relationData instanceof DataObject) { - if (!$relationData->exists()) { - continue; - } - - $result[] = $relationData; - - continue; - } - - if (!$relationData instanceof SS_List) { - continue; - } + if (!$object->hasExtension(RecursivePublishable::class)) { + return []; + } - foreach ($relationData as $relatedObject) { - if (!$relatedObject instanceof DataObject || !$relatedObject->exists()) { - continue; - } + // Add versioned stage to cache key to cover the case where non-versioned model owns versioned models + // In this situation the versioned models can have different cached state which we need to cover + $cacheKey = sprintf('%s-%s', $object->getUniqueKey(), Versioned::get_stage()); - $result[] = $relatedObject; - } + if (!array_key_exists($cacheKey, $this->ownedObjectsCache)) { + $this->ownedObjectsCache[$cacheKey] = $object + // We intentionally avoid recursive traversal here as it's not memory efficient, + // stack based approach is used instead for better performance + ->findOwned(false) + ->toArray(); } - return $result; + return $this->ownedObjectsCache[$cacheKey]; } /** - * @param DataObject|Versioned $item + * Determine if model is dirty (has draft changes that need publishing) + * Non-versioned models are supported + * + * @param DataObject $object * @return bool */ - protected function checkNeedPublishingItem(DataObject $item): bool + protected function isModelDirty(DataObject $object): bool { - if ($item->hasExtension(Versioned::class)) { - /** @var $item Versioned */ - return !$item->isPublished() || $item->stagesDiffer(); + if ($object->hasExtension(Versioned::class)) { + /** @var $object Versioned */ + return !$object->isPublished() || $object->stagesDiffer(); } + // Non-versioned models are never dirty return false; } } diff --git a/src/Versioned.php b/src/Versioned.php index 775c938b..06079cb9 100644 --- a/src/Versioned.php +++ b/src/Versioned.php @@ -4,6 +4,7 @@ use InvalidArgumentException; use LogicException; +use Psr\Container\NotFoundExceptionInterface; use SilverStripe\Control\Controller; use SilverStripe\Control\Cookie; use SilverStripe\Control\Director; @@ -2000,19 +2001,17 @@ public function stagesDiffer() /** * Determine if content differs on stages including nested objects - * $mode determines which relation will be used to traverse the ownership tree - * "strong" will use "cascade_duplicates" - * "weak" will use "owns" + * 'owns' configuration drives the relationship traversal * - * @param string $mode "strong" or "weak" * @return bool + * @throws NotFoundExceptionInterface */ - public function stagesDifferRecursive(string $mode = RecursiveStagesService::OWNERSHIP_STRONG): bool + public function stagesDifferRecursive(): bool { /** @var RecursiveStagesInterface $service */ $service = Injector::inst()->get(RecursiveStagesInterface::class); - return $service->stagesDifferRecursive($this->owner, $mode); + return $service->stagesDifferRecursive($this->owner); } /** diff --git a/tests/php/RecursiveStagesServiceTest.php b/tests/php/RecursiveStagesServiceTest.php index b9cc43cf..33a83e53 100644 --- a/tests/php/RecursiveStagesServiceTest.php +++ b/tests/php/RecursiveStagesServiceTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Versioned\Tests; +use Psr\Container\NotFoundExceptionInterface; use SilverStripe\Dev\SapphireTest; use SilverStripe\ORM\ValidationException; use SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ChildObject; @@ -31,9 +32,6 @@ class RecursiveStagesServiceTest extends SapphireTest PrimaryObject::class => [ Versioned::class, ], - ColumnObject::class => [ - Versioned::class, - ], GroupObject::class => [ Versioned::class, ], @@ -42,51 +40,104 @@ class RecursiveStagesServiceTest extends SapphireTest ], ]; + /** + * @return void + * @throws NotFoundExceptionInterface + */ + public function testStageDiffersRecursiveWithInvalidObject(): void + { + Versioned::withVersionedMode(function (): void { + Versioned::set_stage(Versioned::DRAFT); + + /** @var PrimaryObject|Versioned $primaryItem */ + $primaryItem = PrimaryObject::create(); + + $this->assertFalse($primaryItem->stagesDifferRecursive(), 'We expect to see no changes on invalid object'); + }); + } + /** * @param string $class * @param string $identifier * @param bool $delete + * @param bool $expected + * @return void * @throws ValidationException + * @throws NotFoundExceptionInterface * @dataProvider objectsProvider */ - public function testStageDiffersRecursive(string $class, string $identifier, bool $delete): void + public function testStageDiffersRecursive(string $class, string $identifier, bool $delete, bool $expected): void { - /** @var PrimaryObject|Versioned $primaryItem */ - $primaryItem = $this->objFromFixture(PrimaryObject::class, 'primary-object-1'); - $primaryItem->publishRecursive(); + Versioned::withVersionedMode(function () use ($class, $identifier, $delete, $expected): void { + Versioned::set_stage(Versioned::DRAFT); - $this->assertFalse($primaryItem->stagesDifferRecursive()); + /** @var PrimaryObject|Versioned $primaryObject */ + $primaryObject = $this->objFromFixture(PrimaryObject::class, 'primary-object-1'); + $primaryObject->publishRecursive(); - $record = $this->objFromFixture($class, $identifier); + $this->assertFalse($primaryObject->stagesDifferRecursive(), 'We expect no changes to be present initially'); - if ($delete) { - $record->delete(); - } else { - $record->Title = 'New Title'; - $record->write(); - } + // Fetch a specific record and make an edit + $record = $this->objFromFixture($class, $identifier); - $this->assertTrue($primaryItem->stagesDifferRecursive()); - } - - public function testStageDiffersRecursiveWithInvalidObject(): void - { - /** @var PrimaryObject|Versioned $primaryItem */ - $primaryItem = PrimaryObject::create(); + if ($delete) { + // Delete the record + $record->delete(); + } else { + // Update the record + $record->Title .= '-updated'; + $record->write(); + } - $this->assertFalse($primaryItem->stagesDifferRecursive()); + $this->assertEquals($expected, $primaryObject->stagesDifferRecursive(), 'We expect to see changes depending on the case'); + }); } public function objectsProvider(): array { return [ - [PrimaryObject::class, 'primary-object-1', false], - [ColumnObject::class, 'column-1', false], - [ColumnObject::class, 'column-1', true], - [GroupObject::class, 'group-1', false], - [GroupObject::class, 'group-1', true], - [ChildObject::class, 'child-object-1', false], - [ChildObject::class, 'child-object-1', true], + 'primary object (versioned, update)' => [ + PrimaryObject::class, + 'primary-object-1', + false, + true, + ], + 'column (non-versioned, update)' => [ + ColumnObject::class, + 'column-1', + false, + false, + ], + 'column (non-versioned, delete)' => [ + ColumnObject::class, + 'column-1', + true, + false, + ], + 'group (versioned, update)' => [ + GroupObject::class, + 'group-1', + false, + true, + ], + 'group (versioned, delete)' => [ + GroupObject::class, + 'group-1', + true, + true, + ], + 'child (versioned, update)' => [ + ChildObject::class, + 'child-object-1', + false, + true, + ], + 'child (versioned, delete)' => [ + ChildObject::class, + 'child-object-1', + true, + true, + ], ]; } } diff --git a/tests/php/RecursiveStagesServiceTest.yml b/tests/php/RecursiveStagesServiceTest.yml index 6e35a701..727a1f71 100644 --- a/tests/php/RecursiveStagesServiceTest.yml +++ b/tests/php/RecursiveStagesServiceTest.yml @@ -1,24 +1,24 @@ -# site-config-1 -# -> primary-object-1 (top level publish object) -# --> column-1 -# ---> group-1 -# ----> child-object-1 +# Fixture hierarchy +# -> primary-object-1 (top level publish object, versioned) +# --> column-1 (non-versioned) +# ---> group-1 (versioned) +# ----> child-object-1 (versioned) SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\PrimaryObject: primary-object-1: - Title: PrimaryObject1 + Title: 'PrimaryObject1' SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject: column-1: - Title: Column1 + Title: 'Column1' PrimaryObject: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\PrimaryObject.primary-object-1 SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject: group-1: - Title: Group1 + Title: 'Group1' Column: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject.column-1 SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ChildObject: child-object-1: - Title: Item1 + Title: 'Item1' Group: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject.group-1 diff --git a/tests/php/RecursiveStagesServiceTest/ChildObject.php b/tests/php/RecursiveStagesServiceTest/ChildObject.php index 6eabe6d6..2c8af507 100644 --- a/tests/php/RecursiveStagesServiceTest/ChildObject.php +++ b/tests/php/RecursiveStagesServiceTest/ChildObject.php @@ -6,31 +6,19 @@ use SilverStripe\ORM\DataObject; /** - * Class ChildObject - * * @property string $Title * @property int $GroupID * @method GroupObject Group() - * @package SilverStripe\Versioned\Tests\RecursiveStagesServiceTest */ class ChildObject extends DataObject implements TestOnly { - /** - * @var string - */ - private static $table_name = 'RecursiveStagesServiceTest_ChildObject'; + private static string $table_name = 'RecursiveStagesServiceTest_ChildObject'; - /** - * @var array - */ - private static $db = [ + private static array $db = [ 'Title' => 'Varchar(255)', ]; - /** - * @var array - */ - private static $has_one = [ + private static array $has_one = [ 'Group' => GroupObject::class, ]; } diff --git a/tests/php/RecursiveStagesServiceTest/ColumnObject.php b/tests/php/RecursiveStagesServiceTest/ColumnObject.php index ce6c2878..c7f4e7d8 100644 --- a/tests/php/RecursiveStagesServiceTest/ColumnObject.php +++ b/tests/php/RecursiveStagesServiceTest/ColumnObject.php @@ -7,60 +7,36 @@ use SilverStripe\ORM\HasManyList; /** - * Class ColumnObject - * * @property string $Title * @property int $PrimaryObject * @method PrimaryObject PrimaryObject() * @method HasManyList|GroupObject[] Groups() - * @package SilverStripe\Versioned\Tests\RecursiveStagesServiceTest */ class ColumnObject extends DataObject implements TestOnly { - /** - * @var string - */ - private static $table_name = 'RecursiveStagesServiceTest_ColumnObject'; + private static string $table_name = 'RecursiveStagesServiceTest_ColumnObject'; - /** - * @var array - */ - private static $db = [ + private static array $db = [ 'Title' => 'Varchar(255)', ]; - /** - * @var array - */ - private static $has_one = [ + private static array $has_one = [ 'PrimaryObject' => PrimaryObject::class, ]; - /** - * @var array - */ - private static $has_many = [ + private static array $has_many = [ 'Groups' => GroupObject::class, ]; - /** - * @var array - */ - private static $owns = [ + private static array $owns = [ 'Groups', ]; - /** - * @var array - */ - private static $cascade_duplicates = [ + private static array $cascade_duplicates = [ 'Groups', ]; - /** - * @var array - */ - private static $cascade_deletes = [ + private static array $cascade_deletes = [ 'Groups', ]; } diff --git a/tests/php/RecursiveStagesServiceTest/GroupObject.php b/tests/php/RecursiveStagesServiceTest/GroupObject.php index 4bac1eee..0b624c5d 100644 --- a/tests/php/RecursiveStagesServiceTest/GroupObject.php +++ b/tests/php/RecursiveStagesServiceTest/GroupObject.php @@ -7,60 +7,36 @@ use SilverStripe\ORM\HasManyList; /** - * Class GroupObject - * * @property string $Title * @property int $ColumnID * @method ColumnObject Column() * @method HasManyList|ChildObject[] Children() - * @package SilverStripe\Versioned\Tests\RecursiveStagesServiceTest */ class GroupObject extends DataObject implements TestOnly { - /** - * @var string - */ - private static $table_name = 'RecursiveStagesServiceTest_GroupObject'; + private static string $table_name = 'RecursiveStagesServiceTest_GroupObject'; - /** - * @var array - */ - private static $db = [ + private static array $db = [ 'Title' => 'Varchar(255)', ]; - /** - * @var array - */ - private static $has_one = [ + private static array $has_one = [ 'Column' => ColumnObject::class, ]; - /** - * @var array - */ - private static $has_many = [ + private static array $has_many = [ 'Children' => ChildObject::class, ]; - /** - * @var array - */ - private static $owns = [ + private static array $owns = [ 'Children', ]; - /** - * @var array - */ - private static $cascade_duplicates = [ + private static array $cascade_duplicates = [ 'Children', ]; - /** - * @var array - */ - private static $cascade_deletes = [ + private static array $cascade_deletes = [ 'Children', ]; } diff --git a/tests/php/RecursiveStagesServiceTest/PrimaryObject.php b/tests/php/RecursiveStagesServiceTest/PrimaryObject.php index 741a756c..2520f22d 100644 --- a/tests/php/RecursiveStagesServiceTest/PrimaryObject.php +++ b/tests/php/RecursiveStagesServiceTest/PrimaryObject.php @@ -7,50 +7,29 @@ use SilverStripe\ORM\HasManyList; /** - * Class PrimaryObject - * * @method HasManyList|ColumnObject[] Columns() - * @package SilverStripe\Versioned\Tests\RecursiveStagesServiceTest */ class PrimaryObject extends DataObject implements TestOnly { - /** - * @var string - */ - private static $table_name = 'RecursiveStagesServiceTest_PrimaryObject'; - - /** - * @var array - */ - private static $db = [ + private static string $table_name = 'RecursiveStagesServiceTest_PrimaryObject'; + + private static array $db = [ 'Title' => 'Varchar(255)', ]; - /** - * @var array - */ - private static $has_many = [ + private static array $has_many = [ 'Columns' => ColumnObject::class, ]; - /** - * @var array - */ - private static $owns = [ + private static array $owns = [ 'Columns', ]; - /** - * @var array - */ - private static $cascade_duplicates = [ + private static array $cascade_duplicates = [ 'Columns', ]; - /** - * @var array - */ - private static $cascade_deletes = [ + private static array $cascade_deletes = [ 'Columns', ]; } From d6490cef97a6d3efab6fa03e712a06739d8405ae Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Thu, 19 Oct 2023 12:30:55 +1300 Subject: [PATCH 5/9] PR fixes. --- _config/versionedstate.yml | 1 - src/RecursivePublishable.php | 19 +++++++- src/RecursiveStagesExtension.php | 34 --------------- src/RecursiveStagesInterface.php | 3 -- src/RecursiveStagesService.php | 55 ++++-------------------- src/Versioned.php | 4 -- tests/php/RecursiveStagesServiceTest.php | 13 ------ 7 files changed, 26 insertions(+), 103 deletions(-) delete mode 100644 src/RecursiveStagesExtension.php diff --git a/_config/versionedstate.yml b/_config/versionedstate.yml index 653f5e6f..1813c8e5 100644 --- a/_config/versionedstate.yml +++ b/_config/versionedstate.yml @@ -7,7 +7,6 @@ SilverStripe\Control\RequestHandler: SilverStripe\ORM\DataObject: extensions: - SilverStripe\Versioned\VersionedStateExtension - - SilverStripe\Versioned\RecursiveStagesExtension --- Name: versionedstate-test --- diff --git a/src/RecursivePublishable.php b/src/RecursivePublishable.php index e02a19d0..79d636c8 100644 --- a/src/RecursivePublishable.php +++ b/src/RecursivePublishable.php @@ -12,7 +12,6 @@ use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\Queries\SQLUpdate; use SilverStripe\ORM\SS_List; -use SilverStripe\ORM\Tests\MySQLDatabaseTest\Data; /** * Provides owns / owned_by and recursive publishing API for all objects. @@ -448,4 +447,22 @@ public function onBeforeDuplicate($original, &$doWrite, &$relations) $owns = $this->owner->config()->get('owns'); $relations = array_intersect($allowed ?? [], $owns); } + + /** + * Make sure to flush cached data in case the data changes + * Extension point in @see DataObject::onAfterWrite() + */ + public function onAfterWrite(): void + { + RecursiveStagesService::reset(); + } + + /** + * Make sure to flush cached data in case the data changes + * Extension point in @see DataObject::onAfterDelete() + */ + public function onAfterDelete(): void + { + RecursiveStagesService::reset(); + } } diff --git a/src/RecursiveStagesExtension.php b/src/RecursiveStagesExtension.php deleted file mode 100644 index b77df6f8..00000000 --- a/src/RecursiveStagesExtension.php +++ /dev/null @@ -1,34 +0,0 @@ -ownedObjectsCache = []; } - /** - * @return void - * @throws NotFoundExceptionInterface - */ public static function reset(): void { /** @var RecursiveStagesInterface $service */ @@ -46,10 +40,6 @@ public static function reset(): void * Determine if content differs on stages including nested objects * This method also supports non-versioned models to allow traversal of hierarchy * which includes both versioned and non-versioned models - * - * @param DataObject $object - * @return bool - * @throws Exception */ public function stagesDifferRecursive(DataObject $object): bool { @@ -64,10 +54,6 @@ public function stagesDifferRecursive(DataObject $object): bool /** * Execution ownership hierarchy traversal and inspect individual models - * - * @param DataObject $object - * @return bool - * @throws Exception */ protected function determineStagesDifferRecursive(DataObject $object): bool { @@ -76,11 +62,12 @@ protected function determineStagesDifferRecursive(DataObject $object): bool return false; } + // Compare existing content (perform full ownership traversal) $models = [$object]; - // Compare existing content (perform full ownership traversal) + /** @var DataObject|Versioned $model */ while ($model = array_shift($models)) { - if ($this->isModelDirty($model)) { + if ($model->hasExtension(Versioned::class) && $model->isModifiedOnDraft()) { // Model is dirty, // we can return here as there is no need to check the rest of the hierarchy return true; @@ -101,11 +88,6 @@ protected function determineStagesDifferRecursive(DataObject $object): bool /** * Get unique identifiers for all owned objects, so we can easily compare them - * - * @param DataObject $object - * @param string $stage - * @return array - * @throws Exception */ protected function getOwnedIdentifiers(DataObject $object, string $stage): array { @@ -123,9 +105,10 @@ protected function getOwnedIdentifiers(DataObject $object, string $stage): array $identifiers = []; while ($model = array_shift($models)) { - // Compose a unique identifier, so we can easily compare models - // Note that we intentionally use base class here, so we can cover the situation where model class changes - $identifiers[] = implode('_', [$model->baseClass(), $model->ID]); + // Compose a unique identifier, so we can easily compare models across stages + // Note that we can't use getUniqueKey() as that one contains stage fragments + // which prevents us from making cross-stage comparison + $identifiers[] = $model->ClassName . '-' . $model->ID; $relatedObjects = $this->getOwnedObjects($model); $models = array_merge($models, $relatedObjects); } @@ -141,10 +124,6 @@ protected function getOwnedIdentifiers(DataObject $object, string $stage): array /** * This lookup will attempt to find "owned" objects * This method uses the 'owns' relation, same as @see RecursivePublishable::publishRecursive() - * - * @param DataObject|RecursivePublishable $object - * @return array - * @throws Exception */ protected function getOwnedObjects(DataObject $object): array { @@ -154,7 +133,7 @@ protected function getOwnedObjects(DataObject $object): array // Add versioned stage to cache key to cover the case where non-versioned model owns versioned models // In this situation the versioned models can have different cached state which we need to cover - $cacheKey = sprintf('%s-%s', $object->getUniqueKey(), Versioned::get_stage()); + $cacheKey = $object->getUniqueKey() . '-' . Versioned::get_stage(); if (!array_key_exists($cacheKey, $this->ownedObjectsCache)) { $this->ownedObjectsCache[$cacheKey] = $object @@ -166,22 +145,4 @@ protected function getOwnedObjects(DataObject $object): array return $this->ownedObjectsCache[$cacheKey]; } - - /** - * Determine if model is dirty (has draft changes that need publishing) - * Non-versioned models are supported - * - * @param DataObject $object - * @return bool - */ - protected function isModelDirty(DataObject $object): bool - { - if ($object->hasExtension(Versioned::class)) { - /** @var $object Versioned */ - return !$object->isPublished() || $object->stagesDiffer(); - } - - // Non-versioned models are never dirty - return false; - } } diff --git a/src/Versioned.php b/src/Versioned.php index 06079cb9..8e917cf0 100644 --- a/src/Versioned.php +++ b/src/Versioned.php @@ -4,7 +4,6 @@ use InvalidArgumentException; use LogicException; -use Psr\Container\NotFoundExceptionInterface; use SilverStripe\Control\Controller; use SilverStripe\Control\Cookie; use SilverStripe\Control\Director; @@ -2002,9 +2001,6 @@ public function stagesDiffer() /** * Determine if content differs on stages including nested objects * 'owns' configuration drives the relationship traversal - * - * @return bool - * @throws NotFoundExceptionInterface */ public function stagesDifferRecursive(): bool { diff --git a/tests/php/RecursiveStagesServiceTest.php b/tests/php/RecursiveStagesServiceTest.php index 33a83e53..9e9bc95e 100644 --- a/tests/php/RecursiveStagesServiceTest.php +++ b/tests/php/RecursiveStagesServiceTest.php @@ -2,9 +2,7 @@ namespace SilverStripe\Versioned\Tests; -use Psr\Container\NotFoundExceptionInterface; use SilverStripe\Dev\SapphireTest; -use SilverStripe\ORM\ValidationException; use SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ChildObject; use SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject; use SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject; @@ -40,10 +38,6 @@ class RecursiveStagesServiceTest extends SapphireTest ], ]; - /** - * @return void - * @throws NotFoundExceptionInterface - */ public function testStageDiffersRecursiveWithInvalidObject(): void { Versioned::withVersionedMode(function (): void { @@ -57,13 +51,6 @@ public function testStageDiffersRecursiveWithInvalidObject(): void } /** - * @param string $class - * @param string $identifier - * @param bool $delete - * @param bool $expected - * @return void - * @throws ValidationException - * @throws NotFoundExceptionInterface * @dataProvider objectsProvider */ public function testStageDiffersRecursive(string $class, string $identifier, bool $delete, bool $expected): void From 76568ac021abd442aa7ad53eff7dfbb7e468ee29 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Thu, 19 Oct 2023 13:42:03 +1300 Subject: [PATCH 6/9] PR fixes. --- src/RecursiveStagesService.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php index 1889d0cb..e125c8ca 100644 --- a/src/RecursiveStagesService.php +++ b/src/RecursiveStagesService.php @@ -127,6 +127,7 @@ protected function getOwnedIdentifiers(DataObject $object, string $stage): array */ protected function getOwnedObjects(DataObject $object): array { + /** @var DataObject|Versioned $object */ if (!$object->hasExtension(RecursivePublishable::class)) { return []; } @@ -137,8 +138,10 @@ protected function getOwnedObjects(DataObject $object): array if (!array_key_exists($cacheKey, $this->ownedObjectsCache)) { $this->ownedObjectsCache[$cacheKey] = $object - // We intentionally avoid recursive traversal here as it's not memory efficient, - // stack based approach is used instead for better performance + // We intentionally avoid "true" recursive traversal here as it's not performant + // (often the cause of memory usage spikes and longer exeuction time due to deeper stack depth) + // Instead we use "stack based" recursive traversal approach for better performance + // which avoids the nested method calls ->findOwned(false) ->toArray(); } From 5ed82fe06eb104846762b88e8b38f4e1c832dd18 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Thu, 19 Oct 2023 14:08:25 +1300 Subject: [PATCH 7/9] PR fixes. --- src/RecursiveStagesService.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php index e125c8ca..cbc03a67 100644 --- a/src/RecursiveStagesService.php +++ b/src/RecursiveStagesService.php @@ -40,6 +40,11 @@ public static function reset(): void * Determine if content differs on stages including nested objects * This method also supports non-versioned models to allow traversal of hierarchy * which includes both versioned and non-versioned models + * In-memory cache is included and optimised for the most likely lookup profile: + * Non-shared models can have deep ownership hierarchy (i.e. content blocks) + * Shared models are unlikely to have deep ownership hierarchy (i.e Assets) + * This means that we provide in-memory cache only for top level models as we do not expect to query + * the nested models multiple times */ public function stagesDifferRecursive(DataObject $object): bool { @@ -54,6 +59,8 @@ public function stagesDifferRecursive(DataObject $object): bool /** * Execution ownership hierarchy traversal and inspect individual models + * This method use "stack based" recursive traversal as opposed to "true" recursive traversal + * for performance reasons (avoid memory spikes and long execution times due to deeper stack) */ protected function determineStagesDifferRecursive(DataObject $object): bool { From f21edce874766c6a195f4fb3ed6bc727dd182504 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Thu, 19 Oct 2023 15:39:14 +1300 Subject: [PATCH 8/9] PR fixes. --- src/RecursiveStagesService.php | 38 ++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php index cbc03a67..9ca75de0 100644 --- a/src/RecursiveStagesService.php +++ b/src/RecursiveStagesService.php @@ -72,8 +72,23 @@ protected function determineStagesDifferRecursive(DataObject $object): bool // Compare existing content (perform full ownership traversal) $models = [$object]; + // We will keep track of inspected models so we don;t inspect the same model multiple times + // This prevents some edge cases like infinite loops + $identifiers = []; + /** @var DataObject|Versioned $model */ while ($model = array_shift($models)) { + $identifier = $this->getObjectIdentifier($model); + + if (in_array($identifier, $identifiers)) { + // We've already inspected this model, so we can skip processing it + // This is to prevent potential infinite loops + continue; + } + + // Mark model as inspected + $identifiers[] = $identifier; + if ($model->hasExtension(Versioned::class) && $model->isModifiedOnDraft()) { // Model is dirty, // we can return here as there is no need to check the rest of the hierarchy @@ -112,10 +127,15 @@ protected function getOwnedIdentifiers(DataObject $object, string $stage): array $identifiers = []; while ($model = array_shift($models)) { - // Compose a unique identifier, so we can easily compare models across stages - // Note that we can't use getUniqueKey() as that one contains stage fragments - // which prevents us from making cross-stage comparison - $identifiers[] = $model->ClassName . '-' . $model->ID; + $identifier = $this->getObjectIdentifier($model); + + if (in_array($identifier, $identifiers)) { + // We've already inspected this model, so we can skip processing it + // This is to prevent potential infinite loops + continue; + } + + $identifiers[] = $identifier; $relatedObjects = $this->getOwnedObjects($model); $models = array_merge($models, $relatedObjects); } @@ -155,4 +175,14 @@ protected function getOwnedObjects(DataObject $object): array return $this->ownedObjectsCache[$cacheKey]; } + + /** + * This method covers cases where @see DataObject::getUniqueKey() can't be used + * For example when we want to compare models across stages we can't use getUniqueKey() + * as that one contains stage fragments which prevents us from making cross-stage comparison + */ + private function getObjectIdentifier(DataObject $object): string + { + return $object->ClassName . '-' . $object->ID; + } } From 7eddb6b944b4c0f76a5deb4090f35aacbde88803 Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Mon, 30 Oct 2023 15:14:56 +1300 Subject: [PATCH 9/9] Apply suggestions from code review Co-authored-by: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> --- tests/php/RecursiveStagesServiceTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/php/RecursiveStagesServiceTest.php b/tests/php/RecursiveStagesServiceTest.php index 9e9bc95e..6458ce8f 100644 --- a/tests/php/RecursiveStagesServiceTest.php +++ b/tests/php/RecursiveStagesServiceTest.php @@ -45,6 +45,7 @@ public function testStageDiffersRecursiveWithInvalidObject(): void /** @var PrimaryObject|Versioned $primaryItem */ $primaryItem = PrimaryObject::create(); + $primaryItem->Title = 'This registers as a change in DataObject::isChanged()'; $this->assertFalse($primaryItem->stagesDifferRecursive(), 'We expect to see no changes on invalid object'); });