Fixes.

silverstripe · Oct 17, 2023 · eb048d5 · eb048d5
1 parent f3906ee
commit eb048d5
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 75 deletions.
diff --git 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,18 +55,19 @@ 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];
     }
 
     /**
      * Execution ownership hierarchy traversal and inspect individual models
      *
      * @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
@@ -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