BUGFIX many many through not sorting by join table (#8534)

* BUGFIX many many through not sorting by join table

* #8534 added docs to support many many sorting fix

* #8534 added test cases for many_many default sorting

docs/en/02_Developer_Guides/00_Model/02_Relations.md

@@ -295,6 +295,15 @@ class Supporter extends DataObject
 }
 ```
 
+To ensure this `many_many` is sorted by "Ranking" by default you can add this to your config:
+
+```yaml
+Team_Supporters:
+  default_sort: '"Team_Supporter"."Ranking" ASC'
+```
+
+`Team_Supporters` is the table name automatically generated for the many_many relation in this case.
+
 ### many_many through relationship joined on a separate DataObject
 
 If necessary, a third DataObject class can instead be specified as the joining table,
@@ -312,6 +321,9 @@ This is declared via array syntax, with the following keys on the many_many:
  - `from` Name of the has_one relationship pointing back at the object declaring many_many
  - `to` Name of the has_one relationship pointing to the object declaring belongs_many_many.
 
+Just like a any normal DataObject, you can apply a default sort which will be applied when
+accessing many many through relations.
+
 Note: The `through` class must not also be the name of any field or relation on the parent
 or child record.
 
@@ -348,6 +360,8 @@ class TeamSupporter extends DataObject
         'Team' => Team::class,
         'Supporter' => Supporter::class,
     ];
+
+    private static $default_sort = '"TeamSupporter"."Ranking" ASC'
 }
 ```
 
@@ -468,6 +482,7 @@ the best way to think about it is that the object where the relationship will be
 Product => Categories, the `Product` should contain the `many_many`, because it is much
 more likely that the user will select Categories for a Product than vice-versa.
 
+
 ## Cascading deletions
 
 Relationships between objects can cause cascading deletions, if necessary, through configuration of the

src/ORM/DataObject.php

@@ -2051,6 +2051,12 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
             $query->setQueryParam('Component.ExtraFields', $extraFields);
         });
 
+        // If we have a default sort set for our "join" then we should overwrite any default already set.
+        $joinSort = Config::inst()->get($manyManyComponent['join'], 'default_sort');
+        if (!empty($joinSort)) {
+            $result = $result->sort($joinSort);
+        }
+
         $this->extend('updateManyManyComponents', $result);
 
         // If this is called on a singleton, then we return an 'orphaned relation' that can have the

src/ORM/ManyManyThroughQueryManipulator.php

@@ -254,13 +254,6 @@ class ManyManyThroughQueryManipulator implements DataQueryManipulator
             );
         }
 
-        // Set a default sort from the join model if available and nothing is already set
-        if (empty($sqlSelect->getOrderBy())
-            && $sort = Config::inst()->get($this->getJoinClass(), 'default_sort')
-        ) {
-            $sqlSelect->setOrderBy($sort);
-        }
-
         // Apply join and record sql for later insertion (at end of replacements)
         // By using a string placeholder $$_SUBQUERY_$$ we protect field/table rewrites from interfering twice
         // on the already-finalised inner list

tests/php/ORM/ManyManyListTest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\ORM\Tests;
 
+use SilverStripe\Core\Config\Config;
 use SilverStripe\ORM\FieldType\DBMoney;
 use SilverStripe\ORM\ManyManyList;
 use SilverStripe\Core\Convert;
@@ -369,6 +370,46 @@ class ManyManyListTest extends SapphireTest
         $this->assertSQLEquals($expected, $list->sql($parameters));
     }
 
+    /**
+     * This tests that we can set a default sort on a join table, even though the class doesn't exist.
+     *
+     * @return void
+     */
+    public function testSortByExtraFieldsDefaultSort()
+    {
+        $obj = new ManyManyListTest\ExtraFieldsObject();
+        $obj->write();
+
+        $obj2 = new ManyManyListTest\ExtraFieldsObject();
+        $obj2->write();
+
+        $money = new DBMoney();
+        $money->setAmount(100);
+        $money->setCurrency('USD');
+
+        // Add two objects as relations (first is linking back to itself)
+        $obj->Clients()->add($obj, ['Worth' => $money, 'Reference' => 'A']);
+        $obj->Clients()->add($obj2, ['Worth' => $money, 'Reference' => 'B']);
+
+        // Set the default sort for this relation
+        Config::inst()->update('ManyManyListTest_ExtraFields_Clients', 'default_sort', 'Reference ASC');
+        $clients = $obj->Clients();
+        $this->assertCount(2, $clients);
+
+        list($first, $second) = $obj->Clients();
+        $this->assertEquals('A', $first->Reference);
+        $this->assertEquals('B', $second->Reference);
+
+        // Now we ensure the default sort is being respected by reversing its order
+        Config::inst()->update('ManyManyListTest_ExtraFields_Clients', 'default_sort', 'Reference DESC');
+        $reverseClients = $obj->Clients();
+        $this->assertCount(2, $reverseClients);
+
+        list($reverseFirst, $reverseSecond) = $obj->Clients();
+        $this->assertEquals('B', $reverseFirst->Reference);
+        $this->assertEquals('A', $reverseSecond->Reference);
+    }
+
     public function testFilteringOnPreviouslyJoinedTable()
     {
         /** @var ManyManyListTest\Category $category */

tests/php/ORM/ManyManyThroughListTest.php

@@ -2,11 +2,14 @@
 
 namespace SilverStripe\ORM\Tests;
 
+use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\ManyManyThroughList;
 use SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyItem;
 use SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyJoinObject;
+use SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale;
+use SilverStripe\ORM\Tests\ManyManyThroughListTest\FallbackLocale;
 
 class ManyManyThroughListTest extends SapphireTest
 {
@@ -20,6 +23,8 @@ class ManyManyThroughListTest extends SapphireTest
         ManyManyThroughListTest\PolyJoinObject::class,
         ManyManyThroughListTest\PolyObjectA::class,
         ManyManyThroughListTest\PolyObjectB::class,
+        ManyManyThroughListTest\Locale::class,
+        ManyManyThroughListTest\FallbackLocale::class,
     ];
 
     protected function setUp()
@@ -320,4 +325,32 @@ class ManyManyThroughListTest extends SapphireTest
         $this->assertEquals($joinTable, $objB1->Items()->getJoinTable());
         $this->assertEquals($joinTable, $objB2->Items()->getJoinTable());
     }
+
+    /**
+     * This tests that default sort works when the join table has a default sort set, and the main
+     * dataobject has a default sort set.
+     *
+     * @return void
+     */
+    public function testDefaultSortOnJoinAndMain()
+    {
+        // We have spanish mexico with two fall back locales; argentina and international sorted in that order.
+        $mexico = $this->objFromFixture(Locale::class, 'mexico');
+
+        $fallbacks = $mexico->Fallbacks();
+        $this->assertCount(2, $fallbacks);
+
+        // Ensure the default sort is is correct
+        list($first, $second) = $fallbacks;
+        $this->assertSame('Argentina', $first->Title);
+        $this->assertSame('International', $second->Title);
+
+        // Ensure that we're respecting the default sort by reversing it
+        Config::inst()->update(FallbackLocale::class, 'default_sort', '"ManyManyThroughTest_FallbackLocale"."Sort" DESC');
+
+        $reverse = $mexico->Fallbacks();
+        list($firstReverse, $secondReverse) = $reverse;
+        $this->assertSame('International', $firstReverse->Title);
+        $this->assertSame('Argentina', $secondReverse->Title);
+    }
 }

tests/php/ORM/ManyManyThroughListTest.yml

@@ -51,3 +51,28 @@ SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyJoinObject:
     Sort: 2
     Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyObjectB.objb2
     Child: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\PolyItem.child2
+SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale:
+  international:
+    Title: 'International'
+    Locale: 'en_NZ'
+    URLSegment: 'international'
+    IsGlobalDefault: 1
+  mexico:
+    Title: 'Mexico'
+    Locale: 'es_MX'
+    URLSegment: 'mexico'
+    IsGlobalDefault: 0
+  argentina:
+    Title: 'Argentina'
+    Locale: 'es_AR'
+    URLSegment: 'argentina'
+    IsGlobalDefault: 0
+SilverStripe\ORM\Tests\ManyManyThroughListTest\FallbackLocale:
+  mexico_international:
+    Sort: 2
+    Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.mexico
+    Locale: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.international
+  mexico_argentina:
+    Sort: 1
+    Parent: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.mexico
+    Locale: =>SilverStripe\ORM\Tests\ManyManyThroughListTest\Locale.argentina

tests/php/ORM/ManyManyThroughListTest/FallbackLocale.php

@@ -0,0 +1,22 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\ManyManyThroughListTest;
+
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Dev\TestOnly;
+
+class FallbackLocale extends DataObject implements TestOnly
+{
+    private static $db = [
+        'Sort' => 'Int',
+    ];
+
+    private static $has_one = [
+        'Parent' => Locale::class,
+        'Locale' => Locale::class,
+    ];
+
+    private static $table_name = 'ManyManyThroughTest_FallbackLocale';
+
+    private static $default_sort = 'Sort';
+}

tests/php/ORM/ManyManyThroughListTest/Locale.php

@@ -0,0 +1,36 @@
+<?php
+
+namespace SilverStripe\ORM\Tests\ManyManyThroughListTest;
+
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Dev\TestOnly;
+
+class Locale extends DataObject implements TestOnly
+{
+    private static $table_name = 'ManyManyThroughTest_Locale';
+
+    /**
+     * @config
+     * @var array
+     */
+    private static $db = [
+        'Title' => 'Varchar(100)',
+        'Locale' => 'Varchar(10)',
+        'URLSegment' => 'Varchar(100)',
+        'IsGlobalDefault' => 'Boolean',
+    ];
+
+    private static $has_many = [
+        'FallbackLocales' => FallbackLocale::class . '.Parent',
+    ];
+
+    private static $many_many = [
+        'Fallbacks' => [
+            'through' => FallbackLocale::class,
+            'from' => 'Parent',
+            'to' => 'Locale',
+        ],
+    ];
+
+    private static $default_sort = '"ManyManyThroughTest_Locale"."Locale" ASC';
+}