Skip to content

Support limits #180

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Support limits #180

wants to merge 16 commits into from

Conversation

parkourben99
Copy link

@parkourben99 parkourben99 commented Nov 17, 2024

Fixes #173

Support sub query limits. Now that the Eloquent Eager Limit package is native in Larave 11l.

example usage:

User::with([
    'posts' => fn ($query) => $query->limit(3)
])->paginate();

@parkourben99 parkourben99 marked this pull request as ready for review November 25, 2024 12:34
@topclaudy
Copy link
Owner

Hi @parkourben99 Thank you for the PR! This package is designed to support Laravel versions prior to 11. However, the Laravel version constraint in your composer.json file would restrict compatibility with those versions.

@parkourben99
Copy link
Author

parkourben99 commented Jan 10, 2025

Hi @parkourben99 Thank you for the PR! This package is designed to support Laravel versions prior to 11. However, the Laravel version constraint in your composer.json file would restrict compatibility with those versions.

I'm not sure these changes will work with previous versions of laravel, due to the recent change in laravel to support limits.

Do the tests run on previous versions?

@mpyw
Copy link
Contributor

mpyw commented Mar 4, 2025

@parkourben99 @parallels999 Any updates? I'm going to abandon https://github.com/mpyw/compoships-eager-limit. I'm looking forward to this PR get merged!

@parkourben99
Copy link
Author

parkourben99 commented Mar 4, 2025

@parkourben99 @parallels999 Any updates? I'm going to abandon https://github.com/mpyw/compoships-eager-limit. I'm looking forward to this PR get merged!

Apologies, I have updated this PR.

@mpyw
Copy link
Contributor

mpyw commented Mar 5, 2025

@topclaudy @parallels999 We'll appreciate if you review this in your free time 😃

@parallels999
Copy link

I am not the maintainer of this package, I don't know why you ask me for a review, At most I avoid being left without support in my old apps

@parkourben99
Copy link
Author

I am not the maintainer of this package, I don't know why you ask me for a review, At most I avoid being left without support in my old apps

Hi @parallels999 I suspect you were tagged as you reviewed this to being with.

@chrispappas
Copy link

@parkourben99 I just submitted a PR into your PR 🤯 parkourben99#1

Your changes are not backwards-compatible with older PHP versions, GH Actions test matrix was failing, so I updated the code to use older switch and function closures. Tests are passing now on my fork of your fork.

Fix failing tests, BC with older versions of PHP
@parkourben99
Copy link
Author

@parkourben99 I just submitted a PR into your PR 🤯 parkourben99#1

Your changes are not backwards-compatible with older PHP versions, GH Actions test matrix was failing, so I updated the code to use older switch and function closures. Tests are passing now on my fork of your fork.

Thanks @chrispappas not sure why dead versions of PHP are still being supported

@chrispappas
Copy link

@parkourben99 the linting issue failing the CI build was introduced in my PR (sorry eh 🍁), can you fix please? I'd love to get this PR landed so I can switch back to using the main repo for compoships, and not my personal fork

@chrispappas
Copy link

@topclaudy just a follow-up ping on this PR for review, when you have time. I have been using my forked branch with this change in prod for the last few weeks and it works as expected.

@topclaudy
Copy link
Owner

@parkourben99 Can you please fix the failing tests?

@parkourben99
Copy link
Author

@parkourben99 Can you please fix the failing tests?

Looks like the failing test are due to backed enums. I have merged master into this PR.

@parallels999 I've also added support for mariadb

@topclaudy
Copy link
Owner

@parkourben99 Thanks for the update. The tests are still failing tough. Can you take a look when you have a moment?

@topclaudy
Copy link
Owner

@parkourben99 The tests are green on master 41e7979

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.

Integration with Eloquent Eager Limit
6 participants