From b1551a687d99fee6714f649b28a64e10bd05caeb Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Fri, 21 Jun 2019 09:40:18 +0100 Subject: [PATCH 1/2] Catch Path::join() exceptions in findTemplate() (fixes #9084) --- src/View/ThemeResourceLoader.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/View/ThemeResourceLoader.php b/src/View/ThemeResourceLoader.php index 60945e5f2..303b28854 100644 --- a/src/View/ThemeResourceLoader.php +++ b/src/View/ThemeResourceLoader.php @@ -218,9 +218,13 @@ class ThemeResourceLoader foreach ($themePaths as $themePath) { // Join path $pathParts = [ $this->base, $themePath, 'templates', $head, $type, $tail ]; - $path = Path::join($pathParts) . '.ss'; - if (file_exists($path)) { - return $path; + try { + $path = Path::join($pathParts) . '.ss'; + if (file_exists($path)) { + return $path; + } + } catch (InvalidArgumentException $e) { + // No-op } } } From 591b88a9bc05b40a7ce3604283b9b7cb684f88cc Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Tue, 10 Sep 2019 14:15:28 +1200 Subject: [PATCH 2/2] BUG Allow infinite loop when calling DataObject::writeComponent() recursively --- src/ORM/DataObject.php | 79 ++++++++++++++++--- tests/php/ORM/DataObjectTest.php | 94 ++++++++++++++++++++++- tests/php/ORM/DataObjectTest.yml | 10 +++ tests/php/ORM/DataObjectTest/TreeNode.php | 59 ++++++++++++++ 4 files changed, 231 insertions(+), 11 deletions(-) create mode 100644 tests/php/ORM/DataObjectTest/TreeNode.php diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 8fb69232d..e0ed2aeb8 100644 --- a/src/ORM/DataObject.php +++ b/src/ORM/DataObject.php @@ -1502,12 +1502,14 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity * * @uses DataExtension->augmentWrite() * - * @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 - * retrieved through {@link getComponent()}, {@link getComponents()} or - * {@link getManyManyComponents()} (Default: false) + * @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|array $writeComponents Call write() on all associated component instances which were previously + * retrieved through {@link getComponent()}, {@link getComponents()} or + * {@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 */ @@ -1553,7 +1555,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. @@ -1585,21 +1595,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. diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index a763e215c..bd4643556 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -16,10 +16,12 @@ use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\FieldType\DBField; use SilverStripe\ORM\FieldType\DBPolymorphicForeignKey; use SilverStripe\ORM\FieldType\DBVarchar; +use SilverStripe\ORM\Hierarchy\Hierarchy; use SilverStripe\ORM\ManyManyList; use SilverStripe\ORM\Tests\DataObjectTest\Company; use SilverStripe\ORM\Tests\DataObjectTest\Player; use SilverStripe\ORM\Tests\DataObjectTest\Team; +use SilverStripe\ORM\Tests\DataObjectTest\TreeNode; use SilverStripe\View\ViewableData; use stdClass; @@ -58,7 +60,8 @@ class DataObjectTest extends SapphireTest DataObjectTest\RelationParent::class, DataObjectTest\RelationChildFirst::class, DataObjectTest\RelationChildSecond::class, - DataObjectTest\MockDynamicAssignmentDataObject::class + DataObjectTest\MockDynamicAssignmentDataObject::class, + DataObjectTest\TreeNode::class, ); public static function getExtraDataObjects() @@ -2284,4 +2287,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.' + ); + } } diff --git a/tests/php/ORM/DataObjectTest.yml b/tests/php/ORM/DataObjectTest.yml index 139f654c2..186a88517 100644 --- a/tests/php/ORM/DataObjectTest.yml +++ b/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 diff --git a/tests/php/ORM/DataObjectTest/TreeNode.php b/tests/php/ORM/DataObjectTest/TreeNode.php new file mode 100644 index 000000000..e497748b2 --- /dev/null +++ b/tests/php/ORM/DataObjectTest/TreeNode.php @@ -0,0 +1,59 @@ + '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(); + } +}