fix: ensure Filter receives current HTTP request when resolved from DI container

[Thavarshan]

Summary

Fixes #30 - Request parameters not included in filter object pulled from DI container in controller

The Problem

When using dependency injection in Laravel controllers like:

public function index(PostFilter $postFilter)
{
    $posts = Post::with(['media'])->filter($postFilter)->get();
}

The filter methods were not being called because the Request object passed to PostFilter when created by the DI container was empty.

Manual instantiation worked correctly:

public function index()
{
    $postFilter = new PostFilter(request());
    $posts = Post::with(['media'])->filter($postFilter)->get();
}

The Fix

Added a binding in FilterableServiceProvider that ensures Request::class resolves to the current HTTP request ($app['request']) rather than creating a new empty Request instance:

protected function registerFilterBindings(): void
{
    $this->app->bindIf(Request::class, function ($app) {
        return $app['request'];
    });
}

Tests Added

Created comprehensive test suite tests/Integration/FilterDependencyInjectionTest.php with 12 tests covering:

• Filter resolution from DI container receives current request parameters
• Filters apply correctly when resolved via DI
• Multiple filter parameters handling
• Controller-style method injection simulation
• Empty request handling
• Query string and POST parameters
• JSON request body handling
• Filters with additional dependencies (cache/logger)
• Multiple filter resolutions with request changes
• Regression tests comparing DI vs manual instantiation behavior

Verification

• All 171 tests pass
• Linting passes
• Compatible with PHP 8.4

[Copilot]

Pull request overview

This PR fixes a critical dependency injection issue where Filter classes resolved from Laravel's DI container (e.g., in controller method injection) were receiving an empty Request instance instead of the current HTTP request. The fix adds a binding in FilterableServiceProvider that ensures Request::class resolves to the current request ($app['request']), enabling filters to properly access request parameters when using DI.

Key Changes:

• Adds registerFilterBindings() method in FilterableServiceProvider that binds Request::class to the current request
• Introduces comprehensive integration test suite with 12 tests covering various DI resolution scenarios
• Regression tests verify DI-resolved filters behave identically to manually instantiated filters

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
src/Filterable/Providers/FilterableServiceProvider.php	Adds packageRegistered() hook and registerFilterBindings() method using bindIf() to bind Request::class to current request
tests/Integration/FilterDependencyInjectionTest.php	New comprehensive test suite with 12 tests covering DI resolution scenarios, multiple request types (GET/POST/JSON), multiple parameters, empty requests, and regression tests comparing DI vs manual instantiation

[Copilot]

[nitpick] This test verifies that dependencies are injected but doesn't verify that the filter actually works correctly when applied to a query. While the test confirms getFilterables() returns the expected parameters and that cache/logger are present, it doesn't apply the filter to ensure the query filtering logic works with the injected dependencies. Consider adding an assertion like:

// Verify the filter actually works when applied
$results = MockFilterable::query()->filter($filter)->get();
$this->assertCount(2, $results); // John Doe and Bob Johnson contain 'John'
$this->assertTrue($results->every(fn ($r) => str_contains($r->name, 'John')));

[Copilot]

The constructor in FilterWithDependencies is redundant as it simply passes all parameters to the parent Filter constructor without modification. According to the custom coding guidelines, the abstract Filter class already accepts Request, optional Cache, and optional LoggerInterface in its constructor. This redundant constructor can be removed entirely - the parent constructor will be called automatically with the same signature.

[Copilot]

This test uses inconsistent approaches for setting request parameters: $this->app['request']->merge() for the first request (line 227) but $this->app->instance() for the second request (lines 232-233). This inconsistency could make the test behavior unclear. For consistency and predictability, use the same approach for both - prefer using $this->app->instance() for both cases as it's more explicit about replacing the entire request instance.

[Copilot]

The docblock says "Register contextual bindings for Filter classes" but the implementation uses a global binding with bindIf(), not a contextual binding. Contextual bindings in Laravel use $this->app->when(SomeClass::class)->needs(...), while this is a global binding. Consider updating the docblock to:

/**
 * Register global bindings for Filter dependencies.
 *
 * This ensures that when a Filter subclass is resolved from the DI container,
 * it receives the current HTTP request instance rather than an empty Request.
 */

[Thavarshan]

@copilot open a new pull request to apply changes based on the comments in this thread

[Copilot]

@Thavarshan I've opened a new pull request, #32, to work on those changes. Once the pull request is ready, I'll request review from you.