From fcfbb752c70af81d15538301803fd194657b0096 Mon Sep 17 00:00:00 2001 From: Aaron Carlino Date: Sat, 20 Apr 2019 11:42:24 +1200 Subject: [PATCH] FIX many_many through should allow subclasses ```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. --- 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 ce77dc818a3..6b0acfbce24 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -1239,7 +1239,7 @@ protected function checkManyManyFieldClass($parentClass, $component, $joinClass, } // 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 9fb6e2ade5e..187e3023da2 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 @@ public function sortingProvider() ]; } - 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 f1549fe8935..dfa0e51938c 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 00000000000..8027578f06b --- /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 00000000000..b3f41bccce7 --- /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', + ] + ]; +}