-
Notifications
You must be signed in to change notification settings - Fork 67
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
Issue #170 Paypal rest extension #171
Issue #170 Paypal rest extension #171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments.
Also, there's a typo in the filename (PayPaylRestExtension.php)
* If you don't want to supply a Credit card then unsetting this will | ||
* take you through to PayPal without one | ||
*/ | ||
unset($gatewayData['card']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does paying without a card work? Not sure why you'd have to unset that?
*/ | ||
$gatewayData['transactionReference'] = $_GET['paymentId']; | ||
$gatewayData['payerId'] = $_GET['PayerID']; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be necessary… the gateway should be able to pick up the correct params by itself. I'd have to look into the PayPal REST gateway first though.
|
||
if ($omniPayResponse instanceof RestAuthorizeResponse) { | ||
$payment->TransactionReference = $omniPayResponse->getTransactionId(); | ||
$payment->write(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Altering the TransactionReference
should never be done. Why do you have to do this? If you need an updated reference for a given message, use Payment::getLatestMessageOfType
instead (also see the SagePayExtension for something similar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extension currently only covers the purchase
use-case. I noticed for authorize
to work we're missing completeAuthorize
. I've opened an issue for this: thephpleague/omnipay-paypal#170
What about the other actions, such as void
, refund
and capture
. The API seems to support them, have you tested these with the extension?
* If you don't want to supply a Credit card then unsetting this will | ||
* take you through to PayPal without one | ||
*/ | ||
unset($gatewayData['card']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also tried this locally, and indeed, it seems to be a good idea to remove the card… since you'll be redirected to the offsite payment form anyway and according to the DocBlock, using a card is restricted to US and UK only.
You'd need to make sure this will only happen for the PayPal_Rest
gateway though… eg. use something like:
if ($this->owner->getPayment()->Gateway === 'PayPal_Rest') {
unset($gatewayData['card']);
}
* responds from the payment with the transaction reference and a | ||
* PayerID as GET vars. We gather tham and throw them back to PayPal | ||
* for confirmation | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here should also be wrapped in a gateway check
Not tried the other actions but I can give them a go |
I’m not sure how much of this code is still valid, as this module has since upgraded to Omnipay 3.x (it was 2.x when this PR was created). Either way, 4 years of inactivity probably means we can close this 😅 |
Linked to issue #170. Happy to take feedback and further guidance on this to make it better for all. It might also be better to name this explicitly or split it into two? I think that PayPal_Rest in general will need the
onBeforeCompletePurchase
but the removal of the card inonBeforePurchase
is specific to people who want to be able to take a payment without inputting a credit card.