Filament•13mo ago

Is there an error in `Filament\Table\Table::defaultSort()`...?🤔

I was trying to pass a Closure to defaultSort() for the second argument, $direction for a table that I have. Here is the latest version of defaultSort() in Filament\Tables\Table:

trait CanSortRecords
{
    protected ?string $defaultSortColumn = null;

    protected ?string $defaultSortDirection = null;

    protected ?Closure $defaultSortQuery = null;

    protected bool | Closure | null $persistsSortInSession = false;

    public function defaultSort(string | Closure | null $column, string | Closure | null $direction = 'asc'): static
    {
        if ($column instanceof Closure) {
            $this->defaultSortQuery = $column;
        } else {
            $this->defaultSortColumn = $column;
        }

        if ($direction !== null) {
            $direction = Str::lower($direction);
        }

        $this->defaultSortDirection = $direction;

        return $this;
    }

trait CanSortRecords
{
    protected ?string $defaultSortColumn = null;

    protected ?string $defaultSortDirection = null;

    protected ?Closure $defaultSortQuery = null;

    protected bool | Closure | null $persistsSortInSession = false;

    public function defaultSort(string | Closure | null $column, string | Closure | null $direction = 'asc'): static
    {
        if ($column instanceof Closure) {
            $this->defaultSortQuery = $column;
        } else {
            $this->defaultSortColumn = $column;
        }

        if ($direction !== null) {
            $direction = Str::lower($direction);
        }

        $this->defaultSortDirection = $direction;

        return $this;
    }

Seems like passing $direction to Str::lower() when $direction is a Closure causes an exception: mb_strtolower(): Argument #1 ($string) must be of type string, Closure given Am I doing this wrong, or is this an error...? Seems like this is an error to me.

16 Replies

TravisOP•13mo ago

If this is allowed to take a Closure, can anyone point me to an example that works...?

awcodes•13mo ago

A closure works, but I think it still has to return ‘asc’ or ‘desc’ to be applied to the query. Would help if you provided your default sort code.

TravisOP•13mo ago

Thx....but can Str::lower() process a Closure...? (I'll test shortly...) Here's my defaultSort() code:

            ->defaultSort(
                column: 'date',
                direction: function () {
                    $state = $this->table->getFilter('schedule')->getState();

                    if ($state['schedule'] === 'upcoming') {
                        return 'asc';
                    }

                    return 'desc';
                }
            )

            ->defaultSort(
                column: 'date',
                direction: function () {
                    $state = $this->table->getFilter('schedule')->getState();

                    if ($state['schedule'] === 'upcoming') {
                        return 'asc';
                    }

                    return 'desc';
                }
            )

awcodes•13mo ago

No, the string facade doesn’t accept closures in any of its methods, as far as I know.

TravisOP•13mo ago

So...there's an error in the filament code... It simply takes $direction and passes it to Str::lower()....

TravisOP•13mo ago

And, I just tested, anyway....

TravisOP•13mo ago

$direction can be a string or a Closure...or null....

awcodes•13mo ago

If you’re returning a string from the callback then it shouldn’t matter right.

TravisOP•13mo ago

Wrong...see my screenshot. I'm passing a simple Closure that returns a string...and get the same error. It's exactly what's happening in defaultSort().... I'm hoping I'm missing something....but I don't think I am. I think the Tinkerwell test that I ran (see screenshot) demonstrates this. Here's the defaultSort() code again:

    public function defaultSort(string | Closure | null $column, string | Closure | null $direction = 'asc'): static
    {
        if ($column instanceof Closure) {
            $this->defaultSortQuery = $column;
        } else {
            $this->defaultSortColumn = $column;
        }

        if ($direction !== null) {
            $direction = Str::lower($direction);
        }

        $this->defaultSortDirection = $direction;

        return $this;
    }

    public function defaultSort(string | Closure | null $column, string | Closure | null $direction = 'asc'): static
    {
        if ($column instanceof Closure) {
            $this->defaultSortQuery = $column;
        } else {
            $this->defaultSortColumn = $column;
        }

        if ($direction !== null) {
            $direction = Str::lower($direction);
        }

        $this->defaultSortDirection = $direction;

        return $this;
    }

awcodes•13mo ago

The question though, is why is your direction callback not passing a string of asc or desc, not that it isn’t a bug or you’re doing anything wrong.

TravisOP•13mo ago

It is...it's returning either 'asc' or 'desc'.... Or, do you mean, why isn't it type-hinting a string...? I'll try that, but I imagine it would result in the same thing...as demonstrated in the Tinkerwell test screenshot. Here's a reference to my code...with the callback returning one string or the other....

awcodes•13mo ago

This definitely looks like a bug. There’s nothing evaluating $direction if it’s a closure. And laravel’s Str::lower() doesn’t accept a closure. Please submit an issue on GitHub.

TravisOP•13mo ago

Thx for confirming my suspicions. 👍 I'll submit an issue...perhaps submitting a PR over the weekend, time permitting. 🤓

awcodes•13mo ago

Thank you.

TravisOP•13mo ago

PR: https://github.com/filamentphp/filament/pull/9457

GitHub

Now allowing defaultSort()'s argument to be a Closure. by telkins ...

The Filament\Tables\Table\Concerns\CanSortRecords trait's defaultSort() method has the following signature: public function defaultSort(string | Closure | null $column, string | Closure | null ...

TravisOP•13mo ago

Feedback welcome/appreciated... 🤓

Gaming

Programming

Is there an error in `Filament\Table\Table::defaultSort()`...?🤔