cerbero90 / query-filters

Laravel package to filter Eloquent model records based on query parameters. Fully inspired by the Laracasts episode https://laracasts.com/series/eloquent-techniques/episodes/4
MIT License
84 stars 15 forks source link

Discussion: do not apply filter if value is empty #3

Closed gutierrezps closed 7 years ago

gutierrezps commented 7 years ago

I think a filter shouldn't be applied when its value is empty. Or is it proposital to let it be checked by each filter?

For example,

class OSFilters extends QueryFilters
{
    public function status($id)
    {
        if(!empty($id)) $this->query->where('status_id', $id);
    }
}

If I don't do the empty check, the query will return only models with empty status_id field.

cerbero90 commented 7 years ago

You are right, it's common to check whether a value is empty or null, the problem is that query parameters may not have a value associated.

E.g. /?foo is a perfectly valid query parameter and would be ignored if we relied on its value.

gutierrezps commented 7 years ago

That's right. Then, you could add attributes like $ignoreIfEmpty and $dontIgnoreIfEmpty whose values would be the filters we would like to ignore/apply, like Eloquent's $fillable and $guarded Model attributes, like this:

class OSFilters extends QueryFilters
{
    protected $ignoreIfEmpty = ['status'];

    public function status($id)
    {
        $this->query->where('status_id', $id);
    }
}

In my very brief experience, I would use $dontIgnoreIfEmpty more often, because I'm using boolean filters less often.

Other names that come to my mind are $boolean, $needValue...

cerbero90 commented 7 years ago

I appreciate your suggestion and have updated the package with your requested feature.

Now all the filters without a value are not applied but it's possible to register the implicit filters (the ones that don't need a value) in the related property implicitFilters.

Thanks again for your help :)

gutierrezps commented 7 years ago

Thank you for your time.

I've just updated, and it's not working. I saw the way you did it, and instead of

$valueIsLegit = $value !== '' || in_array($filter, $this->implicitFilters);

you should do

$valueIsLegit = !empty($value) || in_array($filter, $this->implicitFilters);

Since an empty query parameter is null, and not an empty string, because of ConvertEmptyStringsToNull middleware introduced in Laravel 5.4.

gutierrezps commented 7 years ago

Oh, and you got a typo in the readme example. You forgot the $ in $implicitFilters code example.

cerbero90 commented 7 years ago

Thanks @gutierrezps for your watchful eye! :)

I fixed the errors you spotted. I preferred the condition $value !== '' && $value !== null to !empty($value) because the latter would snob a valid value like 0

gutierrezps commented 7 years ago

Indeed, you're right. Thanks for this awesome package.