$response can't be used to reply when an exception is thrown

[jamieburchell]

Forgive me if this is not technically an issue and results in my lack of understanding on how to use the library (some documentation would really be useful here, and I know SagePay is backwards). I'm trying to make my payment handling code as gateway agnostic as possible, perhaps I'm not able to completely do that because of how SagePay works - however consider the following code:

$id = isset($_POST['VendorTxCode']) ? $_POST['VendorTxCode'] : ''; // SagePay specific, can we get this using Omnipay?

try {

    $payment = new Payment_m($id);

    $response = $this->gateway->completePurchase([
        'amount'               => $payment->total,
        'transactionId'        => $payment->id,
        'transactionReference' => $payment->gateway_reference
    ])->send();

    if ($response->isSuccessful()) {
        $this->authorise($payment);
    } else {
        $this->reject($payment);
    }

    $response->confirm($this->callback_url . '?id='   . $id);

} catch (Exception $e) {

    // Here be dragons.
    // If the completePurchase() method throws an exception, we can't use $response to call
    // sendResponse() or error() or invalid()
    // so we have this awkward Sagepay specific hack...

    $status = $e instanceof Omnipay\Common\Exception\InvalidResponseException ? 'INVALID' : 'ERROR';
    echo "Status=${status}\r\nRedirectUrl=" . $this->callback_url . '?id='   . $id . '&msg='  . urlencode($e->getMessage());
    exit();
}

If any part of the process throws an exception, I'm not able to reply to SagePay using the $response object, and I end up with that awkward hack in the exception catch. Am I doing something wrong? Can this be written better? I'd appreciate any pointers - or if this is better in a forum somewhere let me know.

[judgej]

Take a look here:

https://github.com/academe/OmniPay-SagePay-Demo/blob/master/sagepay-confirm.php

I put a demo together for a talk. That link is for a demo notify handler, at least a bear skeleton of one. There is a hack in the middle that gets around an exception being thrown, and you should be able to do the same thing.

[judgej]

Also same issue raised here:

https://github.com/thephpleague/omnipay-sagepay/issues/16

Glad you found it, because it means you are handling the notify properly ;-)

There are other discussions around how to handle these notify messages, because they do tend to all be very different between the gateways - SagePay is not exceptional in any way. One idea is to return the appropriate headers and message in the send() whatever the result, and then allowing you to store the result in the database before doing a simple exit() rather than explicitly having to call the exit-with-status method.

I think also $completePurchaseRequest->getTransactionId() needs to be a thing to avoid the need to look at POST data in order to get the VendorTxCode. You should not have to pass that into completeAuthorize message, because that message should already have access to it in the POST data (my demo does pass it in, but I don't think it should need to). Your notify handler needs it though, so you can fetch the transactionReference from the database. Fancy raising that as a ticket? I'll work in it, but it won't be for a few weeks.

[judgej]

Also here my notify script bails out if the transactionId is completely invalid or the transaction is in a status that is not expecting a notify. It should really be able to call $completeResult->invalid(...) instead, so we at least can give it a URL to send the user back to. I think that could be implemented just by providing a blank or null transactionReference before calling send(). So there are certainly some minor improvements that can be done there.

[jamieburchell]

Excellent! That demo helps a lot. I never considered that I can split that code up and use send() separately within the try/catch block (I feel a bit stupid now). That will mean I always have access to the response object.

I guess there are two choices for me regarding the first exception that can be thrown as a result of the payment record being missing:

1) Pass the empty payment object to the completePurchase function regardless - it will throw an InvalidResponseException because the values will be empty.

2) Bail out like you have, the transaction will time out anyway, right?

I noticed that I'm passing in the amount to completePurchase, is this (no longer) necessary?

Would you like me to raise a ticket regarding passing in the transactionId? I don't mind passing it in, but I guess if it's not required there's no point in doing so.

Agree would be nice to be able to obtain the VendorTxCode via the library.

Really appreciate your help.

Jamie

[judgej]

That will mean I always have access to the response object.

Well, almost. If the exception is raised, it will happen when fetching the response object during a send(), so that object will never be created - you won't have it. That is why I then manually create it (yuk) in the catch section.

Your option (1) sounds like a plan to me. That's what I would do now. You only spot these things when coming back to them after a break. My thought at the time was not to give any clues to the sender of an invalid notify, since we don't know who they are. But then, they may just be SagePay having a bad day, so it's probably best to be nice to them.

[judgej]

From what I can see, the amount is not used in the notify hash, so it does not need to be passed in. Some gateways (e.g. Authorize.Net) do pass in the amount, and hash it, so a check is done there to see if the actual amount authorised is what was expected. But not here (I believe).

[judgej]

If it works without passing in the transactionId, then just run with that. I don't think there is any documentation that says it has to be passed in. If it doesn't work, then I think it ought to, so raise a ticket on that.

[jamieburchell]

Well, almost. If the exception is raised, it will happen when fetching the response object during a send(), so that object will never be created - you won't have it. That is why I then manually create it (yuk) in the catch section.

Ha! Yeah I spotted that as I began adapting my code and was about to ask why you couldn't just use the original object. Now I know :)

[judgej]

Just tried the notify without passing in the transactionId and got an error on SagePay:

HTTP Status Code: 500 HTTP Status Message: The request was unsuccessful due to an unexpected condition encountered by the server. Error Code : 5003 Error Description : Internal server error.

I would say that is a bug.

[jamieburchell]

How do you handle passing a reference number and/or message back to your final URL/callback so you can display details to the user? Appending a simple payment ID to the URL seems insecure when anyone could guess an ID and pull up order details. The SagePay reference number is a little harder to guess but not sure if that alone is secure enough. I did look at a slightly convoluted mixture of IDs and a HMAC signature, but this got a bit messy.

[judgej]

The demo shows that (I've just updated the notify BTW, so a bad venderTxCode will return a proper response - but do make a note of what exception I'm catching).

In the demo, it writes the final result to the transaction in storage. When the user returns, the transactionId should still be in their session from when they left the site to go to SP, and that is used to retrieve the result from the database. This is the kind of result the final.php script in the demo gets from the database:

array:5 [▼
  "finalStatus" => "REJECTED"
  "status" => "ABORT"
  "message" => "2013 : The Transaction was cancelled by the customer."
  "transactionReference" => "{"SecurityKey":"FIHDLMMT4B","VPSTxId":"{8DF82654-1297-9956-7418-B7FD35880197}","VendorTxCode":"phpne-demo-40609045"}"
  "notifyData" => array:8 [▼
    "VPSProtocol" => "3.00"
    "TxType" => "PAYMENT"
    "VendorTxCode" => "phpne-demo-40609045"
    "VPSTxId" => "{8DF82654-1297-9956-7418-B7FD35880197}"
    "Status" => "ABORT"
    "StatusDetail" => "2013 : The Transaction was cancelled by the customer."
    "GiftAid" => "0"
    "VPSSignature" => "AB64800974C7B2DBAB7887BFAEED75BD"
  ]
]

In this case I pressed the "Cancel" button.

[jamieburchell]

Yes, using the session for this and storing the details in the database is much neater.

Maybe I need to look at storing the entire response like you have, I only keep the status, message, gateway ID and security code in separate fields rather than having to pull apart an object each time I need a detail - but I can see it's useful to have the full response at least for troubleshooting.

On 23 Jul 2015, at 17:24, Jason Judge notifications@github.com wrote:

The demo shows that (I've just updated the notify BTW, so a bad venderTxCode will return a proper response - but do make a note of what exception I'm catching).

In the demo, it writes the final result to the transaction in storage. When the user returns, the transactionId should still be in their session from when they left the site to go to SP, and that is used to retrieve the result from the database.

— Reply to this email directly or view it on GitHub.

[judgej]

I also store the important and indexable items in separate fields in production sites, but I do stuff the notify data into a json string and stick it in a text field just for reference. It is handy for diagnosing issues later. I have another SagePay package where all possible fields of the transaction are separate columns, and that is total overkill.

If using an orm such as eloquent, you can automate the serialisation and de-serialisation of these arrays, so it is easy just to throw in the notify data array, the transaction request array etc. and not have to worry about exactly what fields are in it. But pull out statuses, messages, maybe some payment transaction codes into separate fields so you don't need to go sifting through this data. I also put the amount in a separate column, which is very handy for reporting on.

My ultimate aim is to create a data store model that will work for any OmniPay gateway, and would make doing this kind of thing sooo much easier across different sites, frameworks and gateways.

[jamieburchell]

Some great tips. I will store both responses too as a json object for debugging.

[judgej]

Fixed in PR #69 - use the acceptNotification handler to avoid this specific exception.