Tucker-Eric / EloquentFilter

An Eloquent Way To Filter Laravel Models And Their Relationships
http://tucker-eric.github.io/EloquentFilter
MIT License
1.72k stars 120 forks source link

Nested where not working #60

Closed techdaddies-kevin closed 6 years ago

techdaddies-kevin commented 6 years ago

This is my filter:

public function search($search)
    {
        return $this->where(function($q) use ($search)
        {
            return $q->where('description', 'LIKE', "%$search%")
                     ->orWhere('title', 'LIKE', "%$search%");
        });
    }

But this is the query that's being run:

select `listings`.*, (select count(*) from `media` where `listings`.`id` = `media`.`model_id` and `media`.`model_type` = 'App\Listing') as `media_count` from `listings` where (`description` LIKE 'App\Listing' or `title` LIKE '%a%') and `category_id` = '%a%' limit 2 offset 0

For some reason, the first instance of $search is returning the classname of the model instead of the search term that's being passed through the query string. I'm unsure if I've just set this up incorrect or if it's a bug.

Tucker-Eric commented 6 years ago

What does the method calling filter() look like?

Also, if you're passing anymore input to filter() besides ['search' => 'a'], can you provide that too?

And it may also help to see the media relationship definition on your Listing model

techdaddies-kevin commented 6 years ago

The call to filter() is $listings = Listing::filter($request->all())->with('manufacturer', 'media')->simplePaginateFilter(5);

The only other filters are for two foreign keys, and I'm just passing the ID so nothing complicated there

public function category($id)
    {
        return $this->where('category_id', $id);
    }

    public function manufacturer($id)
    {
        return $this->where('manufacturer_id', $id);
    }

    public function search($search)
    {
        return $this->where(function($q) use ($search)
        {
            return $q->where('description', 'LIKE', "%$search%")
                     ->orWhere('title', 'LIKE', "%$search%");
        });
    }

The media relationship comes from the medialibrary package, but I've removed that entirely from the filter call and still experience the same result.

Tucker-Eric commented 6 years ago

I have a feeling there may be something in your media relation on your Listing model. From what your sql prints out from your query it looks like there is an extra App\Listing being pushed to the bindings array of the query, causing all the rest of the parameters to be shifted forward by one.

From what it is printing out, I am assuming the input passed to filter is:

[
    'search'     => 'a',
    'category' => 2
];

Because from what that sql shows, the limit should be 5 from your paginator call, not 2 and if you remove the App\Listing from the nested where() in your search() method and shift all parameters left by one, the query starts to take shape.

techdaddies-kevin commented 6 years ago

The pagination param issue is just because I changed that value in the code between my first post and second here. Right now pagination is set to 5 per page and the query is:

select `listings`.*, (select count(*) from `media` where `listings`.`id` = `media`.`model_id` and `media`.`model_type` = 'App\Listing') as `media_count` from `listings` where (`description` LIKE 'App\Listing' or `title` LIKE '%a%') limit 6 offset 0

However, you're definitely right. Something is clashing between this code and the medialibrary code. If I remove the medialibrary interface and trait from my Listing model, the generated query is correct.

techdaddies-kevin commented 6 years ago

For what it's worth, I just added a global scope to the Listing model to only show results with a renewal date in the future. When I did that, the query changed to: select `listings`.*, (select count(*) from `media` where `listings`.`id` = `media`.`model_id` and `media`.`model_type` = 'App\Listing') as `media_count` from `listings` where (`description` LIKE 'App\Listing' or `title` LIKE '%a%') and `renewal_date` > '%a%' limit 6 offset 0 The problem is definitely that App\Listing is for some reason in the stack twice.

techdaddies-kevin commented 6 years ago

Sorry for the barrage of updates. If I run the search without any filters, this is the query generated with my new global scope in place:

select `listings`.*, (select count(*) from `media` where `listings`.`id` = `media`.`model_id` and `media`.`model_type` = 'App\Listing') as `media_count` from `listings` where `renewal_date` > '2018-04-19 17:36:13' limit 6 offset 0

As you can see, the date is correct and not "App\Listing". Additionally, if I run through my filters (category and manufacturer) WITHOUT specifying a search keyword, the query is correct. It is something specifically happening when using the where method INSIDE the anonymous function. If I simply use a

public function search($search)
    {
        return $this->where('description', 'LIKE', "%$search%");
    }

It works fine. But as soon as I do:

public function search($search)
    {
        return $this->where(function($q) use ($search)
        {
            return $q->where('description', 'LIKE', "%$search%");
        });
    }

It screws up.

Tucker-Eric commented 6 years ago

What version of Laravel and laravel-medialiabrary are you using?

techdaddies-kevin commented 6 years ago

7.1.3 medialibrary 5.6.16 laravel

Tucker-Eric commented 6 years ago

Looking into this, I think it may be a Laravel issue... Do you have $withCount = ['media'] on your Listing model?

techdaddies-kevin commented 6 years ago

Yes indeed.

Tucker-Eric commented 6 years ago

Then it definitely looks like a Laravel issue. If you removed that and added it as a global scope instead of a property, it should fix this issue.

What is going on is that in the nested where() laravel is calling newQueryWithoutScopes on the builder's model which loads that withCount in the nested query, registering the bindings for the withCount query so those get loaded into the subquery without the actual constraint which is where the bindings are getting shifted.

techdaddies-kevin commented 6 years ago

Confirmed. Adding a global scope and removing the withCounts property did fix this issue.

Tucker-Eric commented 6 years ago

Awesome! I'll open an issue in Laravel/send a PR to patch the issue