aarondfrancis / fast-paginate

A fast implementation of offset/limit pagination for Laravel.
MIT License
1.21k stars 56 forks source link

Filter out raw orders #35

Closed rodrigopedra closed 1 year ago

rodrigopedra commented 2 years ago

When a developer adds a raw order by expression using Builder@orderByRaw, the record created in the query builder's $orders property does not contain a field called column.

Therefore, when plucking the column attribute from the $orders properties and mapping to Grammar@wrap a null value is passed to the raw order by statements.

Inside Grammar@wrap it calls stripos() which raises a deprecation warning as it expects its first parameter to be a string and not a null value.

This PR:

rodrigopedra commented 1 year ago

Ping @aarondfrancis

Sorry to tag you, I am trying to review all deprecations on some of my projects, and this is the missing one.

If you didn't like this solution, please guide me on a preferable approach, and I will gladly send a new PR.

Thanks very much, in advance =)

atapatel commented 1 year ago

@rodrigopedra i am also facing the same issue for custom raw order.

atapatel commented 1 year ago

@rodrigopedra, if we filter out that orderByRaw, then it will give us totally wrong result. We have to think about it differently here.

aarondfrancis commented 1 year ago

Hey y'all, I added a test for orderingByRaw and everything seems to work as is. Am I missing something?

https://github.com/hammerstonedev/fast-paginate/commit/1c305ed106eab1dfd6b90781dac15052cf1ccfaf

Thanks for helping me out with this!

atapatel commented 1 year ago

@aarondfrancis perfect 👍

aarondfrancis commented 1 year ago

@atapatel To be clear, I put the test on main and nothing failed. So failures are you and @rodrigopedra seeing?

atapatel commented 1 year ago

@aarondfrancis can you please explain me more about it?

aarondfrancis commented 1 year ago

I added a test to main expecting it to fail, but it did not. So I don't know what your PR is even trying to fix!

aarondfrancis commented 1 year ago

Is there a better / different test that can show the issue you're facing?

atapatel commented 1 year ago

When i try to run this code locally it shows deprecated strict errors for stripos which used via grammer->wrap function.

aarondfrancis commented 1 year ago

What PHP version, what Laravel version

aarondfrancis commented 1 year ago

Ok I've finally figured it out. Laravel was consuming the deprecation notice. Should be all fixed up now, thanks for your help!

rodrigopedra commented 1 year ago

Thanks, @aarondfrancis, sorry for not seeing your messages yesterday. The issue was, precisely that, nulls were passed to a function expecting strings.