Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX Sort without specifying a table name #10675

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ORM/DataList.php
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand Down
10 changes: 5 additions & 5 deletions src/Security/Member.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may as well typehint this - the PHPDoc was wrong before anyway.
I checked and in kitchen sink there's no uses of this method passing in arguments anyway so nothing else for us to change.

{
// non-countable $groups will issue a warning when using count() in PHP 7.2+
if (!$groups) {
if ($groups === null) {
Copy link
Member Author

@GuySartorelli GuySartorelli Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the param is typehinted we can be more specific, which will reduce potential errors and just makes it a little clearer what we intend.

$groups = [];
}

Expand All @@ -1205,7 +1205,7 @@ public static function mapInCMSGroups($groups = null)
return ArrayList::create()->map();
}

if (count($groups ?? []) == 0) {
if (count($groups) === 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this behaviour alone - just tidying up a little ($groups is never not countable here, and === is more correct)

$perms = ['ADMIN', 'CMS_ACCESS_AssetAdmin'];

if (class_exists(CMSMain::class)) {
Expand Down Expand Up @@ -1246,7 +1246,7 @@ public static function mapInCMSGroups($groups = null)
]);
}

return $members->sort('"Member"."Surname", "Member"."FirstName"')->map();
return $members->sort('"Surname", "FirstName"')->map();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is, ultimately, the fix this PR is for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a legit non raw sql sort string? It doesn't match any of the examples in https://github.com/silverstripe/silverstripe-framework/blob/5/src/ORM/DataList.php#L324. None of the examples use double quotes.

If it is legit, then add it to the list of examples in DataList. Otherwise changes this to match one of the other examples

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, so I don't see how it wouldn't be "legit". If it shouldn't work it should explicitly not work.
The validateSortColumn method does explicitly ignore double quotes. I'll add it to the examples I guess but I don't think we need to provide an exhaustive list there.

I'm not for example going to duplicate EVERY SINGLE EXAMPLE there so it shows with and without double quotes. I'll just dupe the first one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were double quotes more a Postgres thing? If so, we could drop them here and not change the examples. But since this is already merged.. nvm.

Copy link
Member Author

@GuySartorelli GuySartorelli Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the argument max made when we decided to drop official support for postgres was that we should still add double quotes so that a community-maintained postgres module would still work.

I don't think that matters for this sort method (I suspect the lower levels of the ORM add quotes to it where needed)... but if it works now it's easier to let it keep working than to willfully break it.

}


Expand Down
120 changes: 120 additions & 0 deletions tests/php/Security/MemberTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -1618,4 +1621,121 @@ public function testChangePasswordToBlankIsValidated()
$result = $member->changePassword('');
$this->assertFalse($result->isValid());
}

public function testMapInCMSGroupsNoLeftAndMain()
{
if (class_exists(LeftAndMain::class)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LeftAndMain will always exist in practice, as silverstripe/admin is installed as part of silverstripe/installer in CI

I guess this is still a required test due to the logic in Member::mapInCMSGroups() .. though :/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it won't run the way our CI is currently set up - there are likely other tests like that too. I'd like to keep it and if we make upgrades to our CI in the future to let framework run without installer then it'll be run.

$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' => [],
],
];
}
}
2 changes: 2 additions & 0 deletions tests/php/Security/MemberTest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@
Email: [email protected]
Password: 1nitialPassword
staffmember:
FirstName: Staff
Surname: User
Comment on lines +61 to +62
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed that this doesn't affect any other tests - but it is necessary for the new one, or the value in the map will just be whitespace.

Email: [email protected]
Groups: '=>SilverStripe\Security\Group.staffgroup'
managementmember:
Expand Down