taxamo / taxamo-php

Other
10 stars 7 forks source link

Exceptions should use an standard output to let programmers handle them #3

Closed ghost closed 9 years ago

ghost commented 9 years ago

I'm developing an integration of Taxamo API using this library.

When creating a transaction, if Taxamo API returns an error code (40x), Swagger throws an exception in a different format for each http code, so I can't program a way to know the error, because I don't even know it was a 400, 401 or 404 error because this information is not included in the Exception message.

I'm talking about Swagger.php file, lines 105 to 123.

tlipski commented 9 years ago

I suppose it is 400 error, which results in invalid input. Can you please provide the exact example input you're using?

Does the example from https://github.com/taxamo/taxamo-php/blob/master/test/TaxamoTest/TestTransactionsApi.php work for you?

danieliser commented 9 years ago

Hey tlipski, I experienced something similiar but self closed the issue this week. Look there for my example. But basically by returning a general error we can't error check without dumping the initial curl response.

Essentially my issue was that I was missing a parameter, but the general error just said API connection failed. Once I dumped the curl response the error was clear as it said exactly what was missing.

So his suggestion as was mine in the other issue, is to not handle the errors in swagger beyond triggering one. Pass the response through an exception and let the developer decide how to handle it.

This would have allowed me to build in error checking and possibly self correction if the needed info was available.

tlipski commented 9 years ago

ok, so to clarify a little bit - during my tests, I have triggered validation error with the following code:

$resp = $this->getApi()->createTransaction(array());

and the exception thrown was something like this:

Unexpected exception of type [Exception] with message [Validation error for https://api.taxamo.com/api/v1/transactions: {"errors":["Validation failed: ","{:transaction missing-required-key}"],"validation_failures":{"transaction":"missing-required-key"}}post data:[]] in [/Users/tomek/d/taxamo-php/lib/Taxamo/Swagger.php line 119]

So actually the whole error 400 response body received was returned and it contains quite verbose (to me at least) response with what's going on. The API 400 response actually uses JSON-like structure to pinpoint the actual point of error when validating input format.

For example, if we use unknown currency - which requires some simple business logic to check against, we're getting the exception as well:

Unexpected exception of type [Exception] with message [Validation error for https://api.taxamo.com/api/v1/transactions: {"errors":["Unknown currency."]}post data:{"transaction":{"billing_country_code":"PL","currency_code":"ZZZ","transaction_lines":[{"custom_id":"line1","amount":"200"}]}}] in [/Users/tomek/d/taxamo-php/lib/Taxamo/Swagger.php line 119]

Of course, if you see any way to improve error reporting in PHP lib, I'm open to all ideas.

danieliser commented 9 years ago

My specific issue was creating a payment, but the amount was passed as a string not float. This triggered a generic message resulting in me having to var_dump the curl response to get an error message that taxamo passed back stating that the amount was improper.

I see that some of your errors do include the response errors, but some like the last do not.

But that said your errors are passed back in string format. You really should pass pass back the json object returned by the api. IE We need to add a custom exception handler that accepts a second parameter for the object.

Otherwise we have to parse the string response into a json string and then decode it to be able to use it properly. In your examples on our end we could simply check the errors for specific things.

But the error handling should ultimately be handled by the developer using this in their project. Some errors should be logged, some displayed to users etc. Let the dev handle them by catching custom exceptions.

tlipski commented 9 years ago

OK, the situation you describe (string in amount field) should get a clearer error. Unfortunately, I am unable to re-create it - could you provide an example code?

We can also parse JSON returned as validation error it that will be helpful.

danieliser commented 9 years ago

i simply had a price stored in a variable like $price = '2.99';

When really the API requires $price = 2.99; || $price = floatval('2.99');

The thing is you would need seperate error handling for each call, could get complex.

But as you said, passing back the json error object would allow further processing by the developer. At least then we can be sure that when an error occurs we know the full details.

tlipski commented 9 years ago

I will add the JSON parsing, but I am still unable to re-create the issue with price as string. Actually when I set the price to '200' or '200.99' it works OK:

    $transaction_line1 = new Input_transaction_line();
    $transaction_line1->amount = '200';
    $transaction_line1->custom_id = 'line1';
tlipski commented 9 years ago

I have added exception hierarchy and json response parsing in version 1.0.11 (commit b9e8831273992b8bf9729107726aa99fad33f6a1)

But still - I am unable to fix reported issue without the example code that caused the errors to manifest in such matter.

ghost commented 9 years ago

Thank you for this modification... now, based on the exception type, I am able to program a better way to deal with the errors.

tlipski commented 9 years ago

ok, so I assume that this solves this issue.