Laravel 10.x

[jbrooksuk]

I'm working on the cachethq/core, which uses the laravel-query-builder package and I'm running into an issue.

I have an index method in my controller that looks like so:

public function index()
{
    $components = QueryBuilder::for(Component::class)
        ->allowedIncludes(['group', 'incidents'])
        ->allowedFilters(['name', 'status', 'enabled'])
        ->allowedSorts(['name', 'order', 'id'])
        ->simplePaginate(request('per_page', 15));

    return ComponentResource::collection($components);
}

We also have a test, like so:

it('can list components', function () {
    Component::factory(2)->create();

    $response = getJson('/status/api/components');

    $response->assertOk();
    $response->assertJsonCount(2, 'data');
});

But, I get an exception when running the test:

Iluminate\Http\Request::get(): Argument #1 ($key) must be of type string, null given, called in /Users/james/Code/CachetHQ/core/vendor/spatie/laravel-query-builder/src/QueryBuilderRequest.php on line 164

Looking at QueryBuilderRequest.php

protected function getRequestData(?string $key = null, $default = null)
{
    if (config('query-builder.request_data_source') === 'body') {
        return $this->input($key, $default);
    }

    return $this->get($key, $default);
}

I don't think the use of $this->get() is the right use here, as this is comment lives in Request.php:

This method belongs to Symfony HttpFoundation and is not usually needed when using Laravel.

Instead, you may use the "input" method.

[AlexVanderbist]

hey @jbrooksuk, thanks for letting us know. It looks like this originates from having the filter query parameter name be null. We never covered this in our tests so this issue slipped through.

I've tagged v5.4.0 with the fix you suggested (and some extra tests). Any chance you can upgrade to this version and check if that resolves the issue?

[jbrooksuk]

Thanks @AlexVanderbist. Unless I've done something wrong, this now seems to stop the simplePaginate(request('per_page', 15)) from working correctly:

{
  "message": "Requested include(s) `18` are not allowed. Allowed include(s) are `components`."
}

This is the test case:

it('can list more than 15 schedules', function () {
    Schedule::factory(20)->create();

    $response = getJson('/status/api/schedules?per_page=18');

    $response->assertOk();
    $response->assertJsonCount(18, 'data');
});

[AlexVanderbist]

Hey, I had a quick look at cachethq/core and I couldn't immediately find the query builder config. Without it, the filter & include parameters will default to null and the QueryBuilderRequest will look for includes/filters in the wrong place: https://github.com/spatie/laravel-query-builder/blob/main/src/QueryBuilderRequest.php#L37

I'll have a look if we can set some sensible defaults in all places where config values are used.

[jbrooksuk]

@AlexVanderbist the config gets published by Workbench. To test, you can run composer prepare and then composer test.

[AlexVanderbist]

@jbrooksuk looks like prepare just runs package discovery. After running composer build the tests turned green for me

[jbrooksuk]

Whoops, that's right. Though, I think your latest commit will fix all issues anyway.

[AlexVanderbist]

Great! I've just tagged it as 5.5.0 👍

[jbrooksuk]

@AlexVanderbist perfect, that's worked for me. Thanks!