From 6ad8d40056d04b14ec6f94ab3684c4a6586821b7 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 | 17 +++++++-------- CRM/Core/Payment/Dummy.php | 1 + Civi/Payment/PropertyBag.php | 19 +++++++++++++++++ tests/phpunit/api/v3/PaymentProcessorTest.php | 21 +++++++++++++------ 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index 5ae135f13d37..1cfd8534f43d 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -1268,7 +1268,7 @@ public function extractCustomPropertiesForDoPayment(PropertyBag $propertyBag, ar * 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,24 @@ public function subscriptionURL($entityID = NULL, $entity = NULL, $action = 'can * @return string */ protected function getPaymentDescription($params = [], $length = 24) { + $propertyBag = PropertyBag::cast($params); $parts = [ 'contactID', 'contributionID', 'description', + 'isRecur', 'billing_first_name', 'billing_last_name', ]; - $validParts = []; - $params['description'] = $this->getDescription(); - foreach ($parts as $part) { - if ((!empty($params[$part]))) { - $validParts[] = $params[$part]; - } + foreach ($parts as $i => $prop) { + $parts[$i] = $propertyBag->getter($prop, TRUE, NULL); } - return substr(implode('-', $validParts), 0, $length); + $parts['is_recur'] = !empty($parts['is_recur']) ? ts('Recurring payment') : NULL; + 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 717c4f3c6fb8..b2b66125fbdd 100644 --- a/CRM/Core/Payment/Dummy.php +++ b/CRM/Core/Payment/Dummy.php @@ -128,6 +128,7 @@ public function doDirectPayment(&$params) { // 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 1419892d0a19..c27e0af4dc05 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 b707f6772029..e3c8a713d72f 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 @@ public function setUp() { } /** - * 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 @@ public function testPaymentProcessorCreate() { /** * Update payment processor. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorUpdate() { $params = $this->_params; @@ -115,6 +120,8 @@ public function testPaymentProcessorCreateExample() { /** * Check payment processor delete. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorDelete() { $result = $this->callAPISuccess('payment_processor', 'create', $this->_params); @@ -127,6 +134,8 @@ public function testPaymentProcessorDelete() { /** * Check with valid params array. + * + * @throws \CRM_Core_Exception */ public function testPaymentProcessorsGet() { $params = $this->_params;