-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use of transactionReference
for purchases in v2.3 causing error
#21
Comments
transactionReference
in 2.3 is not correct causing errortransactionReference
for purchases in v2.3 causing error
transactionReference
for purchases in v2.3 causing errortransactionReference
for purchases in v2.3 causing error
@lukeholder I’ll be happy to look into this. The implementation from the standpoint of this module is correct but if it is causing an interface compatibility issue with the larger Omnipay echo system, then we should find a way to correct that. Perhaps you and I can chat and discuss and come up with a good solution. Cheers! |
Thanks @RobvH Thinking it might be best to revert the commit, and just place a note in the comments and README documentation that to make a secondary I will leave it to you to test that. Otherwise, maybe introduce a |
@lukeholder well I’d highly prefer not to revert since the functionality since it is both needed and currently processes our revenue stream. I’m confident if I understand exactly how it is breaking things on your end that we can devise a suitable replacement. This is how reference sale transactions must be done with Payflow and they must be done. Pm me @Rob_vH and we can hash out a fix that works for us both. :) |
@RobvH Prefer to discuss in the open, happy to get input from others. Would be good to get @delatbabel's thoughts. If the changes in 2.3 regarding I will make a new pull request and reference this issue so we can debate something concrete. |
OK, @RobvH and @delatbabel what do you think of that pull request? If a purchase request includes real card data, the transaction reference is not used as the |
+1 on the PR. Fantastic solution!! I asked there, but will repeat here in case it's helpful to anyone reading this later: the break you're experiencing is from having a transaction reference together with card data at the same time. Could you shed some light on the use case for this? Are you passing a valid Throughout all of the Payflow driver, the transaction reference field is a I used transactionReference b/c the cardReference implementation was broken but historically broken, so I was concerned that correcting that might affect existing implementations. Hence I implemented reference sales to the spec and used a consistent naming, transaction reference. That said, if you can help me understand why transaction reference is legitimate in the sale request with card data, I will do a PR in which I fix the broken aspect of cardReference that lead to this in the first place. That will prevent us piling any more corrective code on. :) |
OK those two PRs look OK to me. Any objections to me merging both? And can someone test in a production environment after I do, and before I tag for release? Sorry about the delay, I have been absent with an extended illness. Slowly catching up on things. |
@lukeholder Did merging PR #23 solve the issue for you? Is PR #22 still necessary? |
It looks like special use of
transactionReference
was added in 2.3 to this omnipay driver. It has broken our multi-gateway commerce implementation when our customers use this driver.Code Changes:
#19
v2.2.2...v2.3
I think using the transaction reference for the purchase is wrong here. No other omnipay gateways do this for a
->purchase()
. I supply a transaction reference to all gateways on purchase in cases where they are happy to use our reference over their own. If they respond with their own transaction reference in response, then we use theirs from then on.I believe it should be setting the
cardReference
to a token that can make the purchase (even if token that is a stored transaction reference).@RobvH You are using the transactionRef to override the
ORIGID
https://github.com/thephpleague/omnipay-payflow/blob/master/src/Message/PurchaseRequest.php#L87-L89
But the card reference is also already doing this:
https://github.com/thephpleague/omnipay-payflow/blob/master/src/Message/AuthorizeRequest.php#L240-L242
@RobvH you state in the code comments:
I understand using the transactionReference to refund or capture since it is the identification of the original transaction in the gateway, but not for new purchases.
From Payflows perspective this may be correct, but there is nothing stopping you from using the previous transaction reference and setting it to the card token to acheive the same outcome
No other omnipay gateway I can find uses transaction reference to determine the charge source. We should stick to using
cardReference
and straight credit card data being passed topurchase()
@delatbabel can we move to correct this soon?
The text was updated successfully, but these errors were encountered: