I just noticed that in the function _commerce_stripe_get_txn_capture_bool(), it assumes that a txn_type option will always be set, and returns NULL if no conditions are met. This is not ideal, since the returned value is also submitted to the payment gateway.

/**
 * Used in commerce_stripe_submit_form_submit to get the capture flag for the
 * charge.
 *
 * @param array $payment_method
 *  An array containing the payment_method information.
 * @param array $pane_values
 *  An array containing values from the Commerce Stripe payment pane.
 *
 * @return bool
 *  Returns TRUE if capture is set & FALSE for auth only.
 */
function _commerce_stripe_get_txn_capture_bool($payment_method, $pane_values) {
  $txn_type = $payment_method['settings']['txn_type'];
  // This handles the case when we are in the payment terminal. The
  // $pane_values contains which type of transaction chosen
  if (!empty($pane_values['txn_type'])) {
    $txn_type = $pane_values['txn_type'];
  }

  // The capture flag in the charge takes a boolean. This simply
  // translates the txn constants to a bool.
  if ($txn_type == COMMERCE_CREDIT_AUTH_CAPTURE) {
    return TRUE;
  }
  else {
    if ($txn_type == COMMERCE_CREDIT_AUTH_ONLY) {
      return FALSE;
    }
  }
  return NULL;
}

We should fix this to ensure only a Boolean is ever returned, and also to handle the default if no option was set. (Since Capturing a payment is the default behavior, this makes a sensible default.)

Comments

torgosPizza created an issue. See original summary.

torgospizza’s picture

Here's the new function:

function _commerce_stripe_get_txn_capture_bool($payment_method, $pane_values) {
  $txn_type = !empty($payment_method['settings']['txn_type']) ? $payment_method['settings']['txn_type'] : COMMERCE_CREDIT_AUTH_CAPTURE;

  // This handles the case when we are in the payment terminal. The
  // $pane_values contains which type of transaction chosen
  if (!empty($pane_values['txn_type'])) {
    $txn_type = $pane_values['txn_type'];
  }

  // The capture flag in the charge takes a boolean. This simply
  // translates the txn constants to a bool.
  if ($txn_type == COMMERCE_CREDIT_AUTH_CAPTURE) {
    return TRUE;
  }
  else {
    if ($txn_type == COMMERCE_CREDIT_AUTH_ONLY) {
      return FALSE;
    }
  }
  // Return TRUE by default.
  return TRUE;
}

The last return TRUE; will probably never be reached, but it's there just in case we miss anything. Not only does this fix any IDEs from complaining about a mixed return value type, but should also prevent the Stripe API from puking. And, for store owners who are upgrading from the previous Alpha version, we can avoid causing problems if they haven't visited the payment settings form in a while.

I'll get this committed to -dev asap and then try to roll another alpha.

  • torgosPizza committed b3fd682 on 7.x-3.x
    Issue #2922329: Use Auth+Capture if upgrading and no option was set
    
torgospizza’s picture

Status: Active » Fixed

Committed to dev and will be included in alpha3.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.