Utilize PropertyAccess component for sorting arrays #159

ostrolucky · 2017-05-29T16:01:43Z

The only data structure currenty ArraySubscriber is able to sort is object[], where object MUST have public get{sortPropertyName} method. This PR utilizes symfony propertyaccess component instead, which allows to sort pure array, nested structure, objects with issers, __call methods, objects without any getter but with pulic properties, etc.

Besides this main feature of this PR I changed some things, like I uncommented whitelist, turned strtolower into mb_strtolower for utf support, introduced ability to change built in sorting function with custom one and allowed to sort by other data types than strings, such as dates

akerouanton

Aside from my little comments, good job 👍

akerouanton · 2017-06-01T21:27:05Z

src/Knp/Component/Pager/Event/Subscriber/Sortable/ArraySubscriber.php

    {
-        if (is_array($event->target) && count($event->target) > 1)


Refactoring and enhancing in the same PR makes the diff roughly hard to read. Please next time create two separate PRs, it will make our life's easier.

What do you mean? Like first PR purely to refactor and second PR with actual feature pointed into first branch/PR? Sure can do so if you prefer that, even though I prefer having it in all in single PR to prevent conflicts (if there are some requested changes in first PR, it's likely second PR will have to be changed as well). In this case reading the full source is easier than diff though https://github.com/ostrolucky/knp-components/blob/db2daf3016612890b7a296dd5d44ae2bfe88856c/src/Knp/Component/Pager/Event/Subscriber/Sortable/ArraySubscriber.php

Yep that's it. From my point of view it's better to not restructure the code and in the mean time change the contact(s). But don't worry, I'm not asking you to recreate two separate PRs :)

akerouanton · 2017-06-01T21:35:09Z

src/Knp/Component/Pager/Event/Subscriber/Sortable/ArraySubscriber.php

-        $fieldValue2 = strtolower($object2->{$this->currentSortingFieldGetter}());
-        if ($fieldValue1 == $fieldValue2) {
-            return 0;
+        if (!is_array($event->target) || !isset($_GET[$event->options['sortFieldParameterName']])) {


We don't need to sort if there're no items in target.

Usort doesn't call the callback if array is empty. But my style is to purposely leave out such things from top of the function if rest of the body is cheap to execute, because if user misconfigured something else, he will not know about it until he passes non-empty array here E.g. if he enables sorting by a field which is missing from (enabled) whitelist, this subscriber will appear to be working correctly until some data appear there.

Seems reasonable 👍

akerouanton · 2017-06-01T21:44:20Z

src/Knp/Component/Pager/Event/Subscriber/Sortable/ArraySubscriber.php

+        }
+
+        $this->sortDirection = isset($_GET[$event->options['sortDirectionParameterName']]) && strtolower($_GET[$event->options['sortDirectionParameterName']]) === 'asc' ? 'asc' : 'desc';
+        $this->currentSortingField = $_GET[$event->options['sortFieldParameterName']];


What happens if this is empty?

Do you mean empty string, or when that property is actually missing in $_GET? If first, PropertyAccessor will throw error. Second case is handled by isset in top of the method.

edit: i've replaced isset with empty to handle empty value

ostrolucky · 2017-06-01T23:17:37Z

I've squashed the commits (mainly to fix CI status)

akerouanton · 2017-06-01T23:52:12Z

src/Knp/Component/Pager/Event/Subscriber/Sortable/ArraySubscriber.php

+        }
+
+        if (isset($_GET[$event->options['sortDirectionParameterName']])) {
+            $this->sortDirection = strtolower($_GET[$event->options['sortDirectionParameterName']]);


I think the ternary was useful, I would prefer to keep it so we're sure either asc or desc are used. Also, there's no default value anymore.

I've changed it because end result is same. Sort function works with "asc" only, anything else (even null) is sorted as if it was "desc". But since you prefer to cast it to "desc" before, I'll change it back.

That's true but only when sortFunction isn't provided, that's why I requested this change at first. But I'm realizing that, sortDirection and currentSortingField are not passed to sortFunction as arguments. I think it would be better to have them, so we don't need to retrieve them again when we implement a custom sort function.

Do you have an idea how to do that though? Usort passes to callback only those two arguments.

Yep, I've got one: create a private proxy method in the subscriber and pass both usort arguments and ours to the custom sort function.

akerouanton · 2017-06-11T22:02:10Z

@ostrolucky It's ready to be merged, right?

ostrolucky · 2017-06-11T22:12:54Z

@NiR- indeed it is! I don't know about your BC policies though. This will require to slightly change user code for sorting arrays. Before, code required to specify sorting field prefixed with a dot, e.g. .name, now it's just name

akerouanton · 2017-06-11T22:49:28Z

We try to follow semver at KnpLabs, thus AFAIK this repo too. Rather than introducing a BC change, could you automatically remove heading dot when detected please?

String comparison when sorting is now utf8 case insensitive Whitelist feature has been enabled for ArraySubscriber Allows to define custom sorting function Default sorting function works also with objects

ostrolucky · 2017-06-12T21:26:33Z

bc break no longer present, ready to merge!

akerouanton · 2017-06-13T11:30:47Z

Thank you!

akerouanton reviewed Jun 1, 2017

View reviewed changes

akerouanton mentioned this pull request Jun 11, 2017

DateTime sortable KnpLabs/KnpPaginatorBundle#409

Closed

Utilize PropertyAccess component for sorting arrays

86e54d9

String comparison when sorting is now utf8 case insensitive Whitelist feature has been enabled for ArraySubscriber Allows to define custom sorting function Default sorting function works also with objects

akerouanton merged commit 35bf97b into KnpLabs:master Jun 13, 2017

okapon mentioned this pull request Mar 19, 2018

Fix sorting bug when no existing class property is specified in query string parameter #193

Closed

okapon mentioned this pull request Jun 9, 2019

Fix sort error when class property does not exist #215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utilize PropertyAccess component for sorting arrays #159

Utilize PropertyAccess component for sorting arrays #159

ostrolucky commented May 29, 2017 •

edited

Loading

akerouanton left a comment

akerouanton Jun 1, 2017

ostrolucky Jun 1, 2017 •

edited

Loading

akerouanton Jun 1, 2017

akerouanton Jun 1, 2017 •

edited

Loading

ostrolucky Jun 1, 2017

akerouanton Jun 1, 2017

akerouanton Jun 1, 2017

ostrolucky Jun 1, 2017 •

edited

Loading

ostrolucky commented Jun 1, 2017

akerouanton Jun 1, 2017

ostrolucky Jun 2, 2017

akerouanton Jun 2, 2017

ostrolucky Jun 2, 2017

akerouanton Jun 2, 2017

akerouanton commented Jun 11, 2017

ostrolucky commented Jun 11, 2017

akerouanton commented Jun 11, 2017 •

edited

Loading

ostrolucky commented Jun 12, 2017

akerouanton commented Jun 13, 2017

Utilize PropertyAccess component for sorting arrays #159

Utilize PropertyAccess component for sorting arrays #159

Conversation

ostrolucky commented May 29, 2017 • edited Loading

akerouanton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostrolucky Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akerouanton Jun 1, 2017 • 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

ostrolucky Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

ostrolucky commented Jun 1, 2017

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

akerouanton commented Jun 11, 2017

ostrolucky commented Jun 11, 2017

akerouanton commented Jun 11, 2017 • edited Loading

ostrolucky commented Jun 12, 2017

akerouanton commented Jun 13, 2017

ostrolucky commented May 29, 2017 •

edited

Loading

ostrolucky Jun 1, 2017 •

edited

Loading

akerouanton Jun 1, 2017 •

edited

Loading

ostrolucky Jun 1, 2017 •

edited

Loading

akerouanton commented Jun 11, 2017 •

edited

Loading