selahattinunlu / laravel-api-query-builder

Laravel & Lumen Api Query Builder Package
331 stars 66 forks source link

Fix pagination on models with scopes #21

Closed RoyXiang closed 7 years ago

RoyXiang commented 7 years ago

If a model has some scopes, like a most common one - SoftDeletes. If there are some records in the corresponding table in database, and one of them is soft deleted, the total on pagination would still include that deleted one.

Refer to this line, I'll suggest using toBase instead of getQuery which would apply the scopes to the model.

selahattinunlu commented 7 years ago

Hi @RoyXiang Thanks for pull request. Yeah you're right exactly. But "toBase" method added in 5.2 version. This package supports greater than or equal 5.0 version also. I guess we need to add "applyScopes" method inside to QueryBuilder. Then we can use it like $this->query->applyScopes()->getQuery()

If you could update your pull request as I said above, I will glad to merge it :) :+1:

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Builder.php#L1314

RoyXiang commented 7 years ago

Updated. :) @selahattinunlu

selahattinunlu commented 7 years ago

@RoyXiang

I guess you understood me wrong. We need to add a method into our QueryBuilder class. That method should do what Illuminate/Database/Eloquent/Builder::applyScopes method does. And It must also be compatible with the older version. (>=5.0 version)

RoyXiang commented 7 years ago

After some investigation, global scopes on a Eloquent model was introduced in laravel 5.2 for the first time. Before that, we don't need to care about the scopes on a model.

So, I'd prefer to check if toBase available on $this->query using method_exists instead of implementing another applyScopes.

Again, updated. :)

selahattinunlu commented 7 years ago

@RoyXiang

Yeah it's good idea! Thanks for contribution. Merged :)