-
-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support one whereHas for several filters. #931
Conversation
- Modify FiltersQuery to support AllowedRelationshipFilter. - Modify AllowedFilter to support AllowedRelationshipFilter. - Added tests for different exist clauses cases.
$this->allowedFilters->each( | ||
function (AllowedFilterContract $allowedFilter) use ($query, $value) { | ||
$allowedFilter->filter( | ||
QueryBuilder::for($query), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but why not pass the current $query
object? It should already be an instance of QueryBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to swap to $query
and get error.
Spatie\QueryBuilder\AllowedFilter::filter(): Argument #1 ($query) must be of type Spatie\QueryBuilder\QueryBuilder, Illuminate\Database\Eloquent\Builder given, called in /src/AllowedRelationshipFilter.php on line 27
Keeping this as is.
public function isRequested(QueryBuilderRequest $request): bool | ||
{ | ||
return $request->filters()->has($this->getName()); | ||
} | ||
|
||
public function getValueFromRequest(QueryBuilderRequest $request): mixed | ||
{ | ||
return $request->filters()->get($this->getName()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice little refactor 👍
Hey @IndexZer0, thanks for the extensive research and PR! As the impact on the existing filters is minimal, I'm happy to merge this in its current state in the v6 release and see what feedback comes in once people start using the Is there anything specific you would like feedback on? |
- Modify FiltersQuery to support AllowedRelationshipFilter. - Modify AllowedFilter to support AllowedRelationshipFilter. - Added tests for different exist clauses cases.
# Conflicts: # src/Concerns/FiltersQuery.php
Thanks for reviewing @AlexVanderbist Not particulary after specific feedback if you're happy to merge this feature. Just was open to changing the implementation approach if you had any better ideas on how to structure this.
FYI - As it turns out, this package is not going to suit my use case anymore so doubt I'd have the desire to update this PR again. Hope you can get this merged soon before more merge conflicts :) |
Dear contributor, because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it. |
This PR adds
AllowedRelationshipFilter
class to be used when wanting to group multipleAllowedFilter
's into the same exists query.Problem:
When using multiple
AllowedFilter
's for nested relations - the package adds mutliple exist clauses to the query.EXISTS
queries.Desired Query
EXISTS
queries.Usage
I have seen various requests for this in the past, for example
#634
#663
At this point just looking for your thoughts and feedback on this.
To me, this feels like quite an important missing feature of this package and if the core maintainers could give some feedback, insight and pointers on this then I'm happy to make any changes and then update documentation to match.