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

Update builder class pagination methods to resolve LengthAwarePaginator using container #413

Conversation

stevenmaguire
Copy link
Contributor

@stevenmaguire stevenmaguire commented Aug 19, 2020

This PR attempts to address to fix #371. The LengthAwarePaginator was being instantiated directly in the pagination methods. This solution intends to use the container to resolve that class instead. This will give other container-based configurations a chance to hook into the paginators emitted by Scout in the same way that they can hook into Paginators emitted by Eloquent.

The specific use case that leads me down this path was an observation that the following Laravel-specific code was not affecting LengthAwarePaginators from Scout.

        $app->resolving(LengthAwarePaginator::class, static function (LengthAwarePaginator $paginator) {
            return $paginator->appends(request()->query());
        });

        $app->resolving(Paginator::class, static function (Paginator $paginator) {
            return $paginator->appends(request()->query());
        });

The test coverage for this class was pretty sparse. No existing tests break. I am happy to write some more specific tests to exercise the container work if needed.

@taylorotwell taylorotwell merged commit 060bd8f into laravel:8.x Aug 19, 2020
@stevenmaguire stevenmaguire deleted the sm-add-support-for-container-resolved-paginator branch August 19, 2020 19:19
@stevenmaguire
Copy link
Contributor Author

Thanks for merging this - hopefully, it helps others out! Is it trivial to cut a new release?

@driesvints
Copy link
Member

@stevenmaguire release day is tomorrow

@stevenmaguire
Copy link
Contributor Author

Great! Thanks for the clarification.

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.

Creating the Paginator using the container
3 participants