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

Fixed mysql not supporting offset() without a limit() #4247

Closed

Conversation

Aethelflaed
Copy link
Contributor

While working on #4245, I realised I needed a limit() call before calling offset() for mysql, and thought that was a normal behaviour.

Then I saw in the 2.0.0 changelog that there should have been a workaround for that already.

It seems it wasn't entirely tested and working, so here are the missing pieces

@weiznich
Copy link
Member

weiznich commented Sep 9, 2024

Thanks for opening this PR

The PR that introduced this behavior was #2029. Back then we explicitly decided to not support offset without limit for mysql wherever possible as that "workaround" is really an ugly hack that should be made explicit by the calling side. Unfortunately we can only guarantee that for unboxed statements, not for boxed ones, therefore the "workaround" there. So no, this is no missing piece but an explicit design decision.

@Aethelflaed
Copy link
Contributor Author

The PR that introduced this behavior was #2029. Back then we explicitly decided to not support offset without limit for mysql wherever possible as that "workaround" is really an ugly hack that should be made explicit by the calling side. Unfortunately we can only guarantee that for unboxed statements, not for boxed ones, therefore the "workaround" there. So no, this is no missing piece but an explicit design decision.

I'll look if it's possible to add a warning

@Aethelflaed
Copy link
Contributor Author

It's still not possible to add a deprecated message on a trait implementation item so nothing changed since #2029.

cc @weiznich

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.

2 participants