Skip to content
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.

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

[10.x] Binding order is incorrect when using cursor paginate with multiple unions with a where #50884

Conversation

thijsvdanker
Copy link
Contributor

When using cursorPaginate on a query that has multiple unions with their own where clause, the binding order is incorrect.

This happens because:

https://github.com/laravel/framework/blob/11.x/src/Illuminate/Database/Concerns/BuildsQueries.php#L436
adds the bindings for the cursor where to the end of the bindings and not in the position that they should be.

The result is a query that binds the wrong values to the wrong parameters in the query.

This PR recreates the union bindings in the cursor paginate logic so that the cursor where binding is in the correct position.

This PR is similar to #50810 but does not change the structure of the union binding to address the fear of a breaking change.
Instead it is only applied in the context of cursor pagination.

@taylorotwell
Copy link
Member

I dunno - if the bindings were being duplicated this PR was already apparently broken? Why did the tests not show that? It shakes my confidence in merging 😅

@thijsvdanker
Copy link
Contributor Author

Yeah I agree that that doesn't look too solid.
I'll see if I can make test that covers that piece of code.

@taylorotwell taylorotwell marked this pull request as draft April 9, 2024 18:51
@thijsvdanker
Copy link
Contributor Author

I've given each builder its own variable name to make it a little bit easier to see what's going on.
b512794

@driesvints
Copy link
Member

@thijsvdanker please mark this as ready if you need another review.

@thijsvdanker
Copy link
Contributor Author

I've added an Eloquent test that demonstrates the issue: 3e87617

@thijsvdanker thijsvdanker marked this pull request as ready for review April 11, 2024 13:56
@crynobone
Copy link
Member

I've added an Eloquent test that demonstrates the issue: 3e87617

Can confirm the usage will be broken via #51076

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
src/Illuminate/Database/Query/Builder.php Outdated Show resolved Hide resolved
src/Illuminate/Database/Eloquent/Builder.php Outdated Show resolved Hide resolved
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
thijsvdanker and others added 3 commits April 16, 2024 13:37
Co-authored-by: Mior Muhammad Zaki <crynobone@gmail.com>
@taylorotwell taylorotwell merged commit 67d4d0d into laravel:10.x Apr 16, 2024
24 checks passed
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.

4 participants