From 05bf1fb0fa5150dcc1c0f50591effcbb30b5147a Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 2 Nov 2019 17:14:19 +1300 Subject: [PATCH 1/5] Revert "Add a bunch of additional getters & setters" This reverts commit c5884dfddb30e40e06af9ef0f2dc940ad6ae55bd. --- CRM/Core/Payment.php | 371 ++----------------------------------------- 1 file changed, 16 insertions(+), 355 deletions(-) diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index 089dffc8221..cb2e2f77cf7 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -81,7 +81,7 @@ abstract class CRM_Core_Payment { RECURRING_PAYMENT_END = 'END'; /** - * @var array + * @var object */ protected $_paymentProcessor; @@ -156,125 +156,6 @@ abstract class CRM_Core_Payment { */ protected $contributionID; - /** - * Contact ID for payment. - * - * @var int - */ - protected $contactID; - - /** - * Recurring contribution that is being paid. - * - * @var int - */ - protected $contributionRecurID; - - /** - * Description of purchase. - * - * @var string - */ - protected $description; - - /** - * CiviCRM generated Invoice ID. - * - * @var string - */ - protected $invoiceID; - - /** - * Is this a recurring contribution. - * - * @var bool - */ - protected $isRecur; - - /** - * Payment processor generated string for charging. - * - * A payment token could be a single use token (e.g generated by - * a client side script) or a token that permits recurring or on demand charging. - * - * The key thing is it is passed to the processor in lieu of card details to - * initiate a payment. - * - * Generally if a processor is going to pass in a payment token generated through - * javascript it would add 'payment_token' to the array it returns in it's - * implementation of getPaymentFormFields. This will add a hidden 'payment_token' field to - * the form. A good example is client side encryption where credit card details are replaced by - * an encrypted token using a gateway provided javascript script. In this case the javascript will - * remove the credit card details from the form before submitting and populate the payment_token field. - * - * A more complex example is used by paypal checkout where the payment token is generated - * via a pre-approval process. In that case the doPreApproval function is called on the processor - * class to get information to then interact with paypal via js, finally getting a payment token. - * (at this stage the pre-approve api is not in core but that is likely to change - we just need - * to think about the permissions since we don't want to expose to anonymous user without thinking - * through any risk of credit-card testing using it. - * - * If the token is not totally transient it would be saved to civicrm_payment_token.token. - * - * @var string - */ - protected $paymentToken; - - /** - * Three digit currency code. - * - * @var string - */ - protected $currency; - - /** - * Payment processor generated string for the transaction ID. - * - * Note some gateways generate a reference for the order and one for the - * payment. This is for the payment reference and is saved to - * civicrm_financial_trxn.trxn_id. - * - * @var string - */ - protected $transactionID; - - /** - * Amount of money charged in fees by the payment processor. - * - * This is notified by (some) payment processers. - * - * @var float - */ - protected $feeAmount; - - /** - * Additional information returned by the payment processor regarding the payment outcome. - * - * This would normally be saved in civicrm_financial_trxn.trxn_result_code. - * - * @var string - */ - protected $trxnResultCode; - - /** - * Combined with recurFrequencyUnit this gives how often billing will take place. - * - * e.g every if this is 1 and recurFrequencyUnit is 'month' then it is every 1 month. - * - * @var int - */ - protected $recurFrequencyInterval; - - /** - * Combined with recurFrequencyInterval this gives how often billing will take place. - * - * e.g every if this is 'month' and recurFrequencyInterval is 1 then it is every 1 month. - * - * @var string - * month|day|year - */ - protected $recurFrequencyUnit; - /** * Passed in parameters. * @@ -285,227 +166,6 @@ abstract class CRM_Core_Payment { */ protected $inputParams = []; - /** - * @return int - */ - public function getContactID(): int { - return $this->contactID ?? $this->inputParams['contactID']; - } - - /** - * @param int $contactID - */ - public function setContactID(int $contactID) { - $this->contactID = $contactID; - } - - /** - * Getter for contributionRecurID. - * - * Ideally this would always be set by the calling function using the setting - * but we need to fall back to the historical passing on inputParams as a transition. - * - * In time we can add a notice if it's set in input params & not via a setter. - * - * @return int - */ - public function getContributionRecurID(): int { - return $this->contributionRecurID ?? $this->inputParams['contributionRecurID']; - } - - /** - * @param int $contributionRecurID - */ - public function setContributionRecurID(int $contributionRecurID) { - $this->contributionRecurID = $contributionRecurID; - } - - /** - * Getter for Payment Token. - * - * @return string - */ - public function getPaymentToken(): string { - return $this->paymentToken ?? $this->inputParams['token']; - } - - /** - * Setter for Payment Token. - * - * @param string $paymentToken - */ - public function setPaymentToken(string $paymentToken) { - $this->paymentToken = $paymentToken; - } - - /** - * Get gateway generated transaction ID. - * - * @return string - */ - public function getTransactionID(): string { - return $this->transactionID; - } - - /** - * Set gateway generated transaction ID for the payment. - * - * Note some gateways generate a reference for the order and one for the - * payment. This is for the payment reference and is saved to - * civicrm_financial_trxn.trxn_id. - * - * @param string $transactionID - */ - public function setTransactionID(string $transactionID) { - $this->transactionID = $transactionID; - } - - /** - * Get additional result information received from gateway. - * - * This is saved to civicrm_financial_trxn.trxn_result_code. - * - * @return string - */ - public function getTrxnResultCode(): string { - return $this->trxnResultCode; - } - - /** - * Set additional result information from gateway. - * - * This is saved to civicrm_financial_trxn.trxn_result_code. - * - * @param string $resultCode - */ - public function setTrxnResultCode(string $resultCode) { - $this->trxnResultCode = $resultCode; - } - - /** - * Get recurring frequency interval. - * - * @return int - */ - public function getRecurFrequencyInterval(): int { - return $this->recurFrequencyInterval ?? $this->inputParams['frequency_interval']; - } - - /** - * Set recurring frequency interval. - * - * @param int $recurFrequencyInterval - */ - public function setRecurFrequencyInterval(int $recurFrequencyInterval) { - $this->recurFrequencyInterval = $recurFrequencyInterval; - } - - /** - * Get recurring frequency unit. - * - * @return string - */ - public function getRecurFrequencyUnit(): string { - return $this->recurFrequencyUnit; - } - - /** - * Set recurring frequency unit. - * - * @param string $recurFrequencyUnit - */ - public function setRecurFrequencyUnit(string $recurFrequencyUnit) { - $this->recurFrequencyUnit = $recurFrequencyUnit ?? $this->inputParams['frequency_unit']; - } - - /** - * Get invoice ID (CiviCRM generated invoice reference). - * - * @return string - */ - public function getInvoiceID(): string { - return $this->invoiceID ?? $this->inputParams['invoiceID']; - } - - /** - * Set invoice ID (CiviCRM generated invoice reference). - * - * @param string $invoiceID - */ - public function setInvoiceID(string $invoiceID) { - $this->invoiceID = $invoiceID; - } - - /** - * Get whether the payment is recurring. - * - * @return bool - */ - public function isRecur(): bool { - return $this->isRecur; - } - - /** - * Set whether the payment is recurring. - * - * @param bool $isRecur - */ - public function setIsRecur(bool $isRecur) { - $this->isRecur = $isRecur ?? $this->inputParams['is_recur']; - } - - /** - * Get the description. - * - * This generates a description string from params if one has been passed in but - * ideally calling functions would use the setDescription function. - * - * @return string - */ - public function getDescription(): string { - if ($this->description) { - return $this->description; - } - if (isset($this->inputParams['description'])) { - $uninformativeStrings = [ - ts('Online Event Registration: '), - ts('Online Contribution: '), - ]; - $this->description = str_replace($uninformativeStrings, '', $this->inputParams['description']); - } - return $this->description; - } - - /** - * Set description. - * - * Generally this should be called when instantiating the processor to override - * getPaymentDescription, if desired. - * - * @param string $description - */ - public function setDescription(string $description) { - $this->description = $description; - } - - /** - * Get fee amount returned by processor. - * - * @return float - */ - public function getFeeAmount(): float { - return $this->feeAmount; - } - - /** - * Set fee amount returned by processor. - * - * @param float $feeAmount - */ - public function setFeeAmount(float $feeAmount) { - $this->feeAmount = $feeAmount; - } - /** * Get the contribution ID. * @@ -632,6 +292,8 @@ public function buildForm(&$form) { * @todo move to factory class \Civi\Payment\System (or similar) * * @param array $params + * + * @return mixed */ public static function logPaymentNotification($params) { $message = 'payment_notification '; @@ -1060,8 +722,6 @@ protected function getMandatoryFields() { * Get the metadata of all the fields configured for this processor. * * @return array - * - * @throws \CiviCRM_API3_Exception */ protected function getAllFields() { $paymentFields = array_intersect_key($this->getPaymentFormFieldsMetadata(), array_flip($this->getPaymentFormFields())); @@ -1104,8 +764,6 @@ protected function getDirectDebitFormFields() { * * @return array * field metadata - * - * @throws \Exception */ public function getPaymentFormFieldsMetadata() { //@todo convert credit card type into an option value @@ -1445,9 +1103,8 @@ protected function getBaseReturnUrl() { * * @return string */ - protected function getCurrency($params = []) { - $params = array_merge($params, (array) $this->inputParams); - return $this->currency ?? CRM_Utils_Array::value('currencyID', $params, CRM_Utils_Array::value('currency', $params)); + protected function getCurrency($params) { + return CRM_Utils_Array::value('currencyID', $params, CRM_Utils_Array::value('currency', $params)); } /** @@ -1455,14 +1112,12 @@ protected function getCurrency($params = []) { * * Handle any inconsistency about how it is passed in here. * - * @param array $params + * @param $params * * @return string - * @throws \CRM_Core_Exception */ - protected function getAmount($params = []) { - $amount = $this->amount ?? $params['amount']; - return CRM_Utils_Money::format($amount, NULL, NULL, TRUE); + protected function getAmount($params) { + return CRM_Utils_Money::format($params['amount'], NULL, NULL, TRUE); } /** @@ -2010,7 +1665,7 @@ public function subscriptionURL($entityID = NULL, $entity = NULL, $action = 'can * * @return string */ - protected function getPaymentDescription($params = [], $length = 24) { + protected function getPaymentDescription($params, $length = 24) { $parts = [ 'contactID', 'contributionID', @@ -2019,7 +1674,13 @@ protected function getPaymentDescription($params = [], $length = 24) { 'billing_last_name', ]; $validParts = []; - $params['description'] = $this->getDescription(); + if (isset($params['description'])) { + $uninformativeStrings = [ + ts('Online Event Registration: '), + ts('Online Contribution: '), + ]; + $params['description'] = str_replace($uninformativeStrings, '', $params['description']); + } foreach ($parts as $part) { if ((!empty($params[$part]))) { $validParts[] = $params[$part]; From 2e75b2448d978343381e4dec38e7570149b41525 Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 2 Nov 2019 17:15:01 +1300 Subject: [PATCH 2/5] Revert "Add getters & setters for contributionID on CRM_Core_Payment." This reverts commit c28ad54a7194e30033ffed1ad727586546e80db3. --- CRM/Core/Payment.php | 36 --------------------------------- CRM/Core/Payment/PayPalImpl.php | 4 ---- 2 files changed, 40 deletions(-) diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index cb2e2f77cf7..69b97a342fb 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -149,41 +149,6 @@ abstract class CRM_Core_Payment { */ protected $backOffice = FALSE; - /** - * Contribution that is being paid. - * - * @var int - */ - protected $contributionID; - - /** - * Passed in parameters. - * - * Using individual getters & setters is preferred but these will be used if - * they are not available. - * - * @var array - */ - protected $inputParams = []; - - /** - * Get the contribution ID. - * - * We prefer the one set by the setter but traditional forms just pass in 'contributionID'. - * - * @return int - */ - public function getContributionID(): int { - return $this->contributionID ?? $this->inputParams['contributionID']; - } - - /** - * @param int $contributionID - */ - public function setContributionID(int $contributionID) { - $this->contributionID = $contributionID; - } - /** * @return bool */ @@ -1279,7 +1244,6 @@ protected function doDirectPayment(&$params) { * @throws \Civi\Payment\Exception\PaymentProcessorException */ public function doPayment(&$params, $component = 'contribute') { - $this->inputParams = $params; $this->_component = $component; $statuses = CRM_Contribute_BAO_Contribution::buildOptions('contribution_status_id', 'validate'); diff --git a/CRM/Core/Payment/PayPalImpl.php b/CRM/Core/Payment/PayPalImpl.php index 04ee47e496f..b2d090a4d99 100644 --- a/CRM/Core/Payment/PayPalImpl.php +++ b/CRM/Core/Payment/PayPalImpl.php @@ -432,7 +432,6 @@ public function createRecurringPayments(&$params) { "&m=" . "&c={$params['contactID']}" . "&r={$params['contributionRecurID']}" . - // @todo use $this->getContributionID(); "&b={$params['contributionID']}" . "&p={$params['contributionPageID']}"; @@ -570,7 +569,6 @@ public function doDirectPayment(&$params, $component = 'contribute') { $args['PROFILEREFERENCE'] = "" . "i=" . $params['invoiceID'] . "&m=" . $component . "&c=" . $params['contactID'] . "&r=" . $params['contributionRecurID'] . - // @todo use $this->getContributionID(); "&b=" . $params['contributionID'] . "&p=" . $params['contributionPageID']; } @@ -882,7 +880,6 @@ public function doTransferCheckout(&$params, $component = 'contribute') { $notifyParameters = ['module' => $component]; $notifyParameterMap = [ 'contactID' => 'contactID', - // @todo use $this->getContributionID(); 'contributionID' => 'contributionID', 'eventID' => 'eventID', 'participantID' => 'participantID', @@ -906,7 +903,6 @@ public function doTransferCheckout(&$params, $component = 'contribute') { $cancelUrlString = "$cancel=1&cancel=1&qfKey={$params['qfKey']}"; if (!empty($params['is_recur'])) { - // @todo use $this->getContributionID(); $cancelUrlString .= "&isRecur=1&recurId={$params['contributionRecurID']}&contribId={$params['contributionID']}"; } From 3fe06aa5f4d98f720c452c6e5de07b955b19221b Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 2 Nov 2019 17:16:33 +1300 Subject: [PATCH 3/5] Revert places where setters are used to set things These are all new in 5.20 & if we are going to change we should reverse & re-do before cutting 5.20 --- CRM/Contribute/Form/CancelSubscription.php | 1 - api/v3/PaymentProcessor.php | 9 --------- 2 files changed, 10 deletions(-) diff --git a/CRM/Contribute/Form/CancelSubscription.php b/CRM/Contribute/Form/CancelSubscription.php index 75ae5a1c05e..ef9dbe32ca3 100644 --- a/CRM/Contribute/Form/CancelSubscription.php +++ b/CRM/Contribute/Form/CancelSubscription.php @@ -220,7 +220,6 @@ public function postProcess() { if (CRM_Utils_Array::value('send_cancel_request', $params) == 1) { $cancelParams = ['subscriptionId' => $this->_subscriptionDetails->subscription_id]; - $this->_paymentProcessorObj->setContributionRecurID($this->contributionRecurID); $cancelSubscription = $this->_paymentProcessorObj->cancelSubscription($message, $cancelParams); } diff --git a/api/v3/PaymentProcessor.php b/api/v3/PaymentProcessor.php index 8373cc7ec7c..a4147179704 100644 --- a/api/v3/PaymentProcessor.php +++ b/api/v3/PaymentProcessor.php @@ -131,15 +131,6 @@ function civicrm_api3_payment_processor_pay($params) { $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']); $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $params['payment_processor_id']])); try { - $processor->setContributionID($params['contribution_id']); - $processor->setInvoiceID($params['invoice_id'] ?? ''); - if (!empty($params['contact_id'])) { - $processor->setContactID((int) $params['contact_id']); - } - if (!empty($params['contribution_recur_id'])) { - $processor->setContributionRecurID((int) $params['contribution_recur_id']); - } - $result = $processor->doPayment($params); } catch (\Civi\Payment\Exception\PaymentProcessorException $e) { From 189e6408db997688e3dfa83db4c4db2e363c62e1 Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 2 Nov 2019 19:06:12 +1300 Subject: [PATCH 4/5] Revert "dev/financial#77 ++ Make contribution_id mandatory for PaymentProcessor.pay, pass incoieID" This reverts commit 783b62a7dfe3c94bd69bcb182732b240e53911be. --- api/v3/PaymentProcessor.php | 6 +-- tests/phpunit/api/v3/PaymentProcessorTest.php | 42 ------------------- 2 files changed, 2 insertions(+), 46 deletions(-) diff --git a/api/v3/PaymentProcessor.php b/api/v3/PaymentProcessor.php index a4147179704..e2406dc5d0a 100644 --- a/api/v3/PaymentProcessor.php +++ b/api/v3/PaymentProcessor.php @@ -124,10 +124,8 @@ function _civicrm_api3_payment_processor_getlist_defaults(&$request) { * API result array. * * @throws \API_Exception - * @throws \CiviCRM_API3_Exception */ function civicrm_api3_payment_processor_pay($params) { - /* @var CRM_Core_Payment $processor */ $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']); $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $params['payment_processor_id']])); try { @@ -141,7 +139,7 @@ function civicrm_api3_payment_processor_pay($params) { } throw new API_Exception('Payment failed', $code, $errorData, $e); } - return civicrm_api3_create_success([$result], $params); + return civicrm_api3_create_success(array($result), $params); } /** @@ -151,7 +149,7 @@ function civicrm_api3_payment_processor_pay($params) { */ function _civicrm_api3_payment_processor_pay_spec(&$params) { $params['payment_processor_id'] = [ - 'api.required' => TRUE, + 'api.required' => 1, 'title' => ts('Payment processor'), 'type' => CRM_Utils_Type::T_INT, ]; diff --git a/tests/phpunit/api/v3/PaymentProcessorTest.php b/tests/phpunit/api/v3/PaymentProcessorTest.php index 6c54af7867b..60ccd4e5cea 100644 --- a/tests/phpunit/api/v3/PaymentProcessorTest.php +++ b/tests/phpunit/api/v3/PaymentProcessorTest.php @@ -35,11 +35,6 @@ class api_v3_PaymentProcessorTest extends CiviUnitTestCase { protected $_apiversion = 3; protected $_params; - /** - * Set up for class. - * - * @throws \CRM_Core_Exception - */ public function setUp() { parent::setUp(); $this->useTransaction(TRUE); @@ -74,8 +69,6 @@ public function testPaymentProcessorCreateWithoutName() { /** * Create payment processor. - * - * @throws \Exception */ public function testPaymentProcessorCreate() { $params = $this->_params; @@ -95,8 +88,6 @@ public function testPaymentProcessorCreate() { /** * Update payment processor. - * - * @throws \CRM_Core_Exception */ public function testPaymentProcessorUpdate() { $params = $this->_params; @@ -140,8 +131,6 @@ public function testPaymentProcessorCreateExample() { /** * Check payment processor delete. - * - * @throws \CRM_Core_Exception */ public function testPaymentProcessorDelete() { $result = $this->callAPISuccess('payment_processor', 'create', $this->_params); @@ -154,8 +143,6 @@ public function testPaymentProcessorDelete() { /** * Check with valid params array. - * - * @throws \CRM_Core_Exception */ public function testPaymentProcessorsGet() { $params = $this->_params; @@ -171,33 +158,4 @@ public function testPaymentProcessorsGet() { $this->assertEquals('test@test.com', $results['values'][$results['id']]['user_name']); } - /** - * Test the processor passed to the hook can access the relevant variables. - * - * @throws \CRM_Core_Exception - * @throws \CiviCRM_API3_Exception - */ - public function testPaymentProcessorPay() { - $this->hookClass->setHook('civicrm_alterPaymentProcessorParams', [$this, 'hook_civicrm_alterPaymentProcessorParams']); - $processor = $this->dummyProcessorCreate(); - $this->callAPISuccess('PaymentProcessor', 'pay', [ - 'payment_processor_id' => $processor->getID(), - 'contribution_id' => 10, - 'invoice_id' => 2, - 'contribution_recur_id' => 3, - 'amount' => 6, - ]); - } - - /** - * Implements civicrm_alterPaymentProcessorParams hook. - * - * @param \CRM_Core_Payment_Dummy $paymentObject - */ - public function hook_civicrm_alterPaymentProcessorParams($paymentObject) { - $this->assertEquals(10, $paymentObject->getContributionID()); - $this->assertEquals(2, $paymentObject->getInvoiceID()); - $this->assertEquals(3, $paymentObject->getContributionRecurID()); - } - } From abf0d662e33ac13475eb03e88e9349df0c21439d Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 5 Nov 2019 11:06:46 +1300 Subject: [PATCH 5/5] Re-apply useful cleanups from reverted commits --- CRM/Core/Payment.php | 4 +--- api/v3/PaymentProcessor.php | 4 +++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index 69b97a342fb..91a7c0be8c5 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -81,7 +81,7 @@ abstract class CRM_Core_Payment { RECURRING_PAYMENT_END = 'END'; /** - * @var object + * @var array */ protected $_paymentProcessor; @@ -257,8 +257,6 @@ public function buildForm(&$form) { * @todo move to factory class \Civi\Payment\System (or similar) * * @param array $params - * - * @return mixed */ public static function logPaymentNotification($params) { $message = 'payment_notification '; diff --git a/api/v3/PaymentProcessor.php b/api/v3/PaymentProcessor.php index e2406dc5d0a..c2e5e4e1632 100644 --- a/api/v3/PaymentProcessor.php +++ b/api/v3/PaymentProcessor.php @@ -124,8 +124,10 @@ function _civicrm_api3_payment_processor_getlist_defaults(&$request) { * API result array. * * @throws \API_Exception + * @throws \CiviCRM_API3_Exception */ function civicrm_api3_payment_processor_pay($params) { + /* @var CRM_Core_Payment $processor */ $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']); $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $params['payment_processor_id']])); try { @@ -149,7 +151,7 @@ function civicrm_api3_payment_processor_pay($params) { */ function _civicrm_api3_payment_processor_pay_spec(&$params) { $params['payment_processor_id'] = [ - 'api.required' => 1, + 'api.required' => TRUE, 'title' => ts('Payment processor'), 'type' => CRM_Utils_Type::T_INT, ];