From 55f95b7bc8f91384df459bd70c87cacf92225f68 Mon Sep 17 00:00:00 2001 From: Michael Strong Date: Thu, 1 Nov 2018 13:42:27 +1300 Subject: [PATCH] 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 --- .../00_Model/02_Relations.md | 15 +++++++ src/ORM/DataObject.php | 6 +++ src/ORM/ManyManyThroughQueryManipulator.php | 7 ---- tests/php/ORM/ManyManyListTest.php | 41 +++++++++++++++++++ tests/php/ORM/ManyManyThroughListTest.php | 33 +++++++++++++++ tests/php/ORM/ManyManyThroughListTest.yml | 25 +++++++++++ .../FallbackLocale.php | 22 ++++++++++ .../ORM/ManyManyThroughListTest/Locale.php | 36 ++++++++++++++++ 8 files changed, 178 insertions(+), 7 deletions(-) create mode 100644 tests/php/ORM/ManyManyThroughListTest/FallbackLocale.php create mode 100644 tests/php/ORM/ManyManyThroughListTest/Locale.php diff --git a/docs/en/02_Developer_Guides/00_Model/02_Relations.md b/docs/en/02_Developer_Guides/00_Model/02_Relations.md index bc0876f4f..938de1a9d 100644 --- a/docs/en/02_Developer_Guides/00_Model/02_Relations.md +++ b/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 diff --git a/src/ORM/DataObject.php b/src/ORM/DataObject.php index 1092afc32..a47034381 100644 --- a/src/ORM/DataObject.php +++ b/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 diff --git a/src/ORM/ManyManyThroughQueryManipulator.php b/src/ORM/ManyManyThroughQueryManipulator.php index 2024a65aa..6fffbbe60 100644 --- a/src/ORM/ManyManyThroughQueryManipulator.php +++ b/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 diff --git a/tests/php/ORM/ManyManyListTest.php b/tests/php/ORM/ManyManyListTest.php index aa6218474..564864f35 100644 --- a/tests/php/ORM/ManyManyListTest.php +++ b/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 */ diff --git a/tests/php/ORM/ManyManyThroughListTest.php b/tests/php/ORM/ManyManyThroughListTest.php index b85c6317b..0b18cbec5 100644 --- a/tests/php/ORM/ManyManyThroughListTest.php +++ b/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); + } } diff --git a/tests/php/ORM/ManyManyThroughListTest.yml b/tests/php/ORM/ManyManyThroughListTest.yml index 88750a29d..c7c3c3c2a 100644 --- a/tests/php/ORM/ManyManyThroughListTest.yml +++ b/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 \ No newline at end of file diff --git a/tests/php/ORM/ManyManyThroughListTest/FallbackLocale.php b/tests/php/ORM/ManyManyThroughListTest/FallbackLocale.php new file mode 100644 index 000000000..cb865028f --- /dev/null +++ b/tests/php/ORM/ManyManyThroughListTest/FallbackLocale.php @@ -0,0 +1,22 @@ + 'Int', + ]; + + private static $has_one = [ + 'Parent' => Locale::class, + 'Locale' => Locale::class, + ]; + + private static $table_name = 'ManyManyThroughTest_FallbackLocale'; + + private static $default_sort = 'Sort'; +} diff --git a/tests/php/ORM/ManyManyThroughListTest/Locale.php b/tests/php/ORM/ManyManyThroughListTest/Locale.php new file mode 100644 index 000000000..37bff23d9 --- /dev/null +++ b/tests/php/ORM/ManyManyThroughListTest/Locale.php @@ -0,0 +1,36 @@ + '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'; +}