From 3209a807f26543c3d1b0c1e5121bbab26afe3713 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 9 Dec 2019 16:21:35 +1300 Subject: [PATCH] Fix unreleased regression where processors using getPaymentDescription get a fatal We added the new property bag in https://github.com/civicrm/civicrm-core/pull/15697 but in the back & forth the call to the function getDescription was retained but the function was removed. This would leave the core processor that calls it (Paypal std) with a fatal. The known external processor using it (Omnipay) also hits a notice because the signature changed ($params became optional). From the Omnipay pov I think it makes sense just to stop overriding the function - hence I am sneaking a tiny change into this to make that possible. Note the doPayment function ensures that propertyBag is set. There is a small chance that an external processor is fully overriding doPayment AND calling this - that would be the case for Omnipay except I'll remove the override in conjunction with this - so I've added a few lines to ensure it is set. Note the test would fail with the changes to Payment_Dummmy & not the changes to Core_Payment so it provides test cover --- CRM/Core/Payment.php | 24 +++++++------------ CRM/Core/Payment/Dummy.php | 1 + Civi/Payment/PropertyBag.php | 19 +++++++++++++++ tests/phpunit/api/v3/PaymentProcessorTest.php | 21 +++++++++++----- 4 files changed, 44 insertions(+), 21 deletions(-) diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index 5ae135f13d..1af72a2301 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -1268,7 +1268,7 @@ abstract class CRM_Core_Payment { * For the current status see: https://lab.civicrm.org/dev/financial/issues/53 * If we DO have a contribution ID, then the payment processor can (and should) update parameters on the contribution record as necessary. * - * @param array $params + * @param array|PropertyBag $params * * @param string $component * @@ -1664,25 +1664,19 @@ INNER JOIN civicrm_contribution con ON ( con.contribution_recur_id = rec.id ) * @return string */ protected function getPaymentDescription($params = [], $length = 24) { + $propertyBag = PropertyBag::cast($params); $parts = [ - 'contactID', - 'contributionID', - 'description', - 'billing_first_name', - 'billing_last_name', + $propertyBag->getter('contactID', TRUE), + $propertyBag->getter('contributionID', TRUE), + $propertyBag->getter('description', TRUE) ?: ($propertyBag->getter('isRecur', TRUE) ? ts('Recurring payment') : NULL), + $propertyBag->getter('billing_first_name', TRUE), + $propertyBag->getter('billing_last_name', TRUE), ]; - $validParts = []; - $params['description'] = $this->getDescription(); - foreach ($parts as $part) { - if ((!empty($params[$part]))) { - $validParts[] = $params[$part]; - } - } - return substr(implode('-', $validParts), 0, $length); + return substr(implode('-', array_filter($parts)), 0, $length); } /** - * Checks if backoffice recurring edit is allowed + * Checks if back-office recurring edit is allowed * * @return bool */ diff --git a/CRM/Core/Payment/Dummy.php b/CRM/Core/Payment/Dummy.php index 717c4f3c6f..b2b66125fb 100644 --- a/CRM/Core/Payment/Dummy.php +++ b/CRM/Core/Payment/Dummy.php @@ -128,6 +128,7 @@ class CRM_Core_Payment_Dummy extends CRM_Core_Payment { // Add a fee_amount so we can make sure fees are handled properly in underlying classes. $params['fee_amount'] = 1.50; $params['net_amount'] = $params['gross_amount'] - $params['fee_amount']; + $params['description'] = $this->getPaymentDescription($params); return $params; } diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php index 1419892d0a..c27e0af4dc 100644 --- a/Civi/Payment/PropertyBag.php +++ b/Civi/Payment/PropertyBag.php @@ -67,6 +67,25 @@ class PropertyBag implements \ArrayAccess { 'trxnResultCode' => TRUE, ]; + /** + * Get the property bag. + * + * This allows us to swap a 'might be an array might be a property bag' + * variable for a known PropertyBag. + * + * @param \Civi\Payment\PropertyBag|array $params + * + * @return \Civi\Payment\PropertyBag + */ + public static function cast($params) { + if ($params instanceof self) { + return $params; + } + $propertyBag = new self(); + $propertyBag->mergeLegacyInputParams($params); + return $propertyBag; + } + /** * @var string Just for unit testing. */ diff --git a/tests/phpunit/api/v3/PaymentProcessorTest.php b/tests/phpunit/api/v3/PaymentProcessorTest.php index b707f67720..e3c8a713d7 100644 --- a/tests/phpunit/api/v3/PaymentProcessorTest.php +++ b/tests/phpunit/api/v3/PaymentProcessorTest.php @@ -16,9 +16,13 @@ */ class api_v3_PaymentProcessorTest extends CiviUnitTestCase { protected $_paymentProcessorType; - protected $_apiversion = 3; protected $_params; + /** + * Set up class. + * + * @throws \CRM_Core_Exception + */ public function setUp() { parent::setUp(); $this->useTransaction(TRUE); @@ -42,17 +46,16 @@ class api_v3_PaymentProcessorTest extends CiviUnitTestCase { } /** - * Check with no name. + * Check create with no name specified. */ public function testPaymentProcessorCreateWithoutName() { - $payProcParams = [ - 'is_active' => 1, - ]; - $this->callAPIFailure('payment_processor', 'create', $payProcParams); + $this->callAPIFailure('payment_processor', 'create', ['is_active' => 1]); } /** * Create payment processor. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorCreate() { $params = $this->_params; @@ -72,6 +75,8 @@ class api_v3_PaymentProcessorTest extends CiviUnitTestCase { /** * Update payment processor. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorUpdate() { $params = $this->_params; @@ -115,6 +120,8 @@ class api_v3_PaymentProcessorTest extends CiviUnitTestCase { /** * Check payment processor delete. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorDelete() { $result = $this->callAPISuccess('payment_processor', 'create', $this->_params); @@ -127,6 +134,8 @@ class api_v3_PaymentProcessorTest extends CiviUnitTestCase { /** * Check with valid params array. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorsGet() { $params = $this->_params; -- 2.25.1