PR fixes.

silverstripe · Oct 18, 2023 · d6490ce · d6490ce
1 parent d2efe5a
commit d6490ce
Show file tree

Hide file tree

Showing 7 changed files with 26 additions and 103 deletions.
diff --git 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
@@ -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
diff --git a/src/RecursiveStagesInterface.php b/src/RecursiveStagesInterface.php
@@ -14,9 +14,6 @@ interface RecursiveStagesInterface
 {
     /**
      * Determine if content differs on stages including nested objects
-     *
-     * @param DataObject $object
-     * @return bool
      */
     public function stagesDifferRecursive(DataObject $object): bool;
 }
diff --git a/src/RecursiveStagesService.php b/src/RecursiveStagesService.php
@@ -2,8 +2,6 @@
 
 namespace SilverStripe\Versioned;
 
-use Exception;
-use Psr\Container\NotFoundExceptionInterface;
 use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Core\Resettable;
@@ -25,10 +23,6 @@ public function flushCachedData(): void
         $this->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
@@ -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
@@ -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