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

Now allowing defaultSort()'s argument to be a Closure. #9457

Merged
merged 4 commits into from
Nov 4, 2023

Conversation

telkins
Copy link
Contributor

@telkins telkins commented Nov 4, 2023

The Filament\Tables\Table\Concerns\CanSortRecords trait's defaultSort() method has the following signature:

public function defaultSort(string | Closure | null $column, string | Closure | null $direction = 'asc'): static

The $direction argument, if not null, is passed through Str::lower() and then assigned to $this->defaultSortDirection, which is a nullable string.

There is at least one problem: if a Closure is passed, then Str::lower() chokes on it.

image

I decided to do the following:

  • pass $direction through evaluate() right away and allow the rest of the code/types to remain as they are currently
  • remove the "last minute" call to evaluate() in getDefaultSortDirection()

All existing tests pass. No new tests were added.

@telkins
Copy link
Contributor Author

telkins commented Nov 4, 2023

@awcodes FYI....

There were at least two ways to do this, depending on when/where evaluate() should be called on whatever the user passed in. I opted to go one way first, keeping evaluate() to be called late....but then decided to go with the "cleaner" solution, which was to move evaluate() to the beginning of the process.

I have mixed feelings, as there may be advantages to having evaluate() called later vs earlier. 🤔

@danharrin danharrin added the enhancement New feature or request label Nov 4, 2023
@danharrin danharrin added this to the v3 milestone Nov 4, 2023
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

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

Can you please explain why you need to pass a function in?

@@ -24,6 +24,8 @@ public function defaultSort(string | Closure | null $column, string | Closure |
$this->defaultSortColumn = $column;
}

$direction = $this->evaluate($direction);
Copy link
Member

Choose a reason for hiding this comment

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

Evaluation should always happen in getters, it should not be evaluated here.

Copy link
Member

Choose a reason for hiding this comment

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

ive changed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Evaluation should always happen in getters, it should not be evaluated here.

I had done that first, as you may have seen. I noticed that this was normally in the getters. But, I changed my mind thinking you might prefer to keep the changes smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ive changed it

Thx. You did what I had done on my first iteration. Makes me feel good that I had the right solution...before I changed it. 😅

@danharrin danharrin merged commit 2debd2e into filamentphp:3.x Nov 4, 2023
4 checks passed
@telkins
Copy link
Contributor Author

telkins commented Nov 4, 2023

Can you please explain why you need to pass a function in?

Sure. I have a filter that filters from today forward or from today backward, depending.

I wasn't able to find a way to sort in the filter. The orderBy() doesn't seem to be respected. So, I found that I could pass a function in for defaultSort()'s $direction argument, determine the filter state, and then sort accordingly.

If there's a better way, I'd love to know about it.

In any case, this was a bug worth fixing, I thought. 🤓

@telkins telkins deleted the default-sort-fix branch November 4, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants