From eb048d5cadd9070e8fd553c6494a460c3cc2b89d Mon Sep 17 00:00:00 2001 From: Mojmir Fendek Date: Tue, 17 Oct 2023 14:15:22 +1300 Subject: [PATCH] Fixes. --- src/RecursiveStagesService.php | 87 ++++++++---------------- tests/php/RecursiveStagesServiceTest.php | 44 +++++++----- 2 files changed, 56 insertions(+), 75 deletions(-) diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php index ddfa75ef..d79ffb61 100644 --- a/src/RecursiveStagesService.php +++ b/src/RecursiveStagesService.php @@ -8,7 +8,6 @@ use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Resettable; use SilverStripe\ORM\DataObject; -use SilverStripe\ORM\SS_List; /** * Functionality for detecting the need of publishing nested objects owned by common parent / ancestor object @@ -17,11 +16,13 @@ class RecursiveStagesService implements RecursiveStagesInterface, Resettable { use Injectable; - private array $cachedData = []; + private array $stagesDifferCache = []; + private array $ownedObjectsCache = []; public function flushCachedData(): void { - $this->cachedData = []; + $this->stagesDifferCache = []; + $this->ownedObjectsCache = []; } /** @@ -54,11 +55,11 @@ public function stagesDifferRecursive(DataObject $object): bool { $cacheKey = $object->getUniqueKey(); - if (!array_key_exists($cacheKey, $this->cachedData)) { - $this->cachedData[$cacheKey] = $this->determineStagesDifferRecursive($object); + if (!array_key_exists($cacheKey, $this->stagesDifferCache)) { + $this->stagesDifferCache[$cacheKey] = $this->determineStagesDifferRecursive($object); } - return $this->cachedData[$cacheKey]; + return $this->stagesDifferCache[$cacheKey]; } /** @@ -66,6 +67,7 @@ public function stagesDifferRecursive(DataObject $object): bool * * @param DataObject $object * @return bool + * @throws Exception */ protected function determineStagesDifferRecursive(DataObject $object): bool { @@ -85,14 +87,14 @@ protected function determineStagesDifferRecursive(DataObject $object): bool } // Discover and add owned objects for inspection - $relatedObjects = $this->findOwnedObjects($model); + $relatedObjects = $this->getOwnedObjects($model); $models = array_merge($models, $relatedObjects); } // Compare deleted content, // this wouldn't be covered in hierarchy traversal as deleted models are no longer present - $draftIdentifiers = $this->findOwnedIdentifiers($object, Versioned::DRAFT); - $liveIdentifiers = $this->findOwnedIdentifiers($object, Versioned::LIVE); + $draftIdentifiers = $this->getOwnedIdentifiers($object, Versioned::DRAFT); + $liveIdentifiers = $this->getOwnedIdentifiers($object, Versioned::LIVE); return $draftIdentifiers !== $liveIdentifiers; } @@ -103,8 +105,9 @@ protected function determineStagesDifferRecursive(DataObject $object): bool * @param DataObject $object * @param string $stage * @return array + * @throws Exception */ - protected function findOwnedIdentifiers(DataObject $object, string $stage): array + protected function getOwnedIdentifiers(DataObject $object, string $stage): array { $identifiers = Versioned::withVersionedMode(function () use ($object, $stage): array { Versioned::set_stage($stage); @@ -123,7 +126,7 @@ protected function findOwnedIdentifiers(DataObject $object, string $stage): arra // 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->findOwnedObjects($model); + $relatedObjects = $this->getOwnedObjects($model); $models = array_merge($models, $relatedObjects); } @@ -139,59 +142,29 @@ protected function findOwnedIdentifiers(DataObject $object, string $stage): arra * This lookup will attempt to find "owned" objects * This method uses the 'owns' relation, same as @see RecursivePublishable::publishRecursive() * - * @param DataObject $object + * @param DataObject|RecursivePublishable $object * @return array + * @throws Exception */ - protected function findOwnedObjects(DataObject $object): array + protected function getOwnedObjects(DataObject $object): array { - $relations = (array) $object->config()->get('owns'); - $relations = array_unique($relations); - $result = []; - - foreach ($relations as $relation) { - $relation = (string) $relation; - - if (!$relation) { - // Relation is empty so we can just ignore it - continue; - } - - if (!$object->hasMethod($relation)) { - // The relation is invalid, we will simply ignore it as we can assume that other parts - // of the framework should cover this error - continue; - } - - $relationData = $object->{$relation}(); - - if ($relationData instanceof DataObject) { - // Has-one relation case - if (!$relationData->exists()) { - continue; - } - - // Add related model to our list - $result[] = $relationData; - - continue; - } - - if (!$relationData instanceof SS_List) { - // We expect a has-many or many-many relation here - continue; - } + if (!$object->hasExtension(RecursivePublishable::class)) { + return []; + } - // Add all related models to our list - foreach ($relationData as $relatedObject) { - if (!$relatedObject instanceof DataObject || !$relatedObject->exists()) { - continue; - } + // Add versioned stage to cache key to cover the cache 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]; } /** diff --git a/tests/php/RecursiveStagesServiceTest.php b/tests/php/RecursiveStagesServiceTest.php index 87d85597..33a83e53 100644 --- a/tests/php/RecursiveStagesServiceTest.php +++ b/tests/php/RecursiveStagesServiceTest.php @@ -46,10 +46,14 @@ class RecursiveStagesServiceTest extends SapphireTest */ public function testStageDiffersRecursiveWithInvalidObject(): void { - /** @var PrimaryObject|Versioned $primaryItem */ - $primaryItem = PrimaryObject::create(); + Versioned::withVersionedMode(function (): void { + Versioned::set_stage(Versioned::DRAFT); - $this->assertFalse($primaryItem->stagesDifferRecursive(), 'We expect to see no changes on invalid object'); + /** @var PrimaryObject|Versioned $primaryItem */ + $primaryItem = PrimaryObject::create(); + + $this->assertFalse($primaryItem->stagesDifferRecursive(), 'We expect to see no changes on invalid object'); + }); } /** @@ -64,25 +68,29 @@ public function testStageDiffersRecursiveWithInvalidObject(): void */ public function testStageDiffersRecursive(string $class, string $identifier, bool $delete, bool $expected): void { - /** @var PrimaryObject|Versioned $primaryObject */ - $primaryObject = $this->objFromFixture(PrimaryObject::class, 'primary-object-1'); - $primaryObject->publishRecursive(); + Versioned::withVersionedMode(function () use ($class, $identifier, $delete, $expected): void { + Versioned::set_stage(Versioned::DRAFT); + + /** @var PrimaryObject|Versioned $primaryObject */ + $primaryObject = $this->objFromFixture(PrimaryObject::class, 'primary-object-1'); + $primaryObject->publishRecursive(); - $this->assertFalse($primaryObject->stagesDifferRecursive(), 'We expect no changes to be present initially'); + $this->assertFalse($primaryObject->stagesDifferRecursive(), 'We expect no changes to be present initially'); - // Fetch a specific record and make an edit - $record = $this->objFromFixture($class, $identifier); + // Fetch a specific record and make an edit + $record = $this->objFromFixture($class, $identifier); - if ($delete) { - // Delete the record - $record->delete(); - } else { - // Update the record - $record->Title .= '-updated'; - $record->write(); - } + if ($delete) { + // Delete the record + $record->delete(); + } else { + // Update the record + $record->Title .= '-updated'; + $record->write(); + } - $this->assertEquals($expected, $primaryObject->stagesDifferRecursive(), 'We expect to see changes depending on the case'); + $this->assertEquals($expected, $primaryObject->stagesDifferRecursive(), 'We expect to see changes depending on the case'); + }); } public function objectsProvider(): array