Merge pull request #10295 from creative-commoners/pulls/4.9/manymanythrough-remove-all

FIX Correctly remove relations with ManyManyThroughList::removeall

src/ORM/ManyManyList.php

@@ -389,7 +389,6 @@ class ManyManyList extends RelationList
      */
     public function removeAll()
     {
-
         // Remove the join to the join table to avoid MySQL row locking issues.
         $query = $this->dataQuery();
         $foreignFilter = $query->getQueryParam('Foreign.Filter');

src/ORM/ManyManyThroughList.php

@@ -130,27 +130,37 @@ class ManyManyThroughList extends RelationList
         // Find has_many row with a local key matching the given id
         $hasManyList = $this->manipulator->getParentRelationship($this->dataQuery());
         $records = $hasManyList->filter($this->manipulator->getLocalKey(), $itemID);
-        $affectedIds = [];
 
         // Rather than simple un-associating the record (as in has_many list)
         // Delete the actual mapping row as many_many deletions behave.
         /** @var DataObject $record */
         foreach ($records as $record) {
-            $affectedIds[] = $record->ID;
             $record->delete();
         }
 
-        if ($this->removeCallbacks && $affectedIds) {
-            $this->removeCallbacks->call($this, $affectedIds);
+        if ($this->removeCallbacks && $itemID) {
+            $this->removeCallbacks->call($this, [$itemID]);
         }
     }
 
     public function removeAll()
     {
-        // Empty has_many table matching the current foreign key
-        $hasManyList = $this->manipulator->getParentRelationship($this->dataQuery());
-        $affectedIds = $hasManyList->column('ID');
-        $hasManyList->removeAll();
+        // Get the IDs of records in the current list
+        $affectedIds = $this->limit(null)->column('ID');
+        if (empty($affectedIds)) {
+            return;
+        }
+
+        // Get the join records that apply for the current list
+        $records = $this->manipulator->getJoinClass()::get()->filter([
+            $this->manipulator->getForeignIDKey() => $this->getForeignID(),
+            $this->manipulator->getLocalKey() => $affectedIds,
+        ]);
+
+        /** @var DataObject $record */
+        foreach ($records as $record) {
+            $record->delete();
+        }
 
         if ($this->removeCallbacks && $affectedIds) {
             $this->removeCallbacks->call($this, $affectedIds);

src/ORM/ManyManyThroughQueryManipulator.php

@@ -3,7 +3,6 @@
 
 namespace SilverStripe\ORM;
 
-use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Injector\Injectable;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Dev\Deprecation;

tests/php/ORM/ManyManyThroughListTest.php

@@ -202,6 +202,64 @@ class ManyManyThroughListTest extends SapphireTest
         );
     }
 
+    public function testRemoveAll()
+    {
+        $first = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1');
+        $first->Items()->add($this->objFromFixture(ManyManyThroughListTest\Item::class, 'child0'));
+        $second = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent2');
+
+        $firstItems = $first->Items();
+        $secondItems = $second->Items();
+        $initialJoins = ManyManyThroughListTest\JoinObject::get()->count();
+        $initialItems = ManyManyThroughListTest\Item::get()->count();
+        $initialRelations = $firstItems->count();
+        $initialSecondListRelations = $secondItems->count();
+
+        $firstItems->removeAll();
+
+        // Validate all items were removed from the first list, but none were removed from the second list
+        $this->assertEquals(0, count($firstItems));
+        $this->assertEquals($initialSecondListRelations, count($secondItems));
+
+        // Validate that the JoinObjects were actually removed from the database
+        $this->assertEquals($initialJoins - $initialRelations, ManyManyThroughListTest\JoinObject::get()->count());
+
+        // Confirm Item objects were not removed from the database
+        $this->assertEquals($initialItems, ManyManyThroughListTest\Item::get()->count());
+    }
+
+    public function testRemoveAllIgnoresLimit()
+    {
+        $parent = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1');
+        $parent->Items()->add($this->objFromFixture(ManyManyThroughListTest\Item::class, 'child0'));
+        $initialJoins = ManyManyThroughListTest\JoinObject::get()->count();
+        // Validate there are enough items in the relation for this test
+        $this->assertTrue($initialJoins > 1);
+
+        $items = $parent->Items()->Limit(1);
+        $items->removeAll();
+
+        // Validate all items were removed from the list - not only one
+        $this->assertEquals(0, count($items));
+    }
+
+    public function testFilteredRemoveAll()
+    {
+        $parent = $this->objFromFixture(ManyManyThroughListTest\TestObject::class, 'parent1');
+        $parent->Items()->add($this->objFromFixture(ManyManyThroughListTest\Item::class, 'child0'));
+        $items = $parent->Items();
+        $initialJoins = ManyManyThroughListTest\JoinObject::get()->count();
+        $initialRelations = $items->count();
+
+        $items->filter('Title:not', 'not filtered')->removeAll();
+
+        // Validate only the filtered items were removed
+        $this->assertEquals(1, $items->count());
+
+        // Validate the list only contains the correct remaining item
+        $this->assertEquals(['not filtered'], $items->column('Title'));
+    }
+
     /**
      * Test validation
      *

tests/php/ORM/ManyManyThroughListTest.yml

@@ -1,7 +1,12 @@
 SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject:
   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:
+    Title: 'not filtered'
   child1:
     Title: 'item 1'
   child2:
@@ -17,6 +22,14 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\JoinObject:
     Sort: 2
     Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent1
     Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2
+  join3:
+    Title: 'join 3'
+    Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent2
+    Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child1
+  join4:
+    Title: 'join 4'
+    Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\TestObject.parent2
+    Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Item.child2
 SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyObjectA:
   obja1:
     Title: 'object A1'
@@ -75,4 +88,4 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\FallbackLocale:
   mexico_argentina:
     Sort: 1
     Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.mexico
-    Locale: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.argentina
+    Locale: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.argentina