Skip to content

Commit

Permalink
Fix unreleased regression where processors using getPaymentDescriptio…
Browse files Browse the repository at this point in the history
…n get a fatal

We added the new property bag in civicrm#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
  • Loading branch information
eileenmcnaughton committed Dec 9, 2019
1 parent a1552cd commit da4d2c3
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 9 deletions.
29 changes: 26 additions & 3 deletions CRM/Core/Payment.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand All @@ -1278,6 +1278,7 @@ public function extractCustomPropertiesForDoPayment(PropertyBag $propertyBag, ar
* @throws \Civi\Payment\Exception\PaymentProcessorException
*/
public function doPayment(&$params, $component = 'contribute') {
$this->setPropertyBag($params);
$this->_component = $component;
$statuses = CRM_Contribute_BAO_Contribution::buildOptions('contribution_status_id', 'validate');

Expand Down Expand Up @@ -1664,6 +1665,10 @@ public function subscriptionURL($entityID = NULL, $entity = NULL, $action = 'can
* @return string
*/
protected function getPaymentDescription($params = [], $length = 24) {
if (!$this->propertyBag) {
CRM_Core_Error::deprecatedFunctionWarning('The property bag should be early on - e.g by doPayment or the calling function');
$this->setPropertyBag($params);
}
$parts = [
'contactID',
'contributionID',
Expand All @@ -1672,17 +1677,20 @@ protected function getPaymentDescription($params = [], $length = 24) {
'billing_last_name',
];
$validParts = [];
$params['description'] = $this->getDescription();
$params['description'] = $this->propertyBag->getDescription();
foreach ($parts as $part) {
if ((!empty($params[$part]))) {
$validParts[] = $params[$part];
}
}
if (empty($validParts) && !empty($params['is_recur'])) {
$validParts[] = ts('Regular payment');
}
return substr(implode('-', $validParts), 0, $length);
}

/**
* Checks if backoffice recurring edit is allowed
* Checks if back-office recurring edit is allowed
*
* @return bool
*/
Expand Down Expand Up @@ -1734,4 +1742,19 @@ public function isSendReceiptForPending() {
return FALSE;
}

/**
* Ensure the property bag property is set.
*
* @param \Civi\Payment\PropertyBag|array $params
*/
protected function setPropertyBag(&$params) {
if (is_a($params, 'PropertyBag')) {
$this->propertyBag = $params;
}
else {
$this->propertyBag = new PropertyBag();
$this->propertyBag->mergeLegacyInputParams($params);
}
}

}
1 change: 1 addition & 0 deletions CRM/Core/Payment/Dummy.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
21 changes: 15 additions & 6 deletions tests/phpunit/api/v3/PaymentProcessorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -72,6 +75,8 @@ public function testPaymentProcessorCreate() {

/**
* Update payment processor.
*
* @throws \CRM_Core_Exception
*/
public function testPaymentProcessorUpdate() {
$params = $this->_params;
Expand Down Expand Up @@ -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);
Expand All @@ -127,6 +134,8 @@ public function testPaymentProcessorDelete() {

/**
* Check with valid params array.
*
* @throws \CRM_Core_Exception
*/
public function testPaymentProcessorsGet() {
$params = $this->_params;
Expand Down

0 comments on commit da4d2c3

Please sign in to comment.