Skip to content
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

Added not found exception #187

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

ptuchik
Copy link

@ptuchik ptuchik commented May 25, 2018

Thrown when the requested resource is not found on payment gateway to differentiate from other invalid responses

*/
class NotFoundException extends \Exception implements OmnipayException
{
public function __construct($message = "Resource not found on payment gateway", $code = 404, $previous = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code can be just 0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to change or 404 is also OK?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to 0

@barryvdh
Copy link
Member

What do you think about the name. SHould it be more descriptive? ResourceNotFoundException perhaps?

@judgej thoughts?

@ptuchik
Copy link
Author

ptuchik commented May 25, 2018

Can be, it is just a bit longer. Let me know please if I have to change or not. My project is waiting for these updates :)

@judgej
Copy link
Member

judgej commented May 25, 2018

Are any of the other exceptions thrown as a result of a specific gateway request error, or does this one stand alone (possibly one of a group of related exceptions)? Just wondering (if I understand correctly) whether exceptions from the remote gateway complaining should be namespaced together, with a generic exception and more detailed exceptions extended by the drivers?

@ptuchik
Copy link
Author

ptuchik commented May 25, 2018

The original integration was throwing only SDK’s exceptions, currently I have PR overriding only “customer not found” error, to be able to catch and recreate profile instead of showing error to user. In future we can try to morph all errors to Omnipay’s exceptions

@barryvdh
Copy link
Member

It can throw an InvalidResponse but that's not really specific.

@judgej
Copy link
Member

judgej commented May 25, 2018

What's throwing it - omnipay-common or the driver?

Assuming it's the driver, the driver could extend InvalidResponse with something that is appropriate to that driver. A few common extensions (stuff REST is generally involved in) could be added to omnipay-common to start with, but it would allow a driver to add its own, e.g. NoPermissionResponse but still be caught as a generic "problem talking to the gateway" exception by the merchant site if it wants to.

Anyway - just a thought - I'm no expert at structuring exceptions, and a little behind at what PHP 7.2+ is bringing to the exception table.

@ptuchik
Copy link
Author

ptuchik commented May 25, 2018

@judgej Totally agreed with you, I just suggested to throw this generic Omnipay's exception, to not hardcode Gateway-specific namespaces in multi-gateway application to be able to catch them all

@ptuchik
Copy link
Author

ptuchik commented May 25, 2018

Changed to ResourceNotFoundException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants