From 50a00183639f6fb1816d42bafa3cb7cfb881c5ee Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Mon, 13 May 2024 14:15:37 +1200 Subject: [PATCH 1/2] FIX many_many through should allow subclasses (#11230) ```php class HomePage extends Page { private static $many_many = [ 'HeroImages' => [ 'through' => PageImageLink::class, 'from' => 'Page', 'to' => 'Image', ] ]; } ``` ```php class PageImageLink extends DataObject { private static $has_one = [ 'Page' => SiteTree::class, 'Image' => Image::class, ]; } This fails because the linking object's relation class doesn't exactly match the owner. Sharing the linking objects across various entries in the ancestry should be a supported use case. Co-authored-by: Aaron Carlino --- src/ORM/DataObjectSchema.php | 2 +- tests/php/ORM/ManyManyThroughListTest.php | 70 ++++++++++++++----- tests/php/ORM/ManyManyThroughListTest.yml | 24 +++++++ .../PseudoPolyJoinObject.php | 28 ++++++++ .../TestObjectSubclass.php | 30 ++++++++ 5 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 tests/php/ORM/ManyManyThroughListTest/PseudoPolyJoinObject.php create mode 100644 tests/php/ORM/ManyManyThroughListTest/TestObjectSubclass.php diff --git a/src/ORM/DataObjectSchema.php b/src/ORM/DataObjectSchema.php index ce77dc818..6b0acfbce 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -1239,7 +1239,7 @@ class DataObjectSchema } // Validate bad types on parent relation - if ($key === 'from' && $relationClass !== $parentClass) { + if ($key === 'from' && $relationClass !== $parentClass && !is_subclass_of($parentClass, $relationClass)) { throw new InvalidArgumentException( "many_many through relation {$parentClass}.{$component} {$key} references a field name " . "{$joinClass}::{$relation} of type {$relationClass}; {$parentClass} expected" diff --git a/tests/php/ORM/ManyManyThroughListTest.php b/tests/php/ORM/ManyManyThroughListTest.php index 9fb6e2ade..187e3023d 100644 --- a/tests/php/ORM/ManyManyThroughListTest.php +++ b/tests/php/ORM/ManyManyThroughListTest.php @@ -25,10 +25,12 @@ class ManyManyThroughListTest extends SapphireTest ManyManyThroughListTest\Item::class, ManyManyThroughListTest\JoinObject::class, ManyManyThroughListTest\TestObject::class, + ManyManyThroughListTest\TestObjectSubclass::class, ManyManyThroughListTest\PolyItem::class, ManyManyThroughListTest\PolyJoinObject::class, ManyManyThroughListTest\PolyObjectA::class, ManyManyThroughListTest\PolyObjectB::class, + ManyManyThroughListTest\PseudoPolyJoinObject::class, ManyManyThroughListTest\Locale::class, ManyManyThroughListTest\FallbackLocale::class, ]; @@ -161,46 +163,82 @@ class ManyManyThroughListTest extends SapphireTest ]; } - public function testAdd() + public function provideAdd(): array { - /** @var ManyManyThroughListTest\TestObject $parent */ - $parent = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1'); + return [ + [ + 'parentClass' => ManyManyThroughListTest\TestObject::class, + 'joinClass' => ManyManyThroughListTest\JoinObject::class, + 'joinProperty' => 'ManyManyThroughListTest_JoinObject', + 'relation' => 'Items', + ], + [ + 'parentClass' => ManyManyThroughListTest\TestObjectSubclass::class, + 'joinClass' => ManyManyThroughListTest\PseudoPolyJoinObject::class, + 'joinProperty' => 'ManyManyThroughListTest_PseudoPolyJoinObject', + 'relation' => 'MoreItems', + ], + ]; + } + + /** + * @dataProvider provideAdd + */ + public function testAdd(string $parentClass, string $joinClass, string $joinProperty, string $relation) + { + $parent = $this->objFromFixture($parentClass, 'parent1'); $newItem = new ManyManyThroughListTest\Item(); $newItem->Title = 'my new item'; $newItem->write(); - $parent->Items()->add($newItem, ['Title' => 'new join record']); + $parent->$relation()->add($newItem, ['Title' => 'new join record']); // Check select - $newItem = $parent->Items()->filter(['Title' => 'my new item'])->first(); + $newItem = $parent->$relation()->filter(['Title' => 'my new item'])->first(); $this->assertNotNull($newItem); $this->assertEquals('my new item', $newItem->Title); $this->assertInstanceOf( - ManyManyThroughListTest\JoinObject::class, + $joinClass, $newItem->getJoin() ); $this->assertInstanceOf( - ManyManyThroughListTest\JoinObject::class, - $newItem->ManyManyThroughListTest_JoinObject + $joinClass, + $newItem->$joinProperty ); - $this->assertEquals('new join record', $newItem->ManyManyThroughListTest_JoinObject->Title); + $this->assertEquals('new join record', $newItem->$joinProperty->Title); } - public function testRemove() + public function provideRemove(): array { - /** @var ManyManyThroughListTest\TestObject $parent */ - $parent = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1'); + return [ + [ + 'parentClass' => ManyManyThroughListTest\TestObject::class, + 'relation' => 'Items', + ], + [ + 'parentClass' => ManyManyThroughListTest\TestObjectSubclass::class, + 'relation' => 'MoreItems', + ], + ]; + } + + /** + * @dataProvider provideRemove + */ + public function testRemove(string $parentClass, string $relation) + { + $parent = $this->objFromFixture($parentClass, 'parent1'); $this->assertListEquals( [ ['Title' => 'item 1'], ['Title' => 'item 2'] ], - $parent->Items() + $parent->$relation() ); - $item1 = $parent->Items()->filter(['Title' => 'item 1'])->first(); - $parent->Items()->remove($item1); + $item1 = $parent->$relation()->filter(['Title' => 'item 1'])->first(); + $parent->$relation()->remove($item1); $this->assertListEquals( [['Title' => 'item 2']], - $parent->Items() + $parent->$relation() ); } diff --git a/tests/php/ORM/ManyManyThroughListTest.yml b/tests/php/ORM/ManyManyThroughListTest.yml index f1549fe89..dfa0e5193 100644 --- a/tests/php/ORM/ManyManyThroughListTest.yml +++ b/tests/php/ORM/ManyManyThroughListTest.yml @@ -3,6 +3,11 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject: Title: 'my object' parent2: Title: 'my object2' +SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObjectSubclass: + parent1: + Title: 'my object' + parent2: + Title: 'my object2' SilverStripe\ORM\Tests\ManyManyThroughListTest\Item: # Having this one first means the IDs of records aren't the same as the IDs of the join objects. child0: @@ -30,6 +35,25 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\JoinObject: Title: 'join 4' Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent2 Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2 +SilverStripe\ORM\Tests\ManyManyThroughListTest\PseudoPolyJoinObject: + join1: + Title: 'join 1' + Sort: 4 + Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObjectSubclass.parent1 + Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child1 + join2: + Title: 'join 2' + Sort: 2 + Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObjectSubclass.parent1 + Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2 + join3: + Title: 'join 3' + Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObjectSubclass.parent2 + Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child1 + join4: + Title: 'join 4' + Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObjectSubclass.parent2 + Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2 SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyObjectA: obja1: Title: 'object A1' diff --git a/tests/php/ORM/ManyManyThroughListTest/PseudoPolyJoinObject.php b/tests/php/ORM/ManyManyThroughListTest/PseudoPolyJoinObject.php new file mode 100644 index 000000000..8027578f0 --- /dev/null +++ b/tests/php/ORM/ManyManyThroughListTest/PseudoPolyJoinObject.php @@ -0,0 +1,28 @@ + 'Varchar', + 'Sort' => 'Int', + ]; + + private static $has_one = [ + 'Parent' => TestObject::class, + 'Child' => Item::class, + ]; + + private static $default_sort = '"Sort" ASC'; +} diff --git a/tests/php/ORM/ManyManyThroughListTest/TestObjectSubclass.php b/tests/php/ORM/ManyManyThroughListTest/TestObjectSubclass.php new file mode 100644 index 000000000..b3f41bccc --- /dev/null +++ b/tests/php/ORM/ManyManyThroughListTest/TestObjectSubclass.php @@ -0,0 +1,30 @@ + 'Varchar' + ]; + + private static $many_many = [ + 'MoreItems' => [ + 'through' => PseudoPolyJoinObject::class, + 'from' => 'Parent', + 'to' => 'Child', + ] + ]; +} From 12a741feee8443e7a55f1153c315b9beb01abdb5 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Wed, 15 May 2024 16:31:39 +1200 Subject: [PATCH 2/2] ENH Rendering scalars in ArrayList in templates --- src/View/SSViewer_DataPresenter.php | 2 +- src/View/SSViewer_Scope.php | 27 +++++++++++++++++++++++---- tests/php/View/SSViewerTest.php | 12 ++++++++++++ 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/View/SSViewer_DataPresenter.php b/src/View/SSViewer_DataPresenter.php index 9a65d01fa..4735bba01 100644 --- a/src/View/SSViewer_DataPresenter.php +++ b/src/View/SSViewer_DataPresenter.php @@ -376,7 +376,7 @@ class SSViewer_DataPresenter extends SSViewer_Scope // Check if the method to-be-called exists on the target object - if so, don't check any further // injection locations - $on = $this->itemIterator ? $this->itemIterator->current() : $this->item; + $on = $this->getItem(); if (is_object($on) && (isset($on->$property) || method_exists($on, $property ?? ''))) { return []; } diff --git a/src/View/SSViewer_Scope.php b/src/View/SSViewer_Scope.php index aa4ee2065..3b0fe1a5e 100644 --- a/src/View/SSViewer_Scope.php +++ b/src/View/SSViewer_Scope.php @@ -5,6 +5,11 @@ namespace SilverStripe\View; use ArrayIterator; use Countable; use Iterator; +use SilverStripe\ORM\FieldType\DBBoolean; +use SilverStripe\ORM\FieldType\DBText; +use SilverStripe\ORM\FieldType\DBFloat; +use SilverStripe\ORM\FieldType\DBInt; +use SilverStripe\ORM\FieldType\DBField; /** * This tracks the current scope for an SSViewer instance. It has three goals: @@ -121,7 +126,11 @@ class SSViewer_Scope */ public function getItem() { - return $this->itemIterator ? $this->itemIterator->current() : $this->item; + $item = $this->itemIterator ? $this->itemIterator->current() : $this->item; + if (is_scalar($item)) { + $item = $this->convertScalarToDBField($item); + } + return $item; } /** @@ -176,7 +185,7 @@ class SSViewer_Scope */ public function getObj($name, $arguments = [], $cache = false, $cacheName = null) { - $on = $this->itemIterator ? $this->itemIterator->current() : $this->item; + $on = $this->getItem(); return $on->obj($name, $arguments, $cache, $cacheName); } @@ -240,7 +249,7 @@ class SSViewer_Scope */ public function self() { - $result = $this->itemIterator ? $this->itemIterator->current() : $this->item; + $result = $this->getItem(); $this->resetLocalScope(); return $result; @@ -338,7 +347,7 @@ class SSViewer_Scope */ public function __call($name, $arguments) { - $on = $this->itemIterator ? $this->itemIterator->current() : $this->item; + $on = $this->getItem(); $retval = $on ? $on->$name(...$arguments) : null; $this->resetLocalScope(); @@ -368,4 +377,14 @@ class SSViewer_Scope { return $this->upIndex; } + + private function convertScalarToDBField(bool|string|float|int $value): DBField + { + return match (gettype($value)) { + 'boolean' => DBBoolean::create()->setValue($value), + 'string' => DBText::create()->setValue($value), + 'double' => DBFloat::create()->setValue($value), + 'integer' => DBInt::create()->setValue($value), + }; + } } diff --git a/tests/php/View/SSViewerTest.php b/tests/php/View/SSViewerTest.php index ce91527f3..e5307a7c1 100644 --- a/tests/php/View/SSViewerTest.php +++ b/tests/php/View/SSViewerTest.php @@ -2218,4 +2218,16 @@ EOC; $this->assertTrue(file_exists($cacheFile ?? ''), 'Cache file wasn\'t created when it was meant to'); unlink($cacheFile ?? ''); } + + public function testPrimitivesConvertedToDBFields() + { + $data = new ArrayData([ + // null value should not be rendered, though should also not throw exception + 'Foo' => new ArrayList(['hello', true, 456, 7.89, null]) + ]); + $this->assertEqualIgnoringWhitespace( + 'hello 1 456 7.89', + $this->render('<% loop $Foo %>$Me<% end_loop %>', $data) + ); + } }