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] Fix cursor paginate with union and column alias #50882

Conversation

thijsvdanker
Copy link
Contributor

When using cursorPaginate on a query with a union the where clause on the union uses the wrong column name.

Table Post:

id
start_time

Table News:

id
created_at
Post::toBase()
    ->select('id', 'start_time as created_at')
    ->union(News::toBase()->select('id', 'created_at'))
    ->orderBy('created_at')
    ->cursorPaginate(10, ['created_at']);

Should sort Post by the start_time column and News by the created_at column.

The current implementation sorts both Post and News by the start_time column.

This is similar to #50809 but with a dedicated test to confirm the issue.

@thijsvdanker
Copy link
Contributor Author

Is there anything I can do to move this forward?

@driesvints
Copy link
Member

@thijsvdanker not really. Just await a review by @taylorotwell.

@crynobone
Copy link
Member

@thijsvdanker Do we still need this if #50884 PR get merged?

@thijsvdanker
Copy link
Contributor Author

@crynobone Yes we do.

I've just ran the tests in this PR against the code in #50884 and they still fail for the same reason.

@crynobone
Copy link
Member

@thijsvdanker I feel like we should prioritize integration tests for this instead of just mocking everything.

@thijsvdanker
Copy link
Contributor Author

@crynobone I agree.
I have a busy week but I'll see if I can make an integration test later this week.

@thijsvdanker
Copy link
Contributor Author

@crynobone I've added an integration test.

@taylorotwell
Copy link
Member

taylorotwell commented Apr 16, 2024

@thijsvdanker would you remind resolving the conflicts? Does the integration test you added fail without your code changes?

@thijsvdanker thijsvdanker force-pushed the fix-cursor-paginate-with-union-and-column-alias-10-x branch from 5593882 to abdb064 Compare April 16, 2024 19:13
thijsvdanker added a commit to thijsvdanker/laravel-framework that referenced this pull request Apr 16, 2024
@thijsvdanker
Copy link
Contributor Author

@taylorotwell I've resolved the conflicts.
This PR shows both the integration test and the mocked test fails without my changes: #51088

@taylorotwell taylorotwell merged commit 877ebca 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.

None yet

4 participants