FIX Sort without specifying a table name

Using a table name in sort() is not allowed in CMS 5. We could use
orderBy() here but member is the table it will sort on by default anyway
so there's no need.

Also added unit tests, which should have caught this ages ago.

src/ORM/DataList.php

@@ -323,6 +323,7 @@ public function distinct($value)
      *
      * @param string|array $args
      * @example $list = $list->sort('Name'); // default ASC sorting
+     * @example $list = $list->sort('"Name"'); // field names can have double quotes around them
      * @example $list = $list->sort('Name ASC, Age DESC');
      * @example $list = $list->sort('Name', 'ASC');
      * @example $list = $list->sort(['Name' => 'ASC', 'Age' => 'DESC']);

src/Security/Member.php

@@ -1188,15 +1188,15 @@ public static function map_in_groups($groups = null)
      *
      * If no groups are passed, all groups with CMS permissions will be used.
      *
-     * @param array $groups Groups to consider or NULL to use all groups with
+     * @param SS_List|array|null $groups Groups to consider or NULL to use all groups with
      *                      CMS permissions.
      * @return Map Returns a map of all members in the groups given that
      *                have CMS permissions.
      */
-    public static function mapInCMSGroups($groups = null)
+    public static function mapInCMSGroups(SS_List|array|null $groups = null): Map
     {
         // non-countable $groups will issue a warning when using count() in PHP 7.2+
-        if (!$groups) {
+        if ($groups === null) {
             $groups = [];
         }
 
@@ -1205,7 +1205,7 @@ public static function mapInCMSGroups($groups = null)
             return ArrayList::create()->map();
         }
 
-        if (count($groups ?? []) == 0) {
+        if (count($groups) === 0) {
             $perms = ['ADMIN', 'CMS_ACCESS_AssetAdmin'];
 
             if (class_exists(CMSMain::class)) {
@@ -1246,7 +1246,7 @@ public static function mapInCMSGroups($groups = null)
             ]);
         }
 
-        return $members->sort('"Member"."Surname", "Member"."FirstName"')->map();
+        return $members->sort('"Surname", "FirstName"')->map();
     }
 
 

tests/php/Security/MemberTest.php

@@ -2,6 +2,8 @@
 
 namespace SilverStripe\Security\Tests;
 
+use InvalidArgumentException;
+use SilverStripe\Admin\LeftAndMain;
 use SilverStripe\Control\Cookie;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Convert;
@@ -12,6 +14,7 @@
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\FieldType\DBDatetime;
+use SilverStripe\ORM\Map;
 use SilverStripe\ORM\ValidationException;
 use SilverStripe\ORM\ValidationResult;
 use SilverStripe\Security\Group;
@@ -1618,4 +1621,121 @@ public function testChangePasswordToBlankIsValidated()
         $result = $member->changePassword('');
         $this->assertFalse($result->isValid());
     }
+
+    public function testMapInCMSGroupsNoLeftAndMain()
+    {
+        if (class_exists(LeftAndMain::class)) {
+            $this->markTestSkipped('LeftAndMain must not exist for this test.');
+        }
+        $result = Member::mapInCMSGroups();
+        $this->assertInstanceOf(Map::class, $result);
+
+        $this->assertEmpty($result, 'Without LeftAndMain, no groups are CMS groups.');
+    }
+
+    /**
+     * @dataProvider provideMapInCMSGroups
+     */
+    public function testMapInCMSGroups(array $groupFixtures, array $groupCodes, array $expectedUsers)
+    {
+        if (!empty($groupFixtures) && !empty($groupCodes)) {
+            throw new InvalidArgumentException('Data provider is misconfigured for this test.');
+        }
+
+        if (!class_exists(LeftAndMain::class)) {
+            $this->markTestSkipped('LeftAndMain must exist for this test.');
+        }
+
+        $groups = [];
+
+        // Convert fixture names to IDs
+        if (!empty($groupFixtures)) {
+            foreach ($groupFixtures as $index => $groupFixtureName) {
+                $groups[$index] = $this->objFromFixture(Group::class, $groupFixtureName)->ID;
+            }
+        }
+
+        // Convert codes to DataList
+        if (!empty($groupCodes)) {
+            $groups = Group::get()->filter(['Code' => $groupCodes]);
+        }
+
+        // Convert user fixtures to IDs
+        foreach ($expectedUsers as $index => $userFixtureName) {
+            // This is not an actual fixture - it was created by $this->logInWithPermission('ADMIN')
+            if ($userFixtureName === 'ADMIN User') {
+                $expectedUsers[$index] = Member::get()->filter(['Email' => '[email protected]'])->first()->ID;
+            } else {
+                $expectedUsers[$index] = $this->objFromFixture(Member::class, $userFixtureName)->ID;
+            }
+        }
+
+        $result = Member::mapInCMSGroups($groups);
+        $this->assertInstanceOf(Map::class, $result);
+
+        $this->assertSame($expectedUsers, $result->keys());
+    }
+
+    public function provideMapInCMSGroups()
+    {
+        // Note: "ADMIN User" is not from the fixtures, that user is created by $this->logInWithPermission('ADMIN')
+        return [
+            'defaults' => [
+                'groupFixtures' => [],
+                'groupCodes' => [],
+                'expectedUsers' => [
+                    'admin',
+                    'other-admin',
+                    'ADMIN User',
+                ],
+            ],
+            'single group in a list' => [
+                'groupFixtures' => [],
+                'groupCodes' => [
+                    'staffgroup'
+                ],
+                'expectedUsers' => [
+                    'staffmember',
+                ],
+            ],
+            'single group in IDs array' => [
+                'groups' => [
+                    'staffgroup',
+                ],
+                'groupCodes' => [],
+                'expectedUsers' => [
+                    'staffmember',
+                ],
+            ],
+            'multiple groups in a list' => [
+                'groupFixtures' => [],
+                'groupCodes' => [
+                    'staffgroup',
+                    'securityadminsgroup',
+                ],
+                'expectedUsers' => [
+                    'staffmember',
+                    'test',
+                ],
+            ],
+            'multiple groups in IDs array' => [
+                'groupFixtures' => [
+                    'staffgroup',
+                    'securityadminsgroup',
+                ],
+                'groupCodes' => [],
+                'expectedUsers' => [
+                    'staffmember',
+                    'test',
+                ],
+            ],
+            'group with no members' => [
+                'groupFixtures' => [],
+                'groupCodes' => [
+                    'memberless',
+                ],
+                'expectedUsers' => [],
+            ],
+        ];
+    }
 }

tests/php/Security/MemberTest.yml

@@ -58,6 +58,8 @@
     Email: [email protected]
     Password: 1nitialPassword
   staffmember:
+    FirstName: Staff
+    Surname: User
     Email: [email protected]
     Groups: '=>SilverStripe\Security\Group.staffgroup'
   managementmember: