Validation/changing parameters
barryvdh opened this issue · 9 comments
The example class (zip found here) does some changes:
- Replace paymentmethod values to make sure they are valid (ideal -> IDEAL, MAESTRO -> MEASTRO etc)
- Strip invalid chars from orderid/transactionid
- Override the merchantid/key etc for testMode
- Force currency to EUR for iDeal/minitix
Is that something that should be done by the Omnipay gateway? Or are we expecting accurate input from everyone, or should the validate() method be extended to check for these items?
And further more, are orderId and transactionReference used correct?
Unique transaction ID used for subsequently updating the status of the order. The webshop must
be able to generate a unique code for every payment request. The code is visible to the customer on the Rabo OmniKassa payment page, the account statement of the consumer and would be referenced with a refund, etc.
Should we perhaps use transactionId to set the orderId, and generate our own transactionReference (unique string)? That way we make sure transactionReference is unique and we can use it similar to other gateways.
To clarify, this is from the original example (translated from dutch)
// Make sure that EVERY PAYMENTREQUEST should have a unique reference.
// In this example we use the the time to make the code unique, but an attempt-number or similar would be better.
$sTransactionReference = $sOrderId . 'x' . date('His'); // Unique transaction reference, up to 35 characters ([a-zA-Z0-9]+)
$oOmniKassa->setTransactionReference($sTransactionReference);
So what if we generate a hash, based on the time()
, transactionId (if set) and/or uniqid()
? That way the developers using this gateway, could get the transactionReference from the PurchaseResponse without having to set it, and compare it with CompletePurchaseResponse.
Should I create a PR to do this, and change orderId to transactionId? (Which would break all the things, so need a new major version)
To the first comment:
I'm okay with overriding setPaymentMethod to ensure that all input is strtoupper()'d, because the gateway is looking for that. I'm hesitant to mutate the data any further than that, however. I'd much rather throw an exception there, if the payment method isn't spelled correctly, or a valid option.
As for orderId/transactionReference, the way the example you linked seems to be doing it is this:
$aData['transactionReference'] = ($this->sTransactionReference ? $this->sTransactionReference : md5(time() . $this->sOrderId));
Which allows it to be set explicitly by the developer, and if it's not set, then it will generate based on the orderID + hashed time. I'll need to look at it a bit further, but i don't think that would involve any BC breaks, and would still accomplish the goal.
Did I miss something?
No that would be fine. But currently the transactionReference is set like this:
$transRef = $this->getTransactionReference() ?: $this->getTransactionId();
So that could be changed to something like this:
$transRef = $this->getTransactionReference() ?: $this->generateTransactionReference();
Which would be fine if people are either not setting the transaction reference (which they normally don't, because other gateways don't need it) or set the transactionReference directly. But the fallback transactionId doesn't work anymore.
I see now that it's probably just wrong now, because getTransactionId() in the response should be getTransactionReference(): https://github.com/thephpleague/omnipay-rabobank/blob/master/src/Message/CompletePurchaseResponse.php#L79
And the transactionId() isn't used, but orderId() is, so that is probably also wrong, right?
Yes, I would think that getTransactionId
should be changed to getTransactionReference()
, which would cause some BC breaks.
The other option is to define both functions, having one call the other.
Yes and I also suggest to remove setOrderId, so that is also BC.
But created a PR in #8 where we can discuss.
I'll look at the other things suggested here on validating/changing the data as done in the example class.
What is your reasoning behind removing set/getOrderId()?
I've already responded inline to that, but just to keep the record here clear.
The whole transactionId() and transactionReference() has always been a bit unclear to me and it ísn't really mentioned in the docs (or I've misread). And most gateways aren't following it exactly.
But in my experience:
- transactionId = an internal reference for the developer (eg. the id in the database or the orderId or similar)
- transactionReference = reference set by the gateway to keep track of transactions.
So in this case orderId == transactionId and transactionReference == transactionReference.
But it would be nice if this was clearer in the docs, what the expected usage of these parameters is, not just what matches the naming conventions of the api being used.
@kayladnls What is the consensus about testMode?
If I enable testMode, should it send the test merchantId/secretKey/keyVersion automatically? Or do the developers need to change them themselves (what would make the testMode parameter useless).
And similar, should Omnipay check if an issuer is invalid for the current mode? For example. sisow uses issuer 99 (or something) in Testmode, so when testMode is off, should it throw an exception when that issuer is used? And similar for Rabobank, if the testKey/merchant is used with testMode disabled, should it also throw an exception? It's the developers responsibility to check etc, but the Omnipay gateway could help prevent these errors.