Upgrade for v3#40
Upgrade for v3#40barryvdh merged 5 commits intothephpleague:masterfrom ptuchik:feature/upgrade_for_v3
Conversation
| public function sendData($data) | ||
| { | ||
| $response = $this->braintree->paymentMethod()->update($data['token'], $data['options']); | ||
| $response = $this->braintree->paymentMethod()->update($data['token'], $data); |
There was a problem hiding this comment.
Is this change related to the v3 upgrade?
There was a problem hiding this comment.
No, this was a bug in previous version. I fixed there on my fork when using with v2.x and it is still actual.
There was a problem hiding this comment.
I just tested with $data['options'] ?? [] instead of data, it is not working because of Braintree\Util's validation. So the only way it will work is the one I've commited
| try { | ||
| $response = $this->braintree->customer()->find($this->getCustomerId()); | ||
| } catch (NotFound $exception) { | ||
| throw new NotFoundHttpException($exception->getMessage()); |
There was a problem hiding this comment.
I wouldn't throw this error, but rather Omnipay\Common\Exception\InvalidResponseException. This error could cause the application to render a 404 page instead of an error page (for example Laravel converts the HttpExceptions to specific errors)
There was a problem hiding this comment.
Thanks for your comment. I agree with you, but the main reason I'm throwing not found exception is for those (like me), who wants to recreate payment profile if the existing one is invalid or not found, without showing the error. So I'm throwing the general not found here, catching it in my Billable trait and recreating the profile. And I used generic Not Found instead of Braintree's one to have the logic working with other gateways also.
There was a problem hiding this comment.
I think it's better then to create your own exception that extends the omnipay invalidresponse exception.
There was a problem hiding this comment.
I agree, the problem is that if I create may own in this package, I can't catch it generalized, because other payment gateway packages will not have that exception. That's why I used Symphony's one
There was a problem hiding this comment.
If you extend the InvalidResponse one, all gateway can catch that. You can just add specific logic for that exception.
There was a problem hiding this comment.
Created, here is the PR for Omnipay Common thephpleague/omnipay-common#187 and I've updated this one as well
There was a problem hiding this comment.
Can you remove the try/catch for now and leave it as before, so we can merge this? We can update it once we merge the PR and a new tag is released.
There was a problem hiding this comment.
OK, just 2 questions before I remove:
- When you think the tag will be released?
- May I leave the previous version with Symphony's NotFound until the release of Common package? To be able to handle it without hardcoding "Braintree" namespace in my application
There was a problem hiding this comment.
- probably with a week or so, but not sure.
- No but you could use a gateway specific exception.
|
Thank you |
|
Could you please let me know, when this merge will be available to Packagist? |
|
Please test this using version |
|
Only thing blocking is this error: https://travis-ci.org/thephpleague/omnipay-braintree/jobs/383745948 |
|
I see, maybe it needs to be |
|
Not sure, I marked it as skipped. Don't think that needs to be intended behavior. |
2 participants
Tested on my project, fully working as with v2.x