From 49ed48886918c4022e90dceaf67f2b342e5552e3 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 7 Sep 2020 11:14:42 +1200 Subject: [PATCH] Remove error handling from loadObjects The processors now have their own try->catch so we don't need to catch here. I wasn't sure about the debug_log_message part so I copied it into the processors , at risk of duplicating --- CRM/Contribute/BAO/Contribution.php | 8 +- CRM/Core/Payment/BaseIPN.php | 36 +----- .../phpunit/CRM/Core/Payment/BaseIPNTest.php | 104 +++++++++--------- 3 files changed, 64 insertions(+), 84 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 3de7326172..6f0823033c 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -2773,7 +2773,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac * Load all related objects - even where id not passed in? (allows API to call this). * * @return bool - * @throws Exception + * @throws CRM_Core_Exception */ public function loadRelatedObjects($input, &$ids, $loadAll = FALSE) { // @todo deprecate this function - the steps should be @@ -2840,7 +2840,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac $payment = new CRM_Pledge_BAO_PledgePayment(); $payment->id = $paymentID; if (!$payment->find(TRUE)) { - throw new Exception("Could not find pledge payment record: " . $paymentID); + throw new CRM_Core_Exception("Could not find pledge payment record: " . $paymentID); } $this->_relatedObjects['pledge_payment'][] = $payment; } @@ -2858,7 +2858,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac if ($ids['event'] && !$event->find(TRUE) ) { - throw new Exception("Could not find event: " . $ids['event']); + throw new CRM_Core_Exception("Could not find event: " . $ids['event']); } $this->_relatedObjects['event'] = &$event; @@ -2868,7 +2868,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac if ($ids['participant'] && !$participant->find(TRUE) ) { - throw new Exception("Could not find participant: " . $ids['participant']); + throw new CRM_Core_Exception("Could not find participant: " . $ids['participant']); } $participant->register_date = CRM_Utils_Date::isoToMysql($participant->register_date); diff --git a/CRM/Core/Payment/BaseIPN.php b/CRM/Core/Payment/BaseIPN.php index 391064c823..e1fa68af31 100644 --- a/CRM/Core/Payment/BaseIPN.php +++ b/CRM/Core/Payment/BaseIPN.php @@ -143,42 +143,16 @@ class CRM_Core_Payment_BaseIPN { * @param array $objects * @param bool $required * @param int $paymentProcessorID - * @param array $error_handling * * @return bool|array + * @throws \CRM_Core_Exception */ - public function loadObjects($input, &$ids, &$objects, $required, $paymentProcessorID, $error_handling = NULL) { - if (empty($error_handling)) { - // default options are that we log an error & echo it out - // note that we should refactor this error handling into error code @ some point - // but for now setting up enough separation so we can do unit tests - $error_handling = [ - 'log_error' => 1, - 'echo_error' => 1, - ]; - } + public function loadObjects($input, &$ids, &$objects, $required, $paymentProcessorID) { $contribution = &$objects['contribution']; $ids['paymentProcessor'] = $paymentProcessorID; - try { - $success = $contribution->loadRelatedObjects($input, $ids); - if ($required && empty($contribution->_relatedObjects['paymentProcessor'])) { - throw new CRM_Core_Exception("Could not find payment processor for contribution record: " . $contribution->id); - } - } - catch (Exception $e) { - $success = FALSE; - if (!empty($error_handling['log_error'])) { - CRM_Core_Error::debug_log_message($e->getMessage()); - } - if (!empty($error_handling['echo_error'])) { - echo $e->getMessage(); - } - if (!empty($error_handling['return_error'])) { - return [ - 'is_error' => 1, - 'error_message' => ($e->getMessage()), - ]; - } + $success = $contribution->loadRelatedObjects($input, $ids); + if ($required && empty($contribution->_relatedObjects['paymentProcessor'])) { + throw new CRM_Core_Exception("Could not find payment processor for contribution record: " . $contribution->id); } $objects = array_merge($objects, $contribution->_relatedObjects); return $success; diff --git a/tests/phpunit/CRM/Core/Payment/BaseIPNTest.php b/tests/phpunit/CRM/Core/Payment/BaseIPNTest.php index acc4eb5433..f56f11cf56 100644 --- a/tests/phpunit/CRM/Core/Payment/BaseIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/BaseIPNTest.php @@ -148,8 +148,8 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { $this->assertArrayHasKey($this->_membershipTypeID, $contribution->_relatedObjects['membership']); $this->assertTrue(is_a($contribution->_relatedObjects['membership'][$this->_membershipTypeID], 'CRM_Member_BAO_Membership')); $this->assertTrue(is_a($contribution->_relatedObjects['financialType'], 'CRM_Financial_BAO_FinancialType')); - $this->assertFalse(empty($contribution->_relatedObjects['contributionRecur'])); - $this->assertFalse(empty($contribution->_relatedObjects['paymentProcessor'])); + $this->assertNotEmpty($contribution->_relatedObjects['contributionRecur']); + $this->assertNotEmpty($contribution->_relatedObjects['paymentProcessor']); } /** @@ -215,10 +215,10 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { public function testLoadParticipantObjects() { $this->_setUpParticipantObjects(); $this->IPN->loadObjects($this->input, $this->ids, $this->objects, FALSE, $this->_processorId); - $this->assertFalse(empty($this->objects['participant'])); + $this->assertNotEmpty($this->objects['participant']); $this->assertTrue(is_a($this->objects['participant'], 'CRM_Event_BAO_Participant')); $this->assertTrue(is_a($this->objects['financialType'], 'CRM_Financial_BAO_FinancialType')); - $this->assertFalse(empty($this->objects['event'])); + $this->assertNotEmpty($this->objects['event']); $this->assertTrue(is_a($this->objects['event'], 'CRM_Event_BAO_Event')); $this->assertTrue(is_a($this->objects['contribution'], 'CRM_Contribute_BAO_Contribution')); $this->assertNotEmpty($this->objects['event']->id); @@ -280,11 +280,11 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { $tablesToTruncate = [ 'civicrm_mailing_spool', ]; - $this->quickCleanup($tablesToTruncate, FALSE); + $this->quickCleanup($tablesToTruncate); $mut = new CiviMailUtils($this, TRUE); $contribution = new CRM_Contribute_BAO_Contribution(); $contribution->id = $this->_contributionId; - $msg = $contribution->composeMessageArray($this->input, $this->ids, $values); + $contribution->composeMessageArray($this->input, $this->ids, $values); $mut->assertMailLogEmpty('no mail should have been send as event set to no confirm'); $mut->stop(); } @@ -295,11 +295,11 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { public function testLoadPledgeObjects() { $this->_setUpPledgeObjects(); $this->IPN->loadObjects($this->input, $this->ids, $this->objects, FALSE, $this->_processorId); - $this->assertFalse(empty($this->objects['pledge_payment'][0])); + $this->assertNotEmpty($this->objects['pledge_payment'][0]); $this->assertTrue(is_a($this->objects['financialType'], 'CRM_Financial_BAO_FinancialType')); $this->assertTrue(is_a($this->objects['contribution'], 'CRM_Contribute_BAO_Contribution')); $this->assertTrue(is_a($this->objects['pledge_payment'][0], 'CRM_Pledge_BAO_PledgePayment')); - $this->assertFalse(empty($this->objects['pledge_payment'][0]->id)); + $this->assertNotEmpty($this->objects['pledge_payment'][0]->id); $this->assertEquals($this->_financialTypeId, $this->objects['financialType']->id); $this->assertEquals($this->_processorId, $this->objects['paymentProcessor']['id']); $this->assertEquals($this->_contributionId, $this->objects['contribution']->id); @@ -313,25 +313,41 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { public function testLoadPledgeObjectsInvalidPledgeID() { $this->_setUpPledgeObjects(); $this->ids['pledge_payment'][0] = 0; - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1]); - $this->assertArrayHasKey('error_message', $result); + try { + $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL); + } + catch (CRM_Core_Exception $e) { + $this->assertEquals('Could not find payment processor for contribution record: 1', $e->getMessage()); + } $this->assertArrayNotHasKey('pledge_payment', $this->objects); - $this->assertEquals('Could not find payment processor for contribution record: 1', $result['error_message']); + $this->ids['pledge_payment'][0] = NULL; - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1]); - $this->assertArrayHasKey('error_message', $result); + try { + $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL); + } + catch (CRM_Core_Exception $e) { + $this->assertEquals('Could not find payment processor for contribution record: 1', $e->getMessage()); + } $this->assertArrayNotHasKey('pledge_payment', $this->objects); - $this->assertEquals('Could not find payment processor for contribution record: 1', $result['error_message']); + $this->ids['pledge_payment'][0] = ''; - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1]); - $this->assertArrayHasKey('error_message', $result); + + try { + $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL); + $this->assertArrayHasKey('error_message', $result); + } + catch (CRM_Core_Exception $e) { + $this->assertEquals('Could not find payment processor for contribution record: 1', $e->getMessage()); + } $this->assertArrayNotHasKey('pledge_payment', $this->objects); - $this->assertEquals('Could not find payment processor for contribution record: 1', $result['error_message']); $this->ids['pledge_payment'][0] = 999; - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, $this->_processorId, ['return_error' => 1]); - $this->assertArrayHasKey('error_message', $result); - $this->assertEquals('Could not find pledge payment record: 999', $result['error_message']); + try { + $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, $this->_processorId); + } + catch (CRM_Core_Exception $e) { + $this->assertEquals('Could not find pledge payment record: 999', $e->getMessage()); + } } /** @@ -350,25 +366,15 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { */ public function testRequiredWithoutProcessorID() { $this->_setUpPledgeObjects(); - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1]); - $this->assertArrayHasKey('error_message', $result); - $this->assertEquals('Could not find payment processor for contribution record: 1', $result['error_message']); + try { + $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL); + } + catch (CRM_Core_Exception $e) { + $this->assertEquals('Could not find payment processor for contribution record: 1', $e->getMessage()); + } // error is only returned if $required set to True - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, FALSE, NULL, ['return_error' => 1]); - $this->assertFalse(is_array($result)); - //check that error is not returned if error checking not set - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['log_error' => 1]); - $this->assertFalse(is_array($result)); - } - - /** - * Test that an error is not if required set & no processor ID - */ - public function testRequiredWithContributionPage() { - $this->_setUpContributionObjects(TRUE); - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1]); - $this->assertEquals(1, $result['is_error']); - ; + $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, FALSE, NULL); + $this->assertEquals(TRUE, $result); } /** @@ -379,7 +385,7 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { public function testPaymentProcessorLoadsAsParam() { $this->_setUpContributionObjects(); $this->input = array_merge($this->input, ['payment_processor_id' => $this->_processorId]); - $this->assertTrue($this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1])); + $this->assertTrue($this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL)); } /** @@ -387,15 +393,14 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { */ public function testRequiredWithContributionPageError() { $this->_setUpContributionObjects(); - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['return_error' => 1]); - $this->assertArrayHasKey('error_message', $result); - $this->assertEquals('Could not find payment processor for contribution record: 1', $result['error_message']); + try { + $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL); + } + catch (CRM_Core_Exception $e) { + $this->assertEquals('Could not find payment processor for contribution record: 1', $e->getMessage()); + } // error is only returned if $required set to True - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, FALSE, NULL, ['return_error' => 1]); - $this->assertFalse(is_array($result)); - //check that error is not returned if error checking not set - $result = $this->IPN->loadObjects($this->input, $this->ids, $this->objects, TRUE, NULL, ['log_error' => 1]); - $this->assertFalse(is_array($result)); + $this->IPN->loadObjects($this->input, $this->ids, $this->objects, FALSE, NULL); } public function testThatCancellingEventPaymentWillCancelAllAdditionalPendingParticipantsAndCreateCancellationActivities() { @@ -469,8 +474,7 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { $contributionPageID = NULL; if (!empty($contributionPage)) { - $dao = new CRM_Core_DAO(); - $contribution_page = $dao->createTestObject('CRM_Contribute_DAO_ContributionPage'); + $contribution_page = CRM_Core_DAO::createTestObject('CRM_Contribute_DAO_ContributionPage'); $contribution_page->payment_processor = 1; $contribution_page->save(); $contribution->contribution_page_id = $contributionPageID = $contribution_page->id; @@ -577,6 +581,8 @@ class CRM_Core_Payment_BaseIPNTest extends CiviUnitTestCase { * * @param string $participantStatus * The participant to create status + * + * @throws \CRM_Core_Exception */ public function _setUpParticipantObjects($participantStatus = 'Attended') { $event = $this->eventCreate(['is_email_confirm' => 1]); -- 2.25.1