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 lowercasing in relationship query #13034

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Removes another instance of non-functional lower casing in searches - which cause some strings to be hishandled

Before

Relationship search passes strings through lower casing. This is not compared with a lower case string so mysql handles the comparison, but can fail on some charactersets. I acheives nothing & causes edge-case harm

After

strtolower removed

Technical Details

This was picked up as being tested through testCreateSavedSearchWithSmartGroup as part of #12494 - the test actually already uses a non-lower case string 'Default Organization' - I found the casting to an array broke my ability to step through the code line by line so I tidied that slightly.

Comments

@civibot civibot bot added the master label Oct 29, 2018
@civibot
Copy link

civibot bot commented Oct 29, 2018

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

test this please

1 similar comment
@eileenmcnaughton
Copy link
Contributor Author

test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 passed now

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 my feeling is we are better to merge the rest of these tweaks into one release now - it feels less risky than spreading out the 'odds & sods' ones - although I'm not that worried now.

@seamuslee001
Copy link
Contributor

This function is well backed up by unit tests following recent work. I am confident enough that if there was an issue Jenkins will have found it. Merging

@seamuslee001 seamuslee001 merged commit ca2f3f8 into civicrm:master Nov 2, 2018
@eileenmcnaughton eileenmcnaughton deleted the strtolower_rels branch November 2, 2018 00:28
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