Skip to content
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

Open
wants to merge 6 commits into
base: 9.0
Choose a base branch
from
Open

Added multiple search #2261

wants to merge 6 commits into from

Conversation

dima-bzz
Copy link

@dima-bzz dima-bzz commented Dec 6, 2019

Added multiple search for CollectionDataTable. If multiple => true, multi_term disable.

@yajra
Copy link
Owner

yajra commented Jan 7, 2020

Sorry for late response. Will try to review this as soon as I can. Thanks!

@yajra
Copy link
Owner

yajra commented Jan 7, 2020

@dima-bzz can you please elaborate the difference of this PR from multi_term search? Can you please provide some use case? Thanks!

*
* @param string $keyword
*/
protected function multiSearch($keywords)
Copy link
Owner

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,
Copy link
Owner

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);
            });
    }

@dima-bzz
Copy link
Author

dima-bzz commented Jan 7, 2020

@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.

@yajra
Copy link
Owner

yajra commented Jan 8, 2020

@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?

@dima-bzz
Copy link
Author

dima-bzz commented Jan 8, 2020

@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 <dimabzz@gmail.com>
@dima-bzz
Copy link
Author

@yajra removed duplicate code. Left both search methods.

@dima-bzz
Copy link
Author

@yajra What about my PR?

@yajra
Copy link
Owner

yajra commented Mar 3, 2020

@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 <dimabzz@gmail.com>
Signed-off-by: Dmitry Mazurov <dimabzz@gmail.com>
Signed-off-by: Dmitry Mazurov <dimabzz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants