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

CRM/Core/Payment.php

@@ -27,6 +27,7 @@
 
 use Civi\Payment\System;
 use Civi\Payment\Exception\PaymentProcessorException;
+use Civi\Payment\PropertyBag;
 
 /**
  * Class CRM_Core_Payment.
@@ -133,15 +134,6 @@ abstract class CRM_Core_Payment {
    */
   protected $billingProfile;
 
-  /**
-   * Payment instrument ID.
-   *
-   * This is normally retrieved from the payment_processor table.
-   *
-   * @var int
-   */
-  protected $paymentInstrumentID;
-
   /**
    * Is this a back office transaction.
    *
@@ -150,32 +142,37 @@ 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.
+   * Returns an explicitly-set paymentInstrumentID but default to the
+   * payment processor's configured one if it's not been set.
+   *
+   * This is akward in relation to the new propertyBag. Recommend that future
+   * payment processor code does the logic below on it's own passed-in propertyBag
+   * and does not use this method.
    *
    * @return int
    */
   public function getPaymentInstrumentID() {
-    return $this->paymentInstrumentID ? $this->paymentInstrumentID : $this->_paymentProcessor['payment_instrument_id'];
+    $propertyBag = $this->getInternalPropertyBag();
+    if ($propertyBag->has('paymentInstrumentID')) {
+      // We have an explicitly set version, use that.
+      return $propertyBag->getPaymentInstrumentID();
+    }
+    // Otherwise, fallback to the payment instrument specified by the processor.
+    return (int) $this->_paymentProcessor['payment_instrument_id'];
   }
 
   /**
-   * Getter for the id.
+   * Getter for the id Payment Processor ID.
    *
    * @return int
    */
@@ -184,14 +181,53 @@ public function getID() {
   }
 
   /**
+   * Legacy wrapper. Better for a method to work on its own PropertyBag.
+   * But see comment on getter.
+   *
    * Set payment Instrument id.
    *
-   * 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'];
+    return $this->getInternalPropertyBag()->setPaymentInstrumentID($paymentInstrumentID);
+  }
+
+  /**
+   * @return bool
+   */
+  public function isBackOffice() {
+    return $this->backOffice;
+  }
+
+  /**
+   * Set back office property.
+   *
+   * @param bool $isBackOffice
+   */
+  public function setBackOffice($isBackOffice) {
+    $this->backOffice = $isBackOffice;
+  }
+
+  /**
+   * Returns the internal propertyBag.
+   *
+   * (this is an internal function made public for the sake of tests)
+   *
+   * New code should not use this.
+   *
+   * @return \Civi\Payment\PropertyBag
+   */
+  public function getInternalPropertyBag() {
+    if (!isset($this->propertyBag)) {
+      // It has not yet been instantiated.
+      // Nb. we can't do this in the constructor because subclasses override
+      // the constructor and don't call parent::__construct.
+      $this->propertyBag = new PropertyBag();
+    }
+    return $this->propertyBag;
   }
 
   /**
@@ -685,6 +721,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()));
@@ -1058,28 +1096,32 @@ protected function getBaseReturnUrl() {
   }
 
   /**
-   * Get the currency for the transaction.
+   * Get the currency for the transaction from the params.
+   *
+   * Legacy wrapper. Better for a method to work on its own PropertyBag.
    *
-   * Handle any inconsistency about how it is passed in here.
+   * 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.
+   * Legacy. Better for a method to work on its own PropertyBag,
+   * but also, this function does not do very much.
    *
-   * Handle any inconsistency about how it is passed in here.
-   *
-   * @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);
   }
 
@@ -1627,7 +1669,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',
@@ -1636,13 +1678,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

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