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

feature #5948 Add mode to search all/any terms #5949

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

tasiot
Copy link
Contributor

@tasiot tasiot commented Oct 8, 2023

Follow up to #5948

This PR add a searchMode option in Crud to set if the search is an AND or OR operator between terms.

@tasiot tasiot force-pushed the feature/5948 branch 4 times, most recently from 916436b to 55b6417 Compare October 8, 2023 10:49
@tasiot
Copy link
Contributor Author

tasiot commented Nov 6, 2023

IMO the default search mode should be SearchMode::ALL_TERMS but for compatibility reasons I set ANY_TERMS as default.

Must I set ALL_TERMS as default?

Copy link
Contributor

@bytes-commerce bytes-commerce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand your PR correctly, ALL_TERMS would be correct search mode by default. Correct me if I am wrong. It means: Condition A and Condition B must match in order to display. Correct? If yes, I'd second your opinion here. Otherwise, maybe the owner wants to have the last word here. 🤔

@tasiot
Copy link
Contributor Author

tasiot commented Nov 6, 2023

Yes, it is.

When you type once word ANY_TERMS and ALL_TERMS are identical.
But with many words (=terms), all terms must be found: the word1 must be found in field1 or field2 or field3 but the word2 must also be found in field1, field2 or field3.

According my documentation:

$crud
    // match any terms (default mode)
    // term1 in (field1 or field2) or term2 in (field1 or field2)
    ->setSearchMode(SearchMode::ANY_TERMS)
    // force to match all the terms
    // term1 in (field1 or field2) and term2 in (field1 or field2)
    ->setSearchMode(SearchMode::ALL_TERMS)
;

@bytes-commerce
Copy link
Contributor

Then I absolutely agree with you. 👍 Thank you very much for explaining it once again.

@tasiot tasiot force-pushed the feature/5948 branch 8 times, most recently from 1e5ea0c to f542df6 Compare November 6, 2023 17:59
@tasiot
Copy link
Contributor Author

tasiot commented Nov 6, 2023

I just pushed a new commit to set ALL_TERMS as default.

I've kept it as a separate commit in case the owner wants to use ANY_TERMS as default like now.

@tasiot tasiot requested a review from bytes-commerce November 6, 2023 18:02
@javiereguiluz javiereguiluz added this to the 4.x milestone Nov 21, 2023
@javiereguiluz javiereguiluz merged commit dc6395c into EasyCorp:4.x Nov 21, 2023
23 checks passed
@javiereguiluz
Copy link
Collaborator

This is finally merged! Roland, thanks for this contribution. I played a bit with it and it worked nicely. I hope it's useful for other people too. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants