The comments for the function commerce_authnet_cim_cardonfile_charge() state:

 * @return
 *   TRUE if the transaction was successful

For all scenarios where the charge failed, FALSE is returned - but in the only instance when the transaction succeeded, it does not return TRUE, just a return statement. This is interpreted as NULL, therefore, when invoking commerce_authnet_cim_cardonfile_charge() in custom code, the return value is NULL when it should be TRUE. Otherwise, it is unreliable to evaluate.

For example, if I have a statement like:

if ($balance['amount'] && commerce_authnet_cim_cardonfile_charge($params...))

The inner statement is never entered because both conditions do not evaluate to TRUE.

Simply changing line 917 in commerce_authnet.module to return TRUE; instead of just return; corrects this behavior.

CommentFileSizeAuthor
#8 return_transaction_status-2161989_8.patch524 bytesfreality
#4 return_transaction_status-2161989.patch485 bytesfavrik
#1 commerce_authnet-return_true_for_cardonfile_charge_callback-2161989.patch469 bytesandyg5000

Comments

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Priority: Normal » Major
StatusFileSize
new commerce_authnet-return_true_for_cardonfile_charge_callback-2161989.patch469 bytes

Setting as major since commerce_cardonfile_order_charge_card expects the callback to return a boolean. When null is returned, the rules to update the order status don't fire. Easy patch attached ;)

favrik’s picture

favrik commented

I think it should really be returning the transaction status and not just TRUE, for future proof or plans? Not sure.

http://drupalcode.org/project/commerce_cardonfile.git/blob/a34a42a926c26...

favrik’s picture

favrik commented

Yep, I think it should be the transaction status. The reason returning TRUE works is because the in_array check is not using the strict parameter, hence you have something like:

  $method_return = TRUE;
  $status = array(COMMERCE_PAYMENT_STATUS_SUCCESS, COMMERCE_PAYMENT_STATUS_PENDING);
  print_r($status);
  /*
Array
(
    [0] => success
    [1] => pending
)
*/

var_dump(in_array(TRUE, $status)); // TRUE
var_dump(in_array(TRUE, $status, $strict=TRUE)); // FALSE
favrik’s picture

favrik commented
StatusFileSize
new return_transaction_status-2161989.patch485 bytes
focal55’s picture

focal55 commented

Thank you favrik and andyg5000, working great.

apower’s picture

apower commented

Patch #4 works great! I actually found this problem when using the Commerce Authorize.Net module with Commerce Recurring. Because the transaction status wasn't returned the Rules event "commerce_cardonfile_charge_success" was never invoked. Which meant the status of my recurring orders never changed.

So for anyone else using Commerce Authorize.Net 7.x-1.1 with Commerce Recurring, you'll definitely need to apply patch #4.

freality’s picture

freality commented

Patch #4 works for me as well. But looking at the code there is a case where $transaction->status is not set.

$transaction->status is set to failed based on the authnet response at line 777. Else, $transaction->status is set within a switch statement starting at line 781. However, for whatever reason, if $payment_method['settings']['txn_type'] is not set, $transaction->status is not set.

I think a default case should be added to catch this, or a test return (isset($transaction->status)) ? $transaction->status : TRUE; should exist in the return. I'm not sure what the default status should be, but a default would make the code more bulletproof.

freality’s picture

freality commented
StatusFileSize
new return_transaction_status-2161989_8.patch524 bytes

The attached patch simple tests if $transaction->status isset.

return (isset($transaction->status)) ? $transaction->status : TRUE;

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Status: Active » Reviewed & tested by the community

I love it when I am stumped by the same issue twice :) Let's get this committed so it doesn't happen again

sushantpaste’s picture

sushantpaste
Primary language Marathi
Location Sawantwadi India
commented

#8 works great . thanks all.

greatmatter’s picture

greatmatter commented

YES. You ALL rock. Thank you.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Reviewed & tested by the community » Fixed

#8 committed!

  • mglaman committed 6dee009 on 7.x-1.x authored by freality 
    Issue #2161989 by freality, favrik, andyg5000 | kevinquillen: Fixed...

Status: Fixed » Closed (fixed)

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

MichaelSt’s picture

MichaelSt commented

Unfortunately I'm getting this again even with the latest 7.x-1.2.

I thought it was committed but several charges were just run and they aren't supposed to be charged until the middle of June.

Turned out I had a parallel cloud development version of my site I never set up or knew about - sorry for the false alarm.