Add default sort for multiple columns

[sebacastroh]

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests and did you add any new tests needed for your feature?
2. Did you update all templates (if applicable)?
3. Did you add the relevant documentation (if applicable)?
4. Did you test locally to make sure your feature works as intended?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully ran tests with your changes locally?

Introduction

Currently it is only possible to define a single default sort column. However, sometimes (specially when you use GROUP BY) you'd like to sort for more than one column. So, following the idea of the original setDefaultSort(string $field, string $direction = 'asc') function, I added the function setDefaultSortColumns(array $fields, array $directions = ['asc']).

This function allows you to define several columns for sorting adding the following lines in applySorting():

if ($this->hasDefaultSortColumns() && ! $this->hasSorts()) {
    $sortColumns    = $this->getDefaultSortColumns();
    $sortDirections = $this->getDefaultSortDirections();
    foreach ($sortColumns as $index => $sortColumn) {
        $this->setBuilder($this->getBuilder()->orderBy($sortColumn, $sortDirections[$index]));
    }

    return $this->getBuilder();
}

Some examples:

• Sort by three columns without setting the orders

public function configure(): void
{
    $this->setDefaultSortColumns(['column1', 'column2', 'column3']); // By default, all columns are in ascending order
}

• Sort by three columns with specific orders

public function configure(): void
{
    $this->setDefaultSortColumns(['column1', 'column2', 'column3'], ['desc', 'asc', 'asc']);
}

Considerations

• If both setDefaultSort and setDefaultSortColumns are set, only setDefaultSort is considered
• It could be possible to rewrite setDefaultSort to have the original behaviour if receiving a single string or this new behaviour if receives an array

[lrljoe]

Firstly, thanks for the contribution!

I've changed the base to develop as that's where all PRs go initially, and I will review properly, but a couple of quick comments from me!

Approach
Would it be a better approach to do it your suggested approach:

    $this->setDefaultSortColumns(['column1', 'column2', 'column3'], ['desc', 'asc', 'asc']);

or defining it against each column

    $this->setDefaultSortColumns([['column1', 'desc'], ['column2', 'asc'], ['column3', 'asc]]);

I'm more thinking if you're doing something like ordering by 5 or 6 columns, it may lead to some headaches doing it your original approach, although it also has benefits using your approach.

Tests
Can I ask you to write some appropriate tests for this too please?

[sebacastroh]

Hi, I'm happy to share this and I hope it helps to others, this library has helped me a lot in my projects.

About your questions

• I thought the same before doing the PR. I chose the first option because is easier if you don't want to specify the order for every column (as I'm doing for my project). If we use the second option, how do we call the function without specifying orders? I imagine something like this

$this->setDefaultSortColumns([['column1'], ['column2'], ['column3']]);

which I don't think is a good option to be honest

• I checked the test folder and I didn't find a test for the default sort, so for that reason I didn't write a test. I can think in some cases, but if you have some ideas I'm glad to hear them. The only thing I can say is that I'm using this feature and searching, filters and other options are working well together 😅

[lrljoe]

Looking at this, I agree with you that it's going to add a slight complexity if you don't specify the order, so I may make a slightly different recommendation perhaps

If you were to create a new function:
addDefaultSortColumn($col,$direction = 'asc')

Then use this to add the col/direction to your new arrays.

You could call that function several times in the configure() method, making it more visible as to the order of the different columns, which may be quite clean.

But also retain your existing method for more advanced users?

Clear Sorts
Glancing at the code again, I'd have to test, but it looks to me that if someone calls clear sorts, the defaults are going to be applied again?
Appreciate that this is the current position, but I'm questioning whether there should be a toggle for enabling/disabling the behaviour of restoring default sorts on clear (I may have overlooked this - and it may already exist in the codebase).
I'm debating whether there should be both "Clear All" and "Restore Defaults" options, with the appropriate behaviours, appreciate this may be beyond the scope of this PR though!
Just thinking in terms of usability, I'd probably be quite annoyed if I couldn't remove multiple default sorts easily as an end-user, so it may need to be included... (annoying, and I apologise!

Tests
In terms of the tests, if you can write the tests then that'd be really helpful and appreciated, I've been trying to improve the test coverage overall, and I've added a large chunk as I'm reviewing each feature, but haven't got around to looking at sorting at all yet.

I really do think this has a lot of value, and I don't want to discourage you btw!

[sebacastroh]

My first approach to this problem was calling setDefaultSort several times like we do in Laravel, i.e.

public function config()
{
    $this->setDefaultSort('name1')
        ->setDefaultSort('name2', 'desc');
}

So I changed the function as follows

public function setDefaultSort(string $field, string $direction = 'asc')
{
    if (!in_array($field, $this->defaultSortColumn)) {
        array_push($field, $this->defaultSortColumn);
        array_push($direction, $this->defaultSortDirection);
    }
}

I had to change the data types of these variables from string to array and replace the code related to the default sorting in applySorting.

I think this is the best option, then why did I choose the other one? Because as I'm already using this library in a project, I don't want to edit the local files and then have conflicts with the production environment. I'm using the PR's approach because I can use it in my controller without edit the library source codes. The second reason, however, is the in_array line. I don't want to use it to improve the performance, but it seems that every time you search, filter or whatever, config is called and then the resulting array is pushed. If we can avoid that, then I would suggest this option and I can share those codes

[sebacastroh]

Well, now I have a little time, so I will paste my original approach if you consider is a better option

WithSorting.php

I redefine two variables and the applySorting function

public ?array $defaultSortColumn = null;
public array $defaultSortDirection = ['asc'];

/**
 * @return Builder
 */
public function applySorting(): Builder
{
    if ($this->hasDefaultSort() && ! $this->hasSorts()) {
        $sortColumns    = $this->getDefaultSortColumn();
        $sortDirections = $this->getDefaultSortDirection();
        foreach ($sortColumns as $index => $sortColumn) {
            $this->setBuilder($this->getBuilder()->orderBy($sortColumn, $sortDirections[$index]));
        }

        return $this->getBuilder();
    }

    foreach ($this->getSorts() as $column => $direction) {
        if (! in_array($direction, ['asc', 'desc'])) {
            $direction = 'asc';
        }

        if (is_null($column = $this->getColumnBySelectName($column))) {
            continue;
        }

        if (! $column->isSortable()) {
            continue;
        }

        // TODO: Test
        if ($column->hasSortCallback()) {
            $this->setBuilder(call_user_func($column->getSortCallback(), $this->getBuilder(), $direction));
        } elseif ($column->isBaseColumn()) {
            $this->setBuilder($this->getBuilder()->orderBy($column->getColumnSelectName(), $direction));
        } else {
            $value = $this->getBuilder()->getGrammar()->wrap($column->getColumn() . ' as ' . $column->getColumnSelectName());
            $segments = preg_split('/\s+as\s+/i', $value);
            $this->setBuilder($this->getBuilder()->orderByRaw($segments[1] . ' ' . $direction));
        }
    }

    return $this->getBuilder();
}

SortingConfiguration.php

I redefine the functions setDefaultSort and removeDefaultSort

/**
 * @param  string  $field
 * @param  string  $direction
 *
 * @return self
 */
public function setDefaultSort(string $field, string $direction = 'asc'): self
{
    if (!$this->hasDefaultSort()) {
        $this->defaultSortColumn = [$field];
        $this->defaultSortDirection = [$direction];
    } else {
        if (!in_array($field, $this->defaultSortColumn)) {
            array_push($this->defaultSortColumn, $field);
            array_push($this->defaultSortDirection, $direction);
        }
    }

    return $this;
}

/**
 * @return self
 */
public function removeDefaultSort(): self
{
    $this->defaultSortColumn = null;
    $this->defaultSortDirection = ['asc'];

    return $this;
}

SortingHelpers.php

I redefine the output type of the getDefaultSortColumn and getDefaultSortDirection functions

/**
 * @return array|null
 */
public function getDefaultSortColumn(): ?array
{
    return $this->defaultSortColumn;
}

/**
 * @return array
 */
public function getDefaultSortDirection(): array
{
    return $this->defaultSortDirection;
}

Sorting multiple columns

public function configure(): void
{
    $this->setDefaultSort('birthday', 'desc')
        ->setDefaultSort('lastname')
        ->setDefaultSort('name');
}

This will sort in descending order by birthday, and then in ascending order by lastname and name.

[lrljoe]

Massively sleep deprived, but rather than checking against an array.
the order is either set to descending, otherwise it defaults to ascending, which is a more memory friendly approach.

E.g (this can be improved on, just a basic example)

foreach ($this->getSorts() as $column => $direction) {
       $direction = ($direction == 'desc') ? 'desc' : 'asc';

Thanks for putting the time in on this, it's massively appreciated and I will definitely go though it all this week and have a chat with Anthony about his preferred approach.

It's a great idea, so thank you, I'll put an update in a couple of days, but from my perspective, there's nothing stopping multiple methods to allow for different approaches, so don't worry your work won't go to waste, as it is likely that multiple approaches will be added in to give some choice, which may just require a tweak elsewhere.

[lrljoe]

Just to add, to redefine any outputs it'll need to cater for existing use.

So if it's not an array, array wrap it to avoid any breaking change.

I'll make some time later this week to review this properly as it's definitely a good addition

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lrljoe]

I'll review this after the next release, as part of a wider "sort out sorting" task

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[lrljoe]

Removing won't fix, will re-review

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[othyn]

Is this still planned to be added? Would be a very nice feature to have 😄

As a usage example, I currently have a table where a dual default sort of Priority and then Name would be excellent, but at the moment its just Priority.

[lrljoe]

Yep, still planned to be added, some tweaks needed still before it can make it in properly.

There were a couple of issues that needed addressing, so bits of the change were rolled back.

Give it two weeks and it'll be in there.

[othyn]

Amazing, thank you!