-
-
Notifications
You must be signed in to change notification settings - Fork 858
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
Added multiple search #2261
base: 9.0
Are you sure you want to change the base?
Added multiple search #2261
Conversation
Sorry for late response. Will try to review this as soon as I can. Thanks! |
@dima-bzz can you please elaborate the difference of this PR from |
src/CollectionDataTable.php
Outdated
* | ||
* @param string $keyword | ||
*/ | ||
protected function multiSearch($keywords) |
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.
This seems like a duplicate of globalSearch
method.
/* | ||
* Multiple search will explode search keyword using spaces resulting into multiple search. | ||
*/ | ||
'multiple' => false, |
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.
Multi_term already explodes the keyword using spaces.
/**
* Perform multi-term search by splitting keyword into
* individual words and searches for each of them.
*
* @param string $keyword
*/
protected function smartGlobalSearch($keyword)
{
collect(explode(' ', $keyword))
->reject(function ($keyword) {
return trim($keyword) === '';
})
->each(function ($keyword) {
$this->globalSearch($keyword);
});
}
@yajra Mutli_term searches through a space, but it works through a loop and each next iteration is searched in the previous one. Multiple search searches for everything in a single query, without looping. For example, I have a list of users and I need to find all users with Administrator and User rights. Through multi_term there will be only the Administrator, and through multiple search both the Administrator and the User. |
@dima-bzz thanks for elaborating. What you described should be the functionality of multi-term and thus it might be a bug on the current implementation. Maybe we should apply your PR to multi-term instead of creating a new multiple config? |
@yajra possible. But on the other hand, multi_term also has its own plus, it is a sequential search. For example, I first found all users with Administrator rights, and then found a specific user. Just for different tasks is convenient or multi_term or multiple search. You can simply combine the duplicated code, but leave both methods. |
Signed-off-by: Dmitry Mazurov <[email protected]>
@yajra removed duplicate code. Left both search methods. |
@yajra What about my PR? |
@dima-bzz a bit busy atm and can't find time to review this yet. Will try to give this some time within the week. Or if anyone in the community can review this would be a great help. |
…ipleSearch runtime set. Adding desctiption for null_last_sql for postgreSQL.
Signed-off-by: Dmitry Mazurov <[email protected]>
Signed-off-by: Dmitry Mazurov <[email protected]>
Signed-off-by: Dmitry Mazurov <[email protected]>
Added multiple search for CollectionDataTable. If multiple => true, multi_term disable.