Skip to content

Conversation

duncanmcclean
Copy link
Member

@duncanmcclean duncanmcclean commented Aug 21, 2025

This pull request aims to fix an issue when ordering assets by integers, floats, dates, where the Eloquent Driver would consider the ints/floats/dates as strings, and order alphabetically, rather than numerically.

This PR takes a similar approach to #154 which fixed the same issue for entries, but I've refactored the logic to make it slightly tidier.

Before:

CleanShot 2025-08-21 at 14 44 37

After:

CleanShot 2025-08-21 at 14 44 56

Fixes #420.
Replaces #486.

@duncanmcclean duncanmcclean marked this pull request as ready for review August 21, 2025 15:59
Copy link
Contributor

@ryanmitchell ryanmitchell left a comment

Choose a reason for hiding this comment

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

This looks good, and is the right solution to fix the ordering issue. I think it should be merged to solve that immediate need.

However, thinking a bit further ahead to how we would solve issues like this one I wonder if we should be moving this logic to within function column() in the base eloquent query builder class.

take for example: https://github.com/statamic/cms/blob/d1e056b3be05d2df0bcbd92a818d11b08eff9e8c/src/Query/EloquentQueryBuilder.php#L185C32-L185C36

if this->column() returned the cast, we could do a ->whereRaw() based on it. Similarly for the other where clauses 🤔

It's required by the EloquentQueryBuilder in core.
@duncanmcclean
Copy link
Member Author

However, thinking a bit further ahead to how we would solve issues like #416 I wonder if we should be moving this logic to within function column() in the base eloquent query builder class.

take for example: https://github.com/statamic/cms/blob/d1e056b3be05d2df0bcbd92a818d11b08eff9e8c/src/Query/EloquentQueryBuilder.php#L185C32-L185C36

if this->column() returned the cast, we could do a ->whereRaw() based on it. Similarly for the other where clauses 🤔

Interesting!

We could do the same sorta thing in ->where() (to fix that other issue), but it wouldn't fix the casting issue everywhere.

I don't know if we want to change what $this->column() returns because there are cases where you might want the column name, but not the casted stuff.

Happy to chat about it offline 🙂

@duncanmcclean duncanmcclean merged commit 7c404f2 into 4.x Aug 22, 2025
38 checks passed
@duncanmcclean duncanmcclean deleted the fix/asset-query-builder-ordering branch August 22, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset sorting by size not functioning correctly
2 participants