FIX Sort without specifying a table name #10675

GuySartorelli · 2023-01-31T23:30:19Z

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.

Parent issue

https://github.com/silverstripeltd/product-issues/issues/644#event-8403907821

GuySartorelli · 2023-01-31T23:34:49Z

src/Security/Member.php

     *                      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


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.

GuySartorelli · 2023-01-31T23:35:05Z

src/Security/Member.php

-        if (!$groups) {
+        if ($groups === null) {


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.

GuySartorelli · 2023-01-31T23:35:51Z

src/Security/Member.php

-        if (count($groups ?? []) == 0) {
+        if (count($groups) === 0) {


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

GuySartorelli · 2023-01-31T23:36:03Z

src/Security/Member.php

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


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

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

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.

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.

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.

GuySartorelli · 2023-01-31T23:36:46Z

tests/php/Security/MemberTest.yml

+    FirstName: Staff
+    Surname: User


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.

emteknetnz · 2023-01-31T23:56:08Z

tests/php/Security/MemberTest.php

+
+    public function testMapInCMSGroupsNoLeftAndMain()
+    {
+        if (class_exists(LeftAndMain::class)) {


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 :/

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.

emteknetnz · 2023-01-31T23:56:11Z

src/Security/Member.php

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


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

emteknetnz · 2023-01-31T23:57:12Z

tests/php/Security/MemberTest.php

+        $this->assertSame($expectedValue, $result->values());
+    }
+
+    public function mapInCMSGroupsProvider()


Suggested change

public function mapInCMSGroupsProvider()

public function provideMapInCMSGroups()

Current convention is to change the leading word 'test' to 'provide' so it's easy to match the test and the provider, they have the same casing

emteknetnz · 2023-01-31T23:57:28Z

tests/php/Security/MemberTest.php

+    }
+
+    /**
+     * @dataProvider mapInCMSGroupsProvider


Suggested change

* @dataProvider mapInCMSGroupsProvider

* @dataProvider provideMapInCMSGroups

GuySartorelli · 2023-02-01T00:20:03Z

The unit tests pass for me locally with kitchen sink installed.... it seems like there might be some extension which changes the Title value in kitchen sink. I'll test against the IDs of the members instead since all that matters is that they're the right members, not what the title of the member is.

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.

GuySartorelli mentioned this pull request Jan 31, 2023

DEP Upgrade frontend build stack symbiote/silverstripe-advancedworkflow#478

Merged

GuySartorelli force-pushed the pulls/5/fix-member-sort branch from 57c9ac2 to 21d7c91 Compare January 31, 2023 23:33

GuySartorelli commented Jan 31, 2023

View reviewed changes

emteknetnz requested changes Jan 31, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/fix-member-sort branch from 21d7c91 to 8df5141 Compare February 1, 2023 00:17

FIX Sort without specifying a table name

e3f8fc2

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.

GuySartorelli force-pushed the pulls/5/fix-member-sort branch from 8df5141 to e3f8fc2 Compare February 1, 2023 00:26

emteknetnz approved these changes Feb 1, 2023

View reviewed changes

emteknetnz merged commit 8260280 into silverstripe:5 Feb 1, 2023

emteknetnz deleted the pulls/5/fix-member-sort branch February 1, 2023 00:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Sort without specifying a table name #10675

FIX Sort without specifying a table name #10675

GuySartorelli commented Jan 31, 2023

GuySartorelli Jan 31, 2023

GuySartorelli Jan 31, 2023 •

edited

Loading

GuySartorelli Jan 31, 2023

GuySartorelli Jan 31, 2023

emteknetnz Jan 31, 2023

GuySartorelli Feb 1, 2023

michalkleiner Feb 1, 2023

GuySartorelli Feb 1, 2023 •

edited

Loading

GuySartorelli Jan 31, 2023

emteknetnz Jan 31, 2023

GuySartorelli Feb 1, 2023

emteknetnz Jan 31, 2023

emteknetnz Jan 31, 2023

GuySartorelli Feb 1, 2023

emteknetnz Jan 31, 2023

GuySartorelli Feb 1, 2023

GuySartorelli commented Feb 1, 2023

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

	public function mapInCMSGroupsProvider()
	public function provideMapInCMSGroups()

	* @dataProvider mapInCMSGroupsProvider
	* @dataProvider provideMapInCMSGroups

FIX Sort without specifying a table name #10675

FIX Sort without specifying a table name #10675

Conversation

GuySartorelli commented Jan 31, 2023

Parent issue

Choose a reason for hiding this comment

GuySartorelli Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Feb 1, 2023

GuySartorelli Jan 31, 2023 •

edited

Loading

GuySartorelli Feb 1, 2023 •

edited

Loading