Skip to content

Commit 5f913e6

Browse files
CopilotThavarshan
andcommitted
Address review comments: update docblock, add query assertion, use consistent request setup, remove redundant constructor
Co-authored-by: Thavarshan <10804999+Thavarshan@users.noreply.github.com>
1 parent c25ef88 commit 5f913e6

2 files changed

Lines changed: 14 additions & 15 deletions

File tree

src/Filterable/Providers/FilterableServiceProvider.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public function packageRegistered(): void
2828
}
2929

3030
/**
31-
* Register contextual bindings for Filter classes.
31+
* Register global bindings for Filter dependencies.
3232
*
3333
* This ensures that when a Filter subclass is resolved from the DI container,
3434
* it receives the current HTTP request instance rather than an empty Request.

tests/Integration/FilterDependencyInjectionTest.php

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -221,18 +221,25 @@ public function test_filter_injection_works_with_other_dependencies(): void
221221
// Cache and logger should be injected when explicitly bound
222222
$this->assertTrue($filter->hasCacheHandler());
223223
$this->assertTrue($filter->hasLogger());
224+
225+
// Verify the filter actually works when applied
226+
$results = MockFilterable::query()->filter($filter)->get();
227+
$this->assertCount(2, $results); // John Doe and Bob Johnson contain 'John'
228+
$this->assertTrue($results->every(fn ($r) => str_contains($r->name, 'John')));
224229
}
225230

226231
public function test_multiple_filter_resolutions_use_current_request(): void
227232
{
228-
// First request
229-
$this->app['request']->merge(['name' => 'John']);
233+
// First request - use $app->instance() for consistency
234+
$firstRequest = Request::create('/posts', 'GET', ['name' => 'John']);
235+
$this->app->instance('request', $firstRequest);
236+
$this->app->instance(Request::class, $firstRequest);
230237
$filter1 = $this->app->make(MockFilter::class);
231238

232-
// Modify request (simulating a different request)
233-
$newRequest = Request::create('/posts', 'GET', ['name' => 'Jane']);
234-
$this->app->instance('request', $newRequest);
235-
$this->app->instance(Request::class, $newRequest);
239+
// Second request - same approach for consistency
240+
$secondRequest = Request::create('/posts', 'GET', ['name' => 'Jane']);
241+
$this->app->instance('request', $secondRequest);
242+
$this->app->instance(Request::class, $secondRequest);
236243

237244
// Second filter resolution should get the new request
238245
$filter2 = $this->app->make(MockFilter::class);
@@ -391,14 +398,6 @@ class FilterWithDependencies extends Filter
391398
{
392399
protected array $filters = ['name', 'email'];
393400

394-
public function __construct(
395-
Request $request,
396-
?Cache $cache = null,
397-
?LoggerInterface $logger = null
398-
) {
399-
parent::__construct($request, $cache, $logger);
400-
}
401-
402401
public function hasCacheHandler(): bool
403402
{
404403
return $this->cache !== null;

0 commit comments

Comments
 (0)