Merge branch '4.3' into 4.4

src/ORM/DataObject.php

@@ -1510,9 +1510,11 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      * @param boolean       $showDebug Show debugging information
      * @param boolean       $forceInsert Run INSERT command rather than UPDATE, even if record already exists
      * @param boolean       $forceWrite Write to database even if there are no changes
-     * @param boolean $writeComponents Call write() on all associated component instances which were previously
+     * @param boolean|array $writeComponents Call write() on all associated component instances which were previously
      *                      retrieved through {@link getComponent()}, {@link getComponents()} or
-     *                                 {@link getManyManyComponents()} (Default: false)
+     *                      {@link getManyManyComponents()}. Default to `false`. The parameter can also be provided in
+     *                      the form of an array: `['recursive' => true, skip => ['Page'=>[1,2,3]]`. This avoid infinite
+     *                      loops when one DataObject are components of each other.
      * @return int The ID of the record
      * @throws ValidationException Exception that can be caught and handled by the calling function
      */
@@ -1563,7 +1565,15 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
 
         // Write relations as necessary
         if ($writeComponents) {
-            $this->writeComponents(true);
+            $recursive = true;
+            $skip = [];
+            if (is_array($writeComponents)) {
+                $recursive = isset($writeComponents['recursive']) && $writeComponents['recursive'];
+                $skip = isset($writeComponents['skip']) && is_array($writeComponents['skip'])
+                    ? $writeComponents['skip']
+                    : [];
+            }
+            $this->writeComponents($recursive, $skip);
         }
 
         // Clears the cache for this object so get_one returns the correct object.
@@ -1595,21 +1605,70 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      * same record.
      *
      * @param bool $recursive Recursively write components
+     * @param array $skip List of DataObject references to skip
      * @return DataObject $this
      */
-    public function writeComponents($recursive = false)
+    public function writeComponents($recursive = false, $skip = [])
     {
+        // Make sure we add our current object to the skip list
+        $this->skipWriteComponents($recursive, $this, $skip);
+
+        // All our write calls have the same arguments ... just need make sure the skip list is pass by reference
+        $args = [
+            false, false, false,
+            $recursive ? ["recursive" => $recursive, "skip" => &$skip] : false
+        ];
+
         foreach ($this->components as $component) {
-            $component->write(false, false, false, $recursive);
+            if (!$this->skipWriteComponents($recursive, $component, $skip)) {
+                $component->write(...$args);
+            }
         }
 
         if ($join = $this->getJoin()) {
-            $join->write(false, false, false, $recursive);
+            if (!$this->skipWriteComponents($recursive, $join, $skip)) {
+                $join->write(...$args);
+            }
         }
 
         return $this;
     }
 
+    /**
+     * Check if target is in the skip list and add it if it isn't.
+     * @param bool $recursive
+     * @param DataObject $target
+     * @param array $skip
+     * @return bool Whether the target is already in the list
+     */
+    private function skipWriteComponents($recursive, DataObject $target, array &$skip)
+    {
+        // We only care about the skip list if our call is meant to be recursive
+        if (!$recursive) {
+            return false;
+        }
+
+        // Get our Skip array keys
+        $classname = get_class($target);
+        $id = $target->ID;
+
+        // Check if the target is in the skip list
+        if (isset($skip[$classname])) {
+            if (in_array($id, $skip[$classname])) {
+                // Skip the object
+                return true;
+            }
+        } else {
+            // This is the first object of this class
+            $skip[$classname] = [];
+        }
+
+        // Add the target to our skip list
+        $skip[$classname][] = $id;
+
+        return false;
+    }
+
     /**
      * Delete this data object.
      * $this->onBeforeDelete() gets called.

src/View/ThemeResourceLoader.php

@@ -236,11 +236,15 @@ class ThemeResourceLoader implements Flushable
             foreach ($themePaths as $themePath) {
                 // Join path
                 $pathParts = [ $this->base, $themePath, 'templates', $head, $type, $tail ];
+                try {
                     $path = Path::join($pathParts) . '.ss';
                     if (file_exists($path)) {
                         $this->getCache()->set($cacheKey, $path);
                         return $path;
                     }
+                } catch (InvalidArgumentException $e) {
+                    // No-op
+                }
             }
         }
 

tests/php/ORM/DataObjectTest.php

@@ -19,6 +19,7 @@ use SilverStripe\ORM\FieldType\DBVarchar;
 use SilverStripe\ORM\ManyManyList;
 use SilverStripe\ORM\Tests\DataObjectTest\Company;
 use SilverStripe\ORM\Tests\DataObjectTest\Player;
+use SilverStripe\ORM\Tests\DataObjectTest\TreeNode;
 use SilverStripe\Security\Member;
 use SilverStripe\View\ViewableData;
 use stdClass;
@@ -58,7 +59,8 @@ class DataObjectTest extends SapphireTest
         DataObjectTest\RelationParent::class,
         DataObjectTest\RelationChildFirst::class,
         DataObjectTest\RelationChildSecond::class,
-        DataObjectTest\MockDynamicAssignmentDataObject::class
+        DataObjectTest\MockDynamicAssignmentDataObject::class,
+        DataObjectTest\TreeNode::class,
     );
 
     protected function setUp()
@@ -2417,4 +2419,93 @@ class DataObjectTest extends SapphireTest
 
         $do->write();
     }
+
+    public function testRecursiveWrite()
+    {
+
+        $root = $this->objFromFixture(TreeNode::class, 'root');
+        $child = $this->objFromFixture(TreeNode::class, 'child');
+        $grandchild = $this->objFromFixture(TreeNode::class, 'grandchild');
+
+        // Create a cycle ... this will test that we can't create an infinite loop
+        $root->CycleID = $grandchild->ID;
+        $root->write();
+
+        // Our count will have been set while loading our fixtures, let's reset eveything back to 0
+        TreeNode::singleton()->resetCounts();
+        $root = TreeNode::get()->byID($root->ID);
+        $child = TreeNode::get()->byID($child->ID);
+        $grandchild = TreeNode::get()->byID($grandchild->ID);
+        $this->assertEquals(0, $root->WriteCount, 'Root node write count has been reset');
+        $this->assertEquals(0, $child->WriteCount, 'Child node write count has been reset');
+        $this->assertEquals(0, $grandchild->WriteCount, 'Grand Child node write count has been reset');
+
+        // Trigger a recursive write of the grand children
+        $grandchild->write(false, false, false, true);
+
+        // Reload the DataObject from the DB to get the new Write Counts
+        $root = TreeNode::get()->byID($root->ID);
+        $child = TreeNode::get()->byID($child->ID);
+        $grandchild = TreeNode::get()->byID($grandchild->ID);
+
+        $this->assertEquals(
+            1,
+            $grandchild->WriteCount,
+            'Grand child has been written once because write was directly called on it'
+        );
+        $this->assertEquals(
+            1,
+            $child->WriteCount,
+            'Child should has been written once because it is directly related to grand child'
+        );
+        $this->assertEquals(
+            1,
+            $root->WriteCount,
+            'Root should have been written once because it is indirectly related to grand child'
+        );
+    }
+
+    public function testShallowRecursiveWrite()
+    {
+        $root = $this->objFromFixture(TreeNode::class, 'root');
+        $child = $this->objFromFixture(TreeNode::class, 'child');
+        $grandchild = $this->objFromFixture(TreeNode::class, 'grandchild');
+
+        // Create a cycle ... this will test that we can't create an infinite loop
+        $root->CycleID = $grandchild->ID;
+        $root->write();
+
+        // Our count will have been set while loading our fixtures, let's reset eveything back to 0
+        TreeNode::singleton()->resetCounts();
+        $root = TreeNode::get()->byID($root->ID);
+        $child = TreeNode::get()->byID($child->ID);
+        $grandchild = TreeNode::get()->byID($grandchild->ID);
+        $this->assertEquals(0, $root->WriteCount);
+        $this->assertEquals(0, $child->WriteCount);
+        $this->assertEquals(0, $grandchild->WriteCount);
+
+        // Recursively only affect component that have been loaded
+        $grandchild->write(false, false, false, ['recursive' => false]);
+
+        // Reload the DataObject from the DB to get the new Write Counts
+        $root = TreeNode::get()->byID($root->ID);
+        $child = TreeNode::get()->byID($child->ID);
+        $grandchild = TreeNode::get()->byID($grandchild->ID);
+
+        $this->assertEquals(
+            1,
+            $grandchild->WriteCount,
+            'Grand child was written once because write was directly called on it'
+        );
+        $this->assertEquals(
+            1,
+            $child->WriteCount,
+            'Child was written once because it is directly related grand child'
+        );
+        $this->assertEquals(
+            0,
+            $root->WriteCount,
+            'Root is 2 step remove from grand children. It was not written on a shallow recursive write.'
+        );
+    }
 }

tests/php/ORM/DataObjectTest.yml

@@ -179,3 +179,13 @@ SilverStripe\ORM\Tests\DataObjectTest\RelationChildFirst:
     ManyNext:
       - =>SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond.test1
       - =>SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond.test3
+
+SilverStripe\ORM\Tests\DataObjectTest\TreeNode:
+  root:
+    Title: Root
+  child:
+    Title: Children
+    Parent: =>SilverStripe\ORM\Tests\DataObjectTest\TreeNode.root
+  grandchild:
+    Title: GrandChildren
+    Parent: =>SilverStripe\ORM\Tests\DataObjectTest\TreeNode.child

tests/php/ORM/DataObjectTest/TreeNode.php

@@ -0,0 +1,59 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\DataObjectTest;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\Connect\Query;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\DB;
+use SilverStripe\ORM\HasManyList;
+use SilverStripe\ORM\Hierarchy\Hierarchy;
+use SilverStripe\ORM\ManyManyList;
+use SilverStripe\ORM\Queries\SQLUpdate;
+
+/**
+ * The purpose of this test class is to test recursive writes and make sure we don't get stuck in an infinite loop.
+ * @property int $WriteCount Number of times this object was written sine the last call of `resetCount`
+ */
+class TreeNode extends DataObject implements TestOnly
+{
+    private static $table_name = 'DataObjectTest_TreeNode';
+
+    private static $db = [
+        'Title' => 'Varchar',
+        'WriteCount' => 'Int'
+    ];
+
+    private static $has_one = [
+        'Parent' => self::class,
+        'Cycle' => self::class,
+    ];
+
+    private static $has_many = [
+        'Children' => self::class,
+    ];
+
+    public function write($showDebug = false, $forceInsert = false, $forceWrite = false, $writeComponents = false)
+    {
+        // Force the component to fetch its Parent and Cycle relation so we have components to recursively write
+        $this->Parent;
+        $this->Cycle;
+
+        // Count a write attempts
+        $this->WriteCount++;
+
+        return parent::write($showDebug, $forceInsert, $forceWrite, $writeComponents);
+    }
+
+    /**
+     * Reset the WriteCount on all TreeNodes
+     */
+    public function resetCounts()
+    {
+        $update = new SQLUpdate(
+            sprintf('"%s"', self::baseTable()),
+            ['"WriteCount"' => 0]
+        );
+        $results = $update->execute();
+    }
+}