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

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 3de7326172e1cb00d24f63881aad16b25a71f1ac..6f0823033ca083c50ce235b20a2c5b568c8aa606 100644 (file)
--- 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 391064c823f606e2df4014b2b39da6b3b9df7d46..e1fa68af31b61bc8293d58dbe01a0b7f1461d892 100644 (file)
--- 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 acc4eb5433ee77de98b54119281d5a4d0dcf16f5..f56f11cf56b9e3b4f023c2b71241ef13c99455d1 100644 (file)
--- 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]);