laravel-mongodb
laravel-mongodb copied to clipboard
Fix/stringable sort
This PR replaces an older one. This is a safe fix, where we type cast column value to string when ordering. This is more inline in how the Eloquent method that is overwritten works too.
There is a simple test to show how you can now pass objects that implement Stringable as a $column variable for orderBy method.
I wasn't sure where to add that mockable class, so I did put it directly into QueryTest.php file.
You can also see all tests passing from my own workflows here https://github.com/apeisa/laravel-mongodb/actions/runs/2681478769
@divine Is it possible to merge this?
Thanks for the suggestion, didn't know about the str() helper, makes the test much cleaner!
Not sure I understand your request:
Also, point me where exactly that library converts this to the stringable object.
If you are referring to the Filament library - I don't think it matters (this fix is not about Filament only). I mean the Eloquent orderBy method that we are overriding allows multiple objects (in addition to string). Most of them are Queryable-objects, but it also accepts Illuminate\Database\Query\Expression -objects, which are stringable ones.
We could use also $age = new \Illuminate\Database\Query\Expression('age'); where we now have $age = str('age'); to make that point more clear.
This fix simply makes it possible to give any stringable object for orderBy method, instead of only pure strings. Of course, we could check also if the object is Expression, and only then do type cast to a string, but I think code is cleaner and more future-proof this way.
Codecov Report
Merging #2420 (f670c5f) into master (ad4422a) will not change coverage. The diff coverage is
100.00%.
@@ Coverage Diff @@
## master #2420 +/- ##
=========================================
Coverage 87.62% 87.62%
Complexity 676 676
=========================================
Files 31 31
Lines 1551 1551
=========================================
Hits 1359 1359
Misses 192 192
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/Query/Builder.php | 89.64% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update ad4422a...f670c5f. Read the comment docs.
@divine @Smolevich any hope to get this merged for the next release?
Thanks!