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 7e68215472..0b6fb080bb 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -2129,7 +2129,6 @@ class CRM_Contact_BAO_Query { $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 @@ class CRM_Contact_BAO_Query { 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 @@ class CRM_Contact_BAO_Query { $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 @@ WHERE $smartGroupClause $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 @@ WHERE $smartGroupClause $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 @@ WHERE $smartGroupClause 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 @@ WHERE $smartGroupClause } $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 @@ WHERE $smartGroupClause if ($opValues && strtolower($opValues[2] == 'AND') ) { + // @todo this line is logially unreachable $operator = 'AND'; } @@ -5700,7 +5694,7 @@ SELECT COUNT( conts.total_amount ) as cancel_count, $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 393d6f0cf9..755233f9b6 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -257,13 +257,13 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { 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 @@ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { ); $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 ebef36284e..fead0ec383 100644 --- a/tests/phpunit/api/v3/ContactTest.php +++ b/tests/phpunit/api/v3/ContactTest.php @@ -150,21 +150,17 @@ class api_v3_ContactTest extends CiviUnitTestCase { * * @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 @@ class api_v3_ContactTest extends CiviUnitTestCase { */ 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; -- 2.25.1