[REF] Simplify is_email_receipt handling

This removes a chunk of confusing handling from the previously shared code.

Philosophically we have a lot of arrays of data being passed around. The difference
between these arrays is unclear & the array that appears to exist specifically to
how what was actually submitted (formValues) is edited rather than left intact.

Where we are not dealing with a calcuated value, but rather referring to
what was submitted the quickform function getSubmittedValue() seems
like a good pick. It's clear we are looking up the submitted value not
some calculated value & when code is moved around we know this
value is not dependent on where it is sitting in the code.

The function returns 1 or NULL - hence I cast to boolean where I'm not
sure NULL is good enough

diff --git a/CRM/Core/Form.php b/CRM/Core/Form.php
index a508f692226e905ab0d1916c29b2e2431412f797..c25e991bdce1ee0dae06f9d822bd23181342c12c 100644 (file)
--- a/CRM/Core/Form.php
+++ b/CRM/Core/Form.php
@@ -178,6 +178,19 @@ class CRM_Core_Form extends HTML_QuickForm_Page {
    */
   public $submitOnce = FALSE;
 
+  /**
+   * Values submitted by the user.
+   *
+   * These values have been checked for injection per
+   * https://pear.php.net/manual/en/package.html.html-quickform.html-quickform.exportvalues.php
+   * and are as submitted.
+   *
+   * Once set this array should be treated as read only.
+   *
+   * @var array
+   */
+  protected $exportedValues = [];
+
   /**
    * @return string
    */
@@ -2719,4 +2732,21 @@ class CRM_Core_Form extends HTML_QuickForm_Page {
     return CRM_Contact_BAO_Contact_Utils::validChecksum($contactID, $userChecksum) ? $contactID : FALSE;
   }
 
+  /**
+   * Get values submitted by the user.
+   *
+   * These values have been validated against the fields added to the form.
+   * https://pear.php.net/manual/en/package.html.html-quickform.html-quickform.exportvalues.php
+   *
+   * @param string $fieldName
+   *
+   * @return mixed|null
+   */
+  protected function getSubmittedValue(string $fieldName) {
+    if (empty($this->exportedValues)) {
+      $this->exportedValues = $this->controller->exportValues($this->_name);
+    }
+    return $this->exportedValues[$fieldName] ?? NULL;
+  }
+
 }

diff --git a/CRM/Member/Form.php b/CRM/Member/Form.php
index b476a249a785f47610d3edaae0a047c6212eb6d5..5398405b6da74a3e4b846ae0105f67837cd8de41 100644 (file)
--- a/CRM/Member/Form.php
+++ b/CRM/Member/Form.php
@@ -478,6 +478,7 @@ class CRM_Member_Form extends CRM_Contribute_Form_AbstractEditPayment {
    * @param array $formValues
    */
   public function testSubmit(array $formValues): void {
+    $this->exportedValues = $formValues;
     $this->setContextVariables($formValues);
     $this->_memType = $formValues['membership_type_id'][1];
     $this->_params = $formValues;

diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php
index 0fa3b42564d000943a9834e0d49c39f6c8805c0a..7be3212c33e191b29cb2be1cb6ff0818cb9463bc 100644 (file)
--- a/CRM/Member/Form/Membership.php
+++ b/CRM/Member/Form/Membership.php
@@ -1215,7 +1215,7 @@ DESC limit 1");
         $this->assign('is_pay_later', 1);
       }
 
-      if (!empty($formValues['send_receipt'])) {
+      if ($this->getSubmittedValue('send_receipt')) {
         $params['receipt_date'] = $formValues['receive_date'] ?? NULL;
       }
 
@@ -1273,7 +1273,7 @@ DESC limit 1");
           $softParams['soft_credit_type_id'] = $formValues['soft_credit_type_id'];
         }
       }
-      if (!empty($formValues['send_receipt'])) {
+      if ($this->getSubmittedValue('send_receipt')) {
         $paymentParams['email'] = $this->_contributorEmail;
       }
 
@@ -1344,8 +1344,6 @@ DESC limit 1");
       if ($paymentStatus !== 'Completed') {
         $params['status_id'] = $pendingMembershipStatusId;
         $params['skipStatusCal'] = TRUE;
-        // unset send-receipt option, since receipt will be sent when ipn is received.
-        unset($formValues['send_receipt'], $formValues['send_receipt']);
         //as membership is pending set dates to null.
         foreach ($this->_memTypeSelected as $memType) {
           $membershipTypeValues[$memType]['joinDate'] = NULL;
@@ -1363,12 +1361,12 @@ DESC limit 1");
       $params['source'] = $formValues['source'] ?: $params['contribution_source'];
       $params['trxn_id'] = $result['trxn_id'] ?? NULL;
       $params['is_test'] = $this->isTest();
-      if (!empty($formValues['send_receipt'])) {
+      $params['receipt_date'] = NULL;
+      if ($this->getSubmittedValue('send_receipt') && $paymentStatus === 'Completed') {
+        // @todo this should be updated by the send function once sent rather than
+        // set here.
         $params['receipt_date'] = $now;
       }
-      else {
-        $params['receipt_date'] = NULL;
-      }
 
       $this->set('params', $formValues);
       $this->assign('trxn_id', CRM_Utils_Array::value('trxn_id', $result));
@@ -1513,7 +1511,7 @@ DESC limit 1");
     }
 
     $receiptSent = FALSE;
-    if (!empty($formValues['send_receipt']) && $receiptSend) {
+    if ($this->getSubmittedValue('send_receipt') && $receiptSend) {
       $formValues['contact_id'] = $this->_contactID;
       $formValues['contribution_id'] = $contributionId;
       // We really don't need a distinct receipt_text_signup vs receipt_text_renewal as they are
@@ -1851,27 +1849,17 @@ DESC limit 1");
     $transaction = new CRM_Core_Transaction();
     $contactID = $contributionParams['contact_id'];
 
-    $isEmailReceipt = !empty($form->_values['is_email_receipt']);
-
     // add these values for the recurringContrib function ,CRM-10188
     $params['financial_type_id'] = $contributionParams['financial_type_id'];
 
-    //@todo - this is being set from the form to resolve CRM-10188 - an
-    // eNotice caused by it not being set @ the front end
-    // however, we then get it being over-written with null for backend contributions
-    // a better fix would be to set the values in the respective forms rather than require
-    // a function being shared by two forms to deal with their respective values
-    // moving it to the BAO & not taking the $form as a param would make sense here.
-    if (!isset($params['is_email_receipt']) && $isEmailReceipt) {
-      $params['is_email_receipt'] = $isEmailReceipt;
-    }
+    $params['is_email_receipt'] = (bool) $this->getSubmittedValue('send_receipt');
     $params['is_recur'] = TRUE;
     $params['payment_instrument_id'] = $contributionParams['payment_instrument_id'] ?? NULL;
     $recurringContributionID = $this->legacyProcessRecurringContribution($params, $contactID);
 
     $now = CRM_Utils_Time::date('YmdHis');
     $receiptDate = $params['receipt_date'] ?? NULL;
-    if ($isEmailReceipt) {
+    if ($params['is_email_receipt']) {
       $receiptDate = $now;
     }
 
@@ -1886,7 +1874,7 @@ DESC limit 1");
         'skipLineItem' => $params['skipLineItem'] ?? 0,
       ], $contributionParams);
 
-      if (!empty($params["is_email_receipt"])) {
+      if ($this->getSubmittedValue('send_receipt')) {
         $contributionParams += [
           'receipt_date' => $receiptDate,
         ];
@@ -1948,7 +1936,7 @@ DESC limit 1");
     $recurParams['invoice_id'] = $params['invoiceID'] ?? NULL;
     $recurParams['contribution_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Pending');
     $recurParams['payment_processor_id'] = $params['payment_processor_id'] ?? NULL;
-    $recurParams['is_email_receipt'] = (bool) ($params['is_email_receipt'] ?? FALSE);
+    $recurParams['is_email_receipt'] = (bool) $this->getSubmittedValue('send_receipt');
     // we need to add a unique trxn_id to avoid a unique key error
     // in paypal IPN we reset this when paypal sends us the real trxn id, CRM-2991
     $recurParams['trxn_id'] = $params['trxn_id'] ?? $params['invoiceID'];