From 4a0e3fe74b5ac5bce64d23166d10502f2c4aa6ab Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Sun, 24 Nov 2019 08:52:04 +1100 Subject: [PATCH] dev/core#560 Replace instances of CRM_Core_Error::fatal with Exceptions or Status bounces in SMS processing and appropriately handle error after --- CRM/Activity/BAO/Activity.php | 36 ++++++++----------- CRM/Core/BAO/ActionSchedule.php | 17 +++++---- CRM/SMS/BAO/Provider.php | 4 +-- CRM/SMS/Form/Group.php | 2 +- CRM/SMS/Form/Schedule.php | 2 +- CRM/SMS/Provider.php | 5 +-- .../phpunit/CRM/Activity/BAO/ActivityTest.php | 6 ++-- 7 files changed, 36 insertions(+), 36 deletions(-) diff --git a/CRM/Activity/BAO/Activity.php b/CRM/Activity/BAO/Activity.php index 5d14e5f6ad..930aaf6313 100644 --- a/CRM/Activity/BAO/Activity.php +++ b/CRM/Activity/BAO/Activity.php @@ -1336,21 +1336,19 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { $errMsgs[] = PEAR::raiseError('Contact Does not accept SMS', NULL, PEAR_ERROR_RETURN); } else { - $sendResult = self::sendSMSMessage( - $contactId, - $tokenText, - $smsProviderParams, - $activityID, - $sourceContactId - ); - - if (PEAR::isError($sendResult)) { - // Collect all of the PEAR_Error objects - $errMsgs[] = $sendResult; - } - else { + try { + $sendResult = self::sendSMSMessage( + $contactId, + $tokenText, + $smsProviderParams, + $activityID, + $sourceContactId + ); $success++; } + catch (CRM_Core_Exception $e) { + $errMsgs[] = $e->getMessage(); + } } } @@ -1381,8 +1379,8 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { * The activity ID that tracks the message. * @param int $sourceContactID * - * @return bool|PEAR_Error - * true on success or PEAR_Error object + * @return bool true on success + * @throws CRM_Core_Exception */ public static function sendSMSMessage( $toID, @@ -1411,11 +1409,7 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { // make sure both phone are valid // and that the recipient wants to receive sms if (empty($toPhoneNumber)) { - return PEAR::raiseError( - 'Recipient phone number is invalid or recipient does not want to receive SMS', - NULL, - PEAR_ERROR_RETURN - ); + throw new CRM_Core_Exception('Recipient phone number is invalid or recipient does not want to receive SMS'); } $recipient = $toPhoneNumber; @@ -1425,7 +1419,7 @@ class CRM_Activity_BAO_Activity extends CRM_Activity_DAO_Activity { $providerObj = CRM_SMS_Provider::singleton(['provider_id' => $smsProviderParams['provider_id']]); $sendResult = $providerObj->send($recipient, $smsProviderParams, $tokenText, NULL, $sourceContactID); if (PEAR::isError($sendResult)) { - return $sendResult; + throw new CRM_Core_Exception($sendResult->getMessage()); } // add activity target record for every sms that is sent diff --git a/CRM/Core/BAO/ActionSchedule.php b/CRM/Core/BAO/ActionSchedule.php index 7d3c857ac5..d65d7b628b 100644 --- a/CRM/Core/BAO/ActionSchedule.php +++ b/CRM/Core/BAO/ActionSchedule.php @@ -558,12 +558,17 @@ FROM civicrm_action_schedule cas $activity = CRM_Activity_BAO_Activity::create($activityParams); - CRM_Activity_BAO_Activity::sendSMSMessage($tokenRow->context['contactId'], - $sms_body_text, - $smsParams, - $activity->id, - $userID - ); + try { + CRM_Activity_BAO_Activity::sendSMSMessage($tokenRow->context['contactId'], + $sms_body_text, + $smsParams, + $activity->id, + $userID + ); + } + catch (CRM_Core_Exception $e) { + return ["sms_send_error" => $e->getMessage()]; + } return []; } diff --git a/CRM/SMS/BAO/Provider.php b/CRM/SMS/BAO/Provider.php index 4792a049f0..4e046f7433 100644 --- a/CRM/SMS/BAO/Provider.php +++ b/CRM/SMS/BAO/Provider.php @@ -123,11 +123,11 @@ class CRM_SMS_BAO_Provider extends CRM_SMS_DAO_Provider { * @param int $providerID * * @return null - * @throws Exception + * @throws CRM_Core_Exception */ public static function del($providerID) { if (!$providerID) { - CRM_Core_Error::fatal(ts('Invalid value passed to delete function.')); + throw new CRM_Core_Exception(ts('Invalid value passed to delete function.')); } $dao = new CRM_SMS_DAO_Provider(); diff --git a/CRM/SMS/Form/Group.php b/CRM/SMS/Form/Group.php index 3e32f31519..7aae52fb1b 100644 --- a/CRM/SMS/Form/Group.php +++ b/CRM/SMS/Form/Group.php @@ -25,7 +25,7 @@ class CRM_SMS_Form_Group extends CRM_Contact_Form_Task { */ public function preProcess() { if (!CRM_SMS_BAO_Provider::activeProviderCount()) { - CRM_Core_Error::fatal(ts('The SMS Provider has not been configured or is not active.', [1 => CRM_Utils_System::url('civicrm/admin/sms/provider', 'reset=1')])); + CRM_Core_Error::statusBounce(ts('The SMS Provider has not been configured or is not active.', [1 => CRM_Utils_System::url('civicrm/admin/sms/provider', 'reset=1')])); } $session = CRM_Core_Session::singleton(); diff --git a/CRM/SMS/Form/Schedule.php b/CRM/SMS/Form/Schedule.php index 1fb81cd182..5cdf860cac 100644 --- a/CRM/SMS/Form/Schedule.php +++ b/CRM/SMS/Form/Schedule.php @@ -134,7 +134,7 @@ class CRM_SMS_Form_Schedule extends CRM_Core_Form { $params['mailing_id'] = $ids['mailing_id'] = $this->_mailingID; if (empty($params['mailing_id'])) { - CRM_Core_Error::fatal(ts('Could not find a mailing id')); + CRM_Core_Error::statusBounce(ts('Could not find a mailing id')); } $params['send_option'] = $this->controller->exportValue($this->_name, 'send_option'); diff --git a/CRM/SMS/Provider.php b/CRM/SMS/Provider.php index 019317277d..18976e1848 100644 --- a/CRM/SMS/Provider.php +++ b/CRM/SMS/Provider.php @@ -32,6 +32,7 @@ abstract class CRM_SMS_Provider { * @param bool $force * * @return object + * @throws CRM_Core_Exception */ public static function &singleton($providerParams = array(), $force = FALSE) { $mailingID = CRM_Utils_Array::value('mailing_id', $providerParams); @@ -47,7 +48,7 @@ abstract class CRM_SMS_Provider { } if (!$providerName) { - CRM_Core_Error::fatal('Provider not known or not provided.'); + throw new CRM_Core_Exception('Provider not known or not provided.'); } $providerName = CRM_Utils_Type::escape($providerName, 'String'); @@ -62,7 +63,7 @@ abstract class CRM_SMS_Provider { else { // If we are running unit tests we simulate an SMS provider with the name "CiviTestSMSProvider" if ($providerName !== 'CiviTestSMSProvider') { - CRM_Core_Error::fatal("Could not locate extension for {$providerName}."); + throw new CRM_Core_Exception("Could not locate extension for {$providerName}."); } $providerClass = 'CiviTestSMSProvider'; } diff --git a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php index 6b84cd7a3b..17f9497ba1 100644 --- a/tests/phpunit/CRM/Activity/BAO/ActivityTest.php +++ b/tests/phpunit/CRM/Activity/BAO/ActivityTest.php @@ -1178,7 +1178,7 @@ $text $this->assertEquals($activity['status_id'], $activityStatusCompleted, 'Expected activity status Completed.'); $this->assertEquals($activity['subject'], 'createSendSmsTest subject', 'Activity subject does not match.'); $this->assertEquals($activity['details'], $details, 'Activity details does not match.'); - $this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0]->message, "Expected error doesn't match"); + $this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0], "Expected error doesn't match"); $this->assertEquals(0, $success, "Expected success to be 0"); } @@ -1193,7 +1193,7 @@ $text $this->assertEquals($activity['status_id'], $activityStatusCompleted, 'Expected activity status Completed.'); $this->assertEquals($activity['subject'], 'createSendSmsTest subject', 'Activity subject does not match.'); $this->assertEquals($activity['details'], $details, 'Activity details does not match.'); - $this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0]->message, "Expected error doesn't match"); + $this->assertEquals("Recipient phone number is invalid or recipient does not want to receive SMS", $sent[0], "Expected error doesn't match"); $this->assertEquals(0, $success, "Expected success to be 0"); } @@ -1229,7 +1229,7 @@ $text public function testSendSMSMobileInToProviderParamWithDoNotSMS() { list($sent, $activityId, $success) = $this->createSendSmsTest(2, TRUE, ['do_not_sms' => 1]); foreach ($sent as $error) { - $this->assertEquals('Contact Does not accept SMS', $error->getMessage()); + $this->assertEquals('Contact Does not accept SMS', $error); } $this->assertEquals(1, count($sent), "Expected sent should a PEAR Error"); $this->assertEquals(0, $success, "Expected success to be 0"); -- 2.25.1