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

Fix #3651 - Insensitive searches with PostgreSQL #4783

Closed
wants to merge 1 commit into from

Conversation

papjul
Copy link
Contributor

@papjul papjul commented Mar 11, 2023

No description provided.

@papjul
Copy link
Contributor Author

papjul commented Mar 25, 2023

Let me know if you need clarifications about this pull request.

Currently, the search feature is almost unusable with PostgreSQL, because in France, we usually wrote surnames all in uppercase (so that we know the difference between John SMITH and Smith JOHN). However, people usually don’t write their search in uppercase (they will write "smith" and will not understand why the search says "no results").

Regarding the implementation, as far as I know, this class is the only one where LIKE should be replaced by ILIKE, other classes should keep the LIKE operator, since insensitive searches are not required in the other cases. That’s why I implemented it as a member of the SearchService class.

As far as I know, the ILIKE doesn't exist in MySQL and SQLite, that’s why I use a condition on the driver.

I believe it should also be backported to version 2.1, but I want to make sure the current implementation is fine for you, before making an additional pull request to the 2.1 branch.

@fisharebest
Copy link
Owner

Let me know if you need clarifications about this pull request.

Please be patient. I'm very busy right now.

I believe it should also be backported to version 2.1

I will merge the changes into both branches.

@papjul
Copy link
Contributor Author

papjul commented Mar 26, 2023

No worries, I didn't mean to rush you!
I just saw that you merged another pull request of mine within minutes so I thought something was wrong with this other pull request ;)

@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Merging #4783 (14e1611) into main (4867704) will increase coverage by 0.00%.
The diff coverage is 9.67%.

@@            Coverage Diff            @@
##               main    #4783   +/-   ##
=========================================
  Coverage     30.97%   30.97%           
- Complexity    11366    11367    +1     
=========================================
  Files          1168     1168           
  Lines         47868    47870    +2     
=========================================
+ Hits          14825    14826    +1     
- Misses        33043    33044    +1     
Impacted Files Coverage Δ
app/Services/SearchService.php 29.65% <9.67%> (+0.06%) ⬆️

@fisharebest
Copy link
Owner

There are a couple of issues with this (hence the delay in merging).

  1. A more general solution is required that supports other RDBMS. This will probably involve collations. We could probably fix this for postgres with collations, but I don't have the opportunity to test other RDBMS right now.

  2. The business logic functions app/Services/... are all pure. i.e. they have no local variables/state.

The first can wait - I'm looking at a rewrite of the database handling. For the second, I merged your code, and modified it to be pure.

Thanks for your contribution.

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

Successfully merging this pull request may close these issues.

2 participants