FIX Correctly remove relations with ManyManyThroughList::removeall

Instead of just setting one side of the relation to null in the through
list, remove the rows entirely.
Remove only the relations which match the filters that have already been
set on the list.
This is consistent with the way ManyManyList works.

Also some small tidy-up (removing an unnecessary line break and an
unused "use" statement)

src/ORM/ManyManyList.php

@@ -389,7 +389,6 @@ public function removeByID($itemID)
      */
     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 @@ public function removeByID($itemID)
         // 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 @@ public function testRemove()
         );
     }
 
+    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