laravel-mongodb icon indicating copy to clipboard operation
laravel-mongodb copied to clipboard

Fix/stringable sort

Open apeisa opened this issue 3 years ago • 4 comments

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.

apeisa avatar Jul 16 '22 09:07 apeisa

You can also see all tests passing from my own workflows here https://github.com/apeisa/laravel-mongodb/actions/runs/2681478769

apeisa avatar Jul 16 '22 10:07 apeisa

@divine Is it possible to merge this?

apeisa avatar Jul 23 '22 06:07 apeisa

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.

apeisa avatar Jul 25 '22 11:07 apeisa

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 data Powered by Codecov. Last update ad4422a...f670c5f. Read the comment docs.

codecov-commenter avatar Jul 26 '22 05:07 codecov-commenter

@divine @Smolevich any hope to get this merged for the next release?

apeisa avatar Sep 01 '22 07:09 apeisa

Thanks!

apeisa avatar Sep 01 '22 15:09 apeisa