8000 Typecast $column to string when ordering by apeisa · Pull Request #2419 · mongodb/laravel-mongodb · GitHub
[go: up one dir, main page]
Skip to content

Typecast $column to string when ordering #2419

8000
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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Typecast $column to string when ordering #2419

wants to merge 1 commit into from

Conversation

apeisa
Copy link
Contributor
@apeisa apeisa commented Jul 13, 2022

Since $column parameter in Illuminate/Database/Query/Builder.php can take other types than strings, it makes sense to typecast this into a string.

This came up when using Filament Admin panel with Mongo, where orderBy doesn't work since the $column passed to orderBy isn't a string, but a Stringable object. There are probably other places also with a similar need and I will provide more PR:s as I find them.

@divine
@apeisa
Copy link
Contributor Author
apeisa commented Jul 13, 2022

Hi

Thanks for your comment. That one test failing in Mongo v4.2 cannot be related to this PR. For me, it looks like some kind of race condition since it is comparing timestamps and there is a one-second difference. So if possible, can you re-run the tests to see them pass?

@codecov-commenter
Copy link
codecov-commenter commented Jul 14, 2022 •
edited
Loading

Codecov Report

Merging #2419 (b86a8b8) into master (ad4422a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #2419   +/-   ##
=========================================
  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...b86a8b8. Read the comment docs.

@apeisa
Copy link
Contributor Author
apeisa commented Jul 14, 2022

Hi @divine

I think the tests were re-run now and all passed.

@divine
Copy link
Contributor
divine commented Jul 14, 2022

Hello,

I've rerun tests but not sure how this could be the problem with that package as they state no support for this package filamentphp/filament#1365?

Can you add a test to see what values pass this package and do indeed solve the problem?

Thanks!

@apeisa
Copy link
Contributor Author
apeisa commented Jul 14, 2022

Hi

Thanks for the link. Filament works actually very well with Mongo, just few minor issues and this fixes the other one.

@apeisa
Copy link
Contributor Author
apeisa commented Jul 14, 2022

I will add tests for this.

@apeisa apeisa mentioned this pull request Jul 16, 2022
Merged
@apeisa
Copy link
Contributor Author
apeisa commented Jul 16, 2022

Hi @divine! I created another PR (with better naming), which has tests for this also. You can find it from #2420

@apeisa apeisa closed this Jul 16, 2022
@apeisa apeisa deleted the patch-1 branch July 16, 2022 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apeisa @divine @codecov-commenter
0