Skip to content

Improve compatibility with general eloquent usage and Laravel Nova #66

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

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

tkuijer
Copy link

@tkuijer tkuijer commented Apr 18, 2024

This PR automatically corrects the sort order when using the orderBy query builder function. This is needed for compatibility with thelatest and oldest methods, which by default use 'asc' or 'desc' for their order.

This also adds a getCountForPagination method override to return the correct FM count.

Both improve compatibility with Laravel Nova.

@tkuijer tkuijer changed the title Correctly set the sort order when adding an orderBy to a query Improve compatibility with general eloquent usage and Laravel Nova Apr 18, 2024
@Smef Smef merged commit 66f8d64 into gearbox-solutions:2.x Apr 18, 2024
@Smef
Copy link
Member

Smef commented Apr 18, 2024

Is the pagination functionality used somewhere? In the regular builder it's only used by the paginate functions, but we have our own versions of those. I'm not sure this paginate functionality is going to work correctly as-is anyway.

@tkuijer
Copy link
Author

tkuijer commented Apr 18, 2024

Thanks for the merge! Love the package so far!

I'm not sure about consequences for the regular pagination, however Laravel Nova encountered errors with its pagination. Appears they use some more obscure eloquent features.

@Smef
Copy link
Member

Smef commented Apr 18, 2024

Ah. I haven't tried it with Nova. Does this get you the right value in there? It seems like it would need to hit the database to get that, and this isn't doing much at all.

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