The Scope Filter with null value

[mits87]

Hi,

I'm using your package and it's awesome but I have small problem. I have a really huge project and I'm using Repository pattern. I have Repository classes for instance the UserRepository is responsible for list, show, create, update, delete users. For each list actions in my all repository I pass $filters with default param null:

UserRepository

public function all(GenericFilterable $filters = null): LengthAwarePaginator {
    return \App\User::filter($filters)->paginate();
}

UsersController

public function index(UserFilters $filters) {
    $this->authorize('view', User::class);

    return UserResource::collection(
        $this->userRepository->all($filters)
    );
}

my problem is: sometimes I don't needed filtering then I don't pass any params for instance: $this->myOtherRepository->all()

In this case I got error from FilterableTrait:

Type error: Argument 2 passed to App\Base::scopeFilter() must be an instance of Kyslik\LaravelFilterable\FilterableContract, null given

IMHO: the scopeFilter should accept optional parameter:

public function scopeFilter(Builder $query, FilterableContract $filters = null)
{
    if (!empty($filters)) {
        return $filters->apply($query);
    }
    return $query;
}

What are you thinking about this solution?

[Kyslik]

I will work on it today night (CET). And make it more customizable.

I am really glad that you find the package useful! & Thanks for excellent report 👍 .

[Kyslik]

I now understand, what is going on (previously I read the issue on the phone).

Yea, package provides trait that you may use but its not really necessary :), just drop the code of scopeFilter(...) method inside your User.php model and use that one although I would use this:

public function scopeFilter(Builder $query, $filters = null) //notice no contract here its just a guideline for user of the package
{
    return is_null($filters) ? $query : $filters->apply($query);
}

Or create your own trait and use that one instead of stock one from the package! :)

---

I will sleep on this and will decide later if its really needed to have default filter() scope that accepts null values.

So far gathered upsides/downsides are (if we implement this to the package):

• upside controllers (in your case repositories) will be cleaner¹ - can be mitigated if user implements his/her own scope/trait
• downside default trait will be harder to understand

¹null check can live in the controller / repository itself.

[mits87]

Hi Kyslik,

Yea, package provides trait that you may use but its not really necessary :), just drop the code of scopeFilter(...) method inside your User.php model and use that one although I would use this:

Yes, I know but I have above 120 models where I need to add the scope.

Or create your own trait and use that one instead of stock one from the package! :)

Again yes, I can and I exactly did it in my project but it should be in package not in extra trait in my project - in my opinion.

So far gathered upsides/downsides are (if we implement this to the package): ... downside default trait will be harder to understand

hmm maybe not... if someone pass the $filters param as GenericFilterable then someone will know what should get when someone pass null then he will also know the filter doesn't change the results.

null check can live in the controller / repository itself.

No, because I have many controllers / repositories.

What do you think?

[Kyslik]

@mits87 I have not forgotten this issue, I am just really sceptic if it needs to be incorporated.

Could you please read:

• https://www.lucidchart.com/techblog/2015/08/31/the-worst-mistake-of-computer-science/

or search topics regarding of "passing null value to a method"