From eba173d631c51d981f83626416fa5d7a951a23e5 Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 11 Dec 2020 08:42:27 +1300 Subject: [PATCH] Clean up error handling in legacy functions in import parser This makes the handling of errors cleaner & makes it easier for us to unravel what is going on here. --- CRM/Contact/Import/Parser/Contact.php | 14 +++--- CRM/Utils/DeprecatedUtils.php | 30 +++++------- .../CRM/Contact/Import/Parser/ContactTest.php | 37 +++++++------- .../phpunit/CRM/Utils/DeprecatedUtilsTest.php | 48 ++++++++++++------- 4 files changed, 72 insertions(+), 57 deletions(-) diff --git a/CRM/Contact/Import/Parser/Contact.php b/CRM/Contact/Import/Parser/Contact.php index 21579263f1..9b3124dad8 100644 --- a/CRM/Contact/Import/Parser/Contact.php +++ b/CRM/Contact/Import/Parser/Contact.php @@ -1560,13 +1560,15 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Contact_Import_Parser { //@todo direct call to API function not supported. // setting required check to false, CRM-2839 // plus we do our own required check in import - $error = _civicrm_api3_deprecated_contact_check_params($formatted, $dupeCheck, $dedupeRuleGroupID); - - if ((is_null($error)) && (civicrm_error(_civicrm_api3_deprecated_validate_formatted_contact($formatted)))) { - $error = _civicrm_api3_deprecated_validate_formatted_contact($formatted); + try { + $error = _civicrm_api3_deprecated_contact_check_params($formatted, $dupeCheck, $dedupeRuleGroupID); + if ($error) { + return $error; + } + _civicrm_api3_deprecated_validate_formatted_contact($formatted); } - if (!is_null($error)) { - return $error; + catch (CRM_Core_Exception $e) { + return ['error_message' => $e->getMessage(), 'is_error' => 1, 'code' => $e->getCode()]; } if ($contactId) { diff --git a/CRM/Utils/DeprecatedUtils.php b/CRM/Utils/DeprecatedUtils.php index af1b411902..074909d28a 100644 --- a/CRM/Utils/DeprecatedUtils.php +++ b/CRM/Utils/DeprecatedUtils.php @@ -567,9 +567,9 @@ function _civicrm_api3_deprecated_duplicate_formatted_contact($params) { * @param array $params * Structured parameter list (as in crm_format_params). * - * @return bool|CRM_Core_Error + * @throw CRM_Core_Error */ -function _civicrm_api3_deprecated_validate_formatted_contact(&$params) { +function _civicrm_api3_deprecated_validate_formatted_contact(&$params): void { // Look for offending email addresses if (array_key_exists('email', $params)) { @@ -580,12 +580,12 @@ function _civicrm_api3_deprecated_validate_formatted_contact(&$params) { if ($email = CRM_Utils_Array::value('email', $values)) { // validate each email if (!CRM_Utils_Rule::email($email)) { - return civicrm_api3_create_error('No valid email address'); + throw new CRM_Core_Exception('No valid email address'); } // check for loc type id. if (empty($values['location_type_id'])) { - return civicrm_api3_create_error('Location Type Id missing.'); + throw new CRM_Core_Exception('Location Type Id missing.'); } } } @@ -600,8 +600,8 @@ function _civicrm_api3_deprecated_validate_formatted_contact(&$params) { CRM_Utils_Array::value('value', $value) ); if (!$valid && $value['is_required']) { - return civicrm_api3_create_error('Invalid value for custom field \'' . - CRM_Utils_Array::value('name', $custom) . '\'' + throw new CRM_Core_Exception('Invalid value for custom field \'' . + $custom['name'] . '\'' ); } if (CRM_Utils_Array::value('type', $custom) == 'Date') { @@ -611,8 +611,6 @@ function _civicrm_api3_deprecated_validate_formatted_contact(&$params) { } } } - - return civicrm_api3_create_success(TRUE); } /** @@ -698,9 +696,9 @@ function _civicrm_api3_deprecated_participant_check_params($params, $checkDuplic /** * @param array $params * @param bool $dupeCheck - * @param int $dedupeRuleGroupID + * @param null|int $dedupeRuleGroupID * - * @return array|null + * @throws \CRM_Core_Exception */ function _civicrm_api3_deprecated_contact_check_params( &$params, @@ -731,16 +729,16 @@ function _civicrm_api3_deprecated_contact_check_params( // contact_type has a limited number of valid values if (empty($params['contact_type'])) { - return civicrm_api3_create_error("No Contact Type"); + throw new CRM_Core_Exception("No Contact Type"); } $fields = $required[$params['contact_type']] ?? NULL; if ($fields == NULL) { - return civicrm_api3_create_error("Invalid Contact Type: {$params['contact_type']}"); + throw new CRM_Core_Exception("Invalid Contact Type: {$params['contact_type']}"); } if ($csType = CRM_Utils_Array::value('contact_sub_type', $params)) { if (!(CRM_Contact_BAO_ContactType::isExtendsContactType($csType, $params['contact_type']))) { - return civicrm_api3_create_error("Invalid or Mismatched Contact Subtype: " . implode(', ', (array) $csType)); + throw new CRM_Core_Exception("Invalid or Mismatched Contact Subtype: " . implode(', ', (array) $csType)); } } @@ -769,7 +767,7 @@ function _civicrm_api3_deprecated_contact_check_params( } if (!$valid) { - return civicrm_api3_create_error("Required fields not found for {$params['contact_type']} : $error"); + throw new CRM_Core_Exception("Required fields not found for {$params['contact_type']} : $error"); } } } @@ -796,7 +794,7 @@ function _civicrm_api3_deprecated_contact_check_params( // check for mismatch employer name and id if (!empty($params['employer_id']) && !in_array($params['employer_id'], $dupeIds) ) { - return civicrm_api3_create_error('Employer name and Employer id Mismatch'); + throw new CRM_Core_Exception('Employer name and Employer id Mismatch'); } // show error if multiple organisation with same name exist @@ -805,8 +803,6 @@ function _civicrm_api3_deprecated_contact_check_params( return civicrm_api3_create_error('Found more than one Organisation with same Name.'); } } - - return NULL; } /** diff --git a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php index b445733873..07a2e21d20 100644 --- a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php +++ b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php @@ -49,13 +49,11 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { /** * Test that import parser will add contact with employee of relationship. - * - * @throws \Exception */ - public function testImportParserWtihEmployeeOfRelationship() { + public function testImportParserWithEmployeeOfRelationship(): void { $this->organizationCreate([ - "organization_name" => "Agileware", - "legal_name" => "Agileware", + 'organization_name' => 'Agileware', + 'legal_name' => 'Agileware', ]); $contactImportValues = [ "first_name" => "Alok", @@ -142,7 +140,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportParserWithUpdateWithExternalIdentifier() { - list($originalValues, $result) = $this->setUpBaseContact(['external_identifier' => 'windows']); + [$originalValues, $result] = $this->setUpBaseContact(['external_identifier' => 'windows']); $this->assertEquals($result['id'], CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', 'windows', 'id', 'external_identifier', TRUE)); $this->assertEquals('windows', $result['external_identifier']); @@ -165,7 +163,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatch() { - list($originalValues, $result) = $this->setUpBaseContact([ + [$originalValues, $result] = $this->setUpBaseContact([ 'external_identifier' => 'windows', 'email' => NULL, ]); @@ -190,7 +188,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportParserWithUpdateWithContactID() { - list($originalValues, $result) = $this->setUpBaseContact([ + [$originalValues, $result] = $this->setUpBaseContact([ 'external_identifier' => '', 'email' => NULL, ]); @@ -210,7 +208,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportParserWithUpdateWithNoExternalIdentifier() { - list($originalValues, $result) = $this->setUpBaseContact(); + [$originalValues, $result] = $this->setUpBaseContact(); $originalValues['nick_name'] = 'Old Bill'; $originalValues['external_identifier'] = 'windows'; $this->runImport($originalValues, CRM_Import_Parser::DUPLICATE_UPDATE, CRM_Import_Parser::VALID); @@ -225,7 +223,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportParserWithUpdateWithChangedExternalIdentifier() { - list($contactValues, $result) = $this->setUpBaseContact(['external_identifier' => 'windows']); + [$contactValues, $result] = $this->setUpBaseContact(['external_identifier' => 'windows']); $contact_id = $result['id']; $contactValues['nick_name'] = 'Old Bill'; $contactValues['external_identifier'] = 'android'; @@ -241,7 +239,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportBillingAddress() { - list($contactValues) = $this->setUpBaseContact(); + [$contactValues] = $this->setUpBaseContact(); $contactValues['nick_name'] = 'Old Bill'; $contactValues['external_identifier'] = 'android'; $contactValues['street_address'] = 'Big Mansion'; @@ -351,7 +349,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportPrimaryAddress() { - list($contactValues) = $this->setUpBaseContact(); + [$contactValues] = $this->setUpBaseContact(); $contactValues['nick_name'] = 'Old Bill'; $contactValues['external_identifier'] = 'android'; $contactValues['street_address'] = 'Big Mansion'; @@ -426,7 +424,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { */ public function testAddressWithCustomData() { $ids = $this->entityCustomGroupWithSingleFieldCreate('Address', 'AddressTest.php'); - list($contactValues) = $this->setUpBaseContact(); + [$contactValues] = $this->setUpBaseContact(); $contactValues['nick_name'] = 'Old Bill'; $contactValues['external_identifier'] = 'android'; $contactValues['street_address'] = 'Big Mansion'; @@ -534,7 +532,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportDeceased() { - list($contactValues) = $this->setUpBaseContact(); + [$contactValues] = $this->setUpBaseContact(); CRM_Core_Session::singleton()->set("dateTypes", 1); $contactValues['birth_date'] = '1910-12-17'; $contactValues['deceased_date'] = '2010-12-17'; @@ -552,7 +550,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportTwoAddressFirstPrimary() { - list($contactValues) = $this->setUpBaseContact(); + [$contactValues] = $this->setUpBaseContact(); $contactValues['nick_name'] = 'Old Bill'; $contactValues['external_identifier'] = 'android'; $contactValues['street_address'] = 'Big Mansion'; @@ -617,7 +615,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportTwoAddressSecondPrimary() { - list($contactValues) = $this->setUpBaseContact(); + [$contactValues] = $this->setUpBaseContact(); $contactValues['nick_name'] = 'Old Bill'; $contactValues['external_identifier'] = 'android'; $contactValues['street_address'] = 'Big Mansion'; @@ -656,7 +654,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @throws \Exception */ public function testImportPrimaryAddressUpdate() { - list($contactValues) = $this->setUpBaseContact(['external_identifier' => 'android']); + [$contactValues] = $this->setUpBaseContact(['external_identifier' => 'android']); $contactValues['email'] = 'melinda.gates@microsoft.com'; $contactValues['phone'] = '98765'; $contactValues['external_identifier'] = 'android'; @@ -855,8 +853,11 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * that method does not cope with duplicates. * @param int|null $ruleGroupId * To test against a specific dedupe rule group, pass its ID as this argument. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ - protected function runImport($originalValues, $onDuplicateAction, $expectedResult, $mapperLocType = [], $fields = NULL, int $ruleGroupId = NULL) { + protected function runImport(array $originalValues, $onDuplicateAction, $expectedResult, $mapperLocType = [], $fields = NULL, int $ruleGroupId = NULL): void { if (!$fields) { $fields = array_keys($originalValues); } diff --git a/tests/phpunit/CRM/Utils/DeprecatedUtilsTest.php b/tests/phpunit/CRM/Utils/DeprecatedUtilsTest.php index 2cb51c4195..3091598b40 100644 --- a/tests/phpunit/CRM/Utils/DeprecatedUtilsTest.php +++ b/tests/phpunit/CRM/Utils/DeprecatedUtilsTest.php @@ -27,17 +27,23 @@ class CRM_Utils_DeprecatedUtilsTest extends CiviUnitTestCase { /** * Test civicrm_contact_check_params with no contact type. */ - public function testCheckParamsWithNoContactType() { + public function testCheckParamsWithNoContactType(): void { $params = ['foo' => 'bar']; - $contact = _civicrm_api3_deprecated_contact_check_params($params, FALSE); - $this->assertEquals(1, $contact['is_error']); + try { + _civicrm_api3_deprecated_contact_check_params($params, FALSE); + } + catch (CRM_Core_Exception $e) { + return; + } + $this->fail('An exception should have been thrown'); } /** - * Test civicrm_contact_check_params with a duplicate. - * and request the error in array format + * Test civicrm_contact_check_params with a duplicate. + * + * @throws \CiviCRM_API3_Exception */ - public function testCheckParamsWithDuplicateContact2() { + public function testCheckParamsWithDuplicateContact2(): void { $this->individualCreate(['first_name' => 'Test', 'last_name' => 'Contact', 'email' => 'TestContact@example.com']); $params = [ @@ -46,21 +52,31 @@ class CRM_Utils_DeprecatedUtilsTest extends CiviUnitTestCase { 'email' => 'TestContact@example.com', 'contact_type' => 'Individual', ]; - $contact = _civicrm_api3_deprecated_contact_check_params($params, TRUE); - $this->assertEquals(1, $contact['is_error']); - $this->assertRegexp("/matching contacts.*1/s", - $contact['error_message']['message'] - ); + try { + $error = _civicrm_api3_deprecated_contact_check_params($params, TRUE); + $this->assertEquals(1, $error['is_error']); + } + catch (CRM_Core_Exception $e) { + $this->assertRegexp("/matching contacts.*1/s", + $e->getMessage() + ); + return; + } + // $this->fail('Exception was not optional'); } /** - * Test civicrm_contact_check_params with check for required - * params and no params + * Test civicrm_contact_check_params with check for required params. */ - public function testCheckParamsWithNoParams() { + public function testCheckParamsWithNoParams(): void { $params = []; - $contact = _civicrm_api3_deprecated_contact_check_params($params, FALSE); - $this->assertEquals(1, $contact['is_error']); + try { + _civicrm_api3_deprecated_contact_check_params($params, FALSE); + } + catch (CRM_Core_Exception $e) { + return; + } + $this->fail('Exception required'); } } -- 2.25.1