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

Remove instances of LOWER casing searches from sort_name, display_name #12987

Merged
merged 1 commit into from
Oct 29, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Improve performance and reliability of searches involving sort_name & display_name (under some circumstances) by relying on mysql to handle case.

Before

We actively convert the string to use lower case in php and then use mysql LOWER for the comparison

After

We let mysql do what it does well. Test added

Technical Details

Per #12494 mysql handles lcase.

Adding LOWER to mysql queries makes them slower. lowercasing php strings
breaks under some character sets with some server configs. Less is more

Comments

@seamuslee001 @monishdeb @colemanw I think we should push through on these since we have been chipping away for a few months after this discussion #12494 with no fallout

Per civicrm#12494 mysql handles lcase.

Adding LOWER to mysql queries makes them slower. lowercasing php strings
breaks under some character sets with some server configs. Less is more
@civibot
Copy link

civibot bot commented Oct 23, 2018

(Standard links)

@civibot civibot bot added the master label Oct 23, 2018
@eileenmcnaughton eileenmcnaughton changed the title Remove instances of LOWERE casing searches from sort_name, display_name. Remove instances of LOWER casing searches from sort_name, display_name, email Oct 23, 2018
@eileenmcnaughton eileenmcnaughton changed the title Remove instances of LOWER casing searches from sort_name, display_name, email Remove instances of LOWER casing searches from sort_name, display_name Oct 23, 2018
@seamuslee001
Copy link
Contributor

Code change looks sensible here, again there should be enough coverage of this change in the unit tests to prevent regression, Jenkins has passed. Merging

@seamuslee001 seamuslee001 merged commit 8638750 into civicrm:master Oct 29, 2018
@seamuslee001 seamuslee001 deleted the strtolower_sort branch October 29, 2018 07:14
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.

2 participants