Introduce PropertyBag to help standardise payment params

WIP: proof of concept for payment property bag

WIP: keep civilint happy

WIP: keep civilint happy again

Add other getters, setters, tests

Add require() method and test

some wip code

linter fixes

remove :void from function declaration in Civi/Payment/PropertyBag

fix PropertyBagTest.php  tests

fix linter

Re-do payment code lost in rebase

linting fixes

More WIP on payment property bag

one day i'll run civilint before committing

WIP checkin

Correct test which referred to Credit card instead of Credit Card causing a fail with new property bag which is stricter

Payment PropertyBag: allow week as recurFrequencyUnit; accept ZLS for feeAmount for legacy sake

CRM_Core_Payment: remove stuff cut from release

remove test no longer needed

remove un-needed code because feature dropped before it was released

remove comment

Remove/disarm payment instrument setter

restore getPaymentInstrumentID behaviour

remove not required function

set x

CRM/Core/Payment.php

@@ -11,6 +11,7 @@
 
 use Civi\Payment\System;
 use Civi\Payment\Exception\PaymentProcessorException;
+use Civi\Payment\PropertyBag;
 
 /**
  * Class CRM_Core_Payment.
@@ -134,32 +135,41 @@ abstract class CRM_Core_Payment {
   protected $backOffice = FALSE;
 
   /**
-   * @return bool
-   */
-  public function isBackOffice() {
-    return $this->backOffice;
-  }
-
-  /**
-   * Set back office property.
+   * This is only needed during the transitional phase. In future you should
+   * pass your own PropertyBag into the method you're calling.
    *
-   * @param bool $isBackOffice
+   * New code should NOT use $this->propertyBag.
+   *
+   * @var Civi\Payment\PropertyBag
    */
-  public function setBackOffice($isBackOffice) {
-    $this->backOffice = $isBackOffice;
-  }
+  protected $propertyBag;
 
   /**
-   * Get payment instrument id.
+   * Return the payment instrument ID to use.
+   *
+   * Note:
+   * We normally SHOULD be returning the payment instrument of the payment processor.
+   * However there is an outstanding case where this needs overriding, which is
+   * when using CRM_Core_Payment_Manual which uses the pseudoprocessor (id = 0).
+   *
+   * i.e. If you're writing a Payment Processor you should NOT be using
+   * setPaymentInstrumentID() at all.
+   *
+   * @todo
+   * Ideally this exception-to-the-rule should be handled outside of this class
+   * i.e. this class's getPaymentInstrumentID method should return it from the
+   * payment processor and CRM_Core_Payment_Manual could override it to provide 0.
    *
    * @return int
    */
   public function getPaymentInstrumentID() {
-    return $this->paymentInstrumentID ? $this->paymentInstrumentID : $this->_paymentProcessor['payment_instrument_id'];
+    return isset($this->paymentInstrumentID)
+      ? $this->paymentInstrumentID
+      : (int) ($this->_paymentProcessor['payment_instrument_id'] ?? 0);
   }
 
   /**
-   * Getter for the id.
+   * Getter for the id Payment Processor ID.
    *
    * @return int
    */
@@ -168,14 +178,33 @@ public function getID() {
   }
 
   /**
-   * Set payment Instrument id.
+   * @deprecated Set payment Instrument id - see note on getPaymentInstrumentID.
    *
-   * By default we actually ignore the form value. The manual processor takes it more seriously.
+   * By default we actually ignore the form value. The manual processor takes
+   * it more seriously.
    *
    * @param int $paymentInstrumentID
    */
   public function setPaymentInstrumentID($paymentInstrumentID) {
-    $this->paymentInstrumentID = $this->_paymentProcessor['payment_instrument_id'];
+    $this->paymentInstrumentID = (int) $paymentInstrumentID;
+    // See note on getPaymentInstrumentID().
+    return $this->getPaymentInstrumentID();
+  }
+
+  /**
+   * @return bool
+   */
+  public function isBackOffice() {
+    return $this->backOffice;
+  }
+
+  /**
+   * Set back office property.
+   *
+   * @param bool $isBackOffice
+   */
+  public function setBackOffice($isBackOffice) {
+    $this->backOffice = $isBackOffice;
   }
 
   /**
@@ -669,6 +698,8 @@ 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()));
@@ -1042,28 +1073,32 @@ protected function getBaseReturnUrl() {
   }
 
   /**
-   * Get the currency for the transaction.
+   * Get the currency for the transaction from the params.
    *
-   * Handle any inconsistency about how it is passed in here.
+   * Legacy wrapper. Better for a method to work on its own PropertyBag.
+   *
+   * This code now uses PropertyBag to allow for old inputs like currencyID.
    *
    * @param $params
    *
    * @return string
    */
-  protected function getCurrency($params) {
-    return CRM_Utils_Array::value('currencyID', $params, CRM_Utils_Array::value('currency', $params));
+  protected function getCurrency($params = []) {
+    $localPropertyBag = new PropertyBag();
+    $localPropertyBag->mergeLegacyInputParams($params);
+    return $localPropertyBag->getCurrency();
   }
 
   /**
-   * Get the currency for the transaction.
-   *
-   * Handle any inconsistency about how it is passed in here.
+   * Legacy. Better for a method to work on its own PropertyBag,
+   * but also, this function does not do very much.
    *
-   * @param $params
+   * @param array $params
    *
    * @return string
+   * @throws \CRM_Core_Exception
    */
-  protected function getAmount($params) {
+  protected function getAmount($params = []) {
     return CRM_Utils_Money::format($params['amount'], NULL, NULL, TRUE);
   }
 
@@ -1611,7 +1646,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',
@@ -1620,13 +1655,7 @@ protected function getPaymentDescription($params, $length = 24) {
       'billing_last_name',
     ];
     $validParts = [];
-    if (isset($params['description'])) {
-      $uninformativeStrings = [
-        ts('Online Event Registration: '),
-        ts('Online Contribution: '),
-      ];
-      $params['description'] = str_replace($uninformativeStrings, '', $params['description']);
-    }
+    $params['description'] = $this->getDescription();
     foreach ($parts as $part) {
       if ((!empty($params[$part]))) {
         $validParts[] = $params[$part];

CRM/Core/Payment/Form.php

@@ -48,7 +48,9 @@ public static function setPaymentFieldsByProcessor(&$form, $processor, $billing_
 
     $processor['object']->setBillingProfile($billing_profile_id);
     $processor['object']->setBackOffice($isBackOffice);
-    $processor['object']->setPaymentInstrumentID($paymentInstrumentID);
+    if (isset($paymentInstrumentID)) {
+      $processor['object']->setPaymentInstrumentID($paymentInstrumentID);
+    }
     $paymentTypeName = self::getPaymentTypeName($processor);
     $form->assign('paymentTypeName', $paymentTypeName);
     $form->assign('paymentTypeLabel', self::getPaymentLabel($processor['object']));