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

Storing Response in ApiException #10

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

Conversation

mpouris
Copy link

@mpouris mpouris commented Dec 7, 2015

Beanstream API can throw a plethora of exceptions stating issues causing declined payments as well as issues with certain fields.

It may be necessary to parse these errors and map them to internal errors at a more granular level than error codes. For example, Error 314 can pertain to any payment field errors. It may be necessary to display problematic errors to the user after parsing the response.

ApiException now takes a $response parameter defaulting to null before initializing the parent Exception. The ApiException's response variable is initialized within the HttpConnector when a response is received.

@grantpalin
Copy link

Thank you for the PR. We would like to use language features where possible, and PHP exceptions do allow for inner exceptions (see http://php.net/manual/en/exception.construct.php). You are welcome to offer an amendment to the SDK base exception class if you wish, else I can code one up.

@mpouris
Copy link
Author

mpouris commented Dec 18, 2015

Thank you for the suggestion. The logic followed for this change was to make available the response in the most appropriate exception.

As an example, the ConnectorException designated a CURL error or an unexpected response format. Whereas an ApiException is thrown when Beanstream's response error codes need to be checked. It seems appropriate to add the response into the ApiException at this point, because there exists a response, whereas there will not be a valid JSON response with a ConnectorException.

In terms of your suggestion to use a previous exception, I also had the same thought. The reason why I was deterred is because it's generally used to chain previously thrown exceptions together. Creating an exception and attaching it as a previous exception without throwing it seems to break the semantics.

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.

2 participants