From 2fc640828ea218db079f46d6c16b07373c40f751 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 24 Oct 2018 08:54:54 +1300 Subject: [PATCH] Remove use of LOWER from city & street searches and fix international strings test. Adding LOWER to mysql queries makes them slower. lowercasing php strings breaks under some character sets with some server configs. In trying to review PR 12364 I dug into the strotolower behaviour. I found that in the path altered in this PR we are setting the value to lower case but not the mysql clause. I ran through ContactTest without these lines & found it only hit this strtolower in one test - testInternationalStrings which searches by organization name. I found the this test actually failed for me when I ran it locally - but it passed when I removed the strotlower lines. Wonder in jenkins finds the same --- CRM/Contact/BAO/Query.php | 22 ++++++++------------- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 6 +++--- tests/phpunit/api/v3/ContactTest.php | 14 +++++-------- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 7e682154722f..0b6fb080bb77 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -2129,7 +2129,6 @@ public function restWhere(&$values) { $setTables = TRUE; - $strtolower = function_exists('mb_strtolower') ? 'mb_strtolower' : 'strtolower'; $locationType = CRM_Core_PseudoConstant::get('CRM_Core_DAO_Address', 'location_type_id'); if (isset($locType[1]) && is_numeric($locType[1])) { $lType = $locationType[$locType[1]]; @@ -2268,7 +2267,7 @@ public function restWhere(&$values) { elseif (substr($name, 0, 4) === 'url-') { $tName = 'civicrm_website'; $this->_whereTables[$tName] = $this->_tables[$tName] = "\nLEFT JOIN civicrm_website ON ( civicrm_website.contact_id = contact_a.id )"; - $value = $strtolower(CRM_Core_DAO::escapeString($value)); + $value = CRM_Core_DAO::escapeString($value); if ($wildcard) { $op = 'LIKE'; $value = self::getWildCardedValue($wildcard, $op, $value); @@ -2338,9 +2337,6 @@ public function restWhere(&$values) { $this->_where[$grouping][] = CRM_Core_DAO::createSQLFilter($fieldName, $value, $type); } else { - if (!self::caseImportant($op)) { - $value = $strtolower($value); - } if ($wildcard) { $op = 'LIKE'; $value = self::getWildCardedValue($wildcard, $op, $value); @@ -3312,9 +3308,8 @@ public function notes(&$values) { $this->_tables['civicrm_note'] = $this->_whereTables['civicrm_note'] = " LEFT JOIN civicrm_note ON ( civicrm_note.entity_table = 'civicrm_contact' AND contact_a.id = civicrm_note.entity_id ) "; - $strtolower = function_exists('mb_strtolower') ? 'mb_strtolower' : 'strtolower'; $n = trim($value); - $value = $strtolower(CRM_Core_DAO::escapeString($n)); + $value = CRM_Core_DAO::escapeString($n); if ($wildcard) { if (strpos($value, '%') === FALSE) { $value = "%$value%"; @@ -3589,10 +3584,8 @@ public function street_number(&$values) { $this->_qill[$grouping][] = ts('Street Number is even'); } else { - $value = strtolower($n); - - // LOWER roughly translates to 'hurt my database without deriving any benefit' See CRM-19811. - $this->_where[$grouping][] = self::buildClause('LOWER(civicrm_address.street_number)', $op, $value, 'String'); + $value = $n; + $this->_where[$grouping][] = self::buildClause('civicrm_address.street_number', $op, $value, 'String'); $this->_qill[$grouping][] = ts('Street Number') . " $op '$n'"; } @@ -3608,7 +3601,7 @@ public function sortByCharacter(&$values) { list($name, $op, $value, $grouping, $wildcard) = $values; $name = trim($value); - $cond = " contact_a.sort_name LIKE '" . strtolower(CRM_Core_DAO::escapeWildCardString($name)) . "%'"; + $cond = " contact_a.sort_name LIKE '" . CRM_Core_DAO::escapeWildCardString($name) . "%'"; $this->_where[$grouping][] = $cond; $this->_qill[$grouping][] = ts('Showing only Contacts starting with: \'%1\'', array(1 => $name)); } @@ -3875,7 +3868,7 @@ public function changeLog(&$values) { } $name = trim($targetName[2]); - $name = strtolower(CRM_Core_DAO::escapeString($name)); + $name = CRM_Core_DAO::escapeString($name); $name = $targetName[4] ? "%$name%" : $name; $this->_where[$grouping][] = "contact_b_log.sort_name LIKE '%$name%'"; $this->_tables['civicrm_log'] = $this->_whereTables['civicrm_log'] = 1; @@ -3968,6 +3961,7 @@ public function privacyOptions($values) { if ($opValues && strtolower($opValues[2] == 'AND') ) { + // @todo this line is logially unreachable $operator = 'AND'; } @@ -5700,7 +5694,7 @@ public static function buildClause($field, $op, $value = NULL, $dataType = NULL) $value = CRM_Utils_Type::escape($value, $dataType); // if we don't have a dataType we should assume if ($dataType == 'String' || $dataType == 'Text') { - $value = "'" . strtolower($value) . "'"; + $value = "'" . $value . "'"; } return "$clause $value"; } diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 393d6f0cf993..755233f9b6f7 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -257,13 +257,13 @@ public function testSearchProfilePrimaryCityCRM14263($params, $selectClause, $wh public function getSearchProfileData() { return [ [ - [['city', '=', 'Cool City', 1, 0]], "civicrm_address.city as `city`", "civicrm_address.city = 'cool city'", + [['city', '=', 'Cool City', 1, 0]], "civicrm_address.city as `city`", "civicrm_address.city = 'Cool City'", ], [ // Note that in the query 'long street' is lower cased. We eventually want to change that & not mess with the vars - it turns out // it doesn't work on some charsets. However, the the lcasing affects more vars & we are looking to stagger removal of lcasing 'in case' // (although we have been removing without blowback since 2017) - [['street_address', '=', 'Long Street', 1, 0]], "civicrm_address.street_address as `street_address`", "civicrm_address.street_address LIKE '%long street%'", + [['street_address', '=', 'Long Street', 1, 0]], "civicrm_address.street_address as `street_address`", "civicrm_address.street_address LIKE '%Long Street%'", ], ]; } @@ -382,7 +382,7 @@ public function testNonNumericEqualsPostal() { ); $sql = $query->query(FALSE); - $this->assertEquals("WHERE ( civicrm_address.postal_code = 'eh10 4rb-889' ) AND (contact_a.is_deleted = 0)", $sql[2]); + $this->assertEquals("WHERE ( civicrm_address.postal_code = 'EH10 4RB-889' ) AND (contact_a.is_deleted = 0)", $sql[2]); $result = CRM_Core_DAO::executeQuery(implode(' ', $sql)); $this->assertEquals(1, $result->N); diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php index ebef36284e3c..fead0ec3838e 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -150,21 +150,17 @@ public function testCreateIndividualNoCacheClear() { * * @param string $string * String to be tested. - * @param bool $checkCharSet + * * Bool to see if we should check charset. * * @throws \Exception */ - public function testInternationalStrings($string, $checkCharSet = FALSE) { + public function testInternationalStrings($string) { $this->callAPISuccess('Contact', 'create', array_merge( $this->_params, array('first_name' => $string) )); - if ($checkCharSet) { - if (!CRM_Utils_SQL::supportStorageOfAccents()) { - $this->markTestIncomplete('Database is not Charset UTF8 therefore can not store accented strings properly'); - } - } + $result = $this->callAPISuccessGetSingle('Contact', array('first_name' => $string)); $this->assertEquals($string, $result['first_name']); @@ -183,8 +179,8 @@ public function testInternationalStrings($string, $checkCharSet = FALSE) { */ public function getInternationalStrings() { $invocations = array(); - $invocations[] = array('Scarabée', TRUE); - $invocations[] = array('Iñtërnâtiônàlizætiøn', TRUE); + $invocations[] = array('Scarabée'); + $invocations[] = array('Iñtërnâtiônàlizætiøn'); $invocations[] = array('これは日本語のテキストです。読めますか'); $invocations[] = array('देखें हिन्दी कैसी नजर आती है। अरे वाह ये तो नजर आती है।'); return $invocations;