TomorrowIdeas / plaid-sdk-php

PHP bindings for the Plaid API
MIT License
111 stars 42 forks source link

Implement Plaid's API Errors #41

Closed stephenjude closed 3 years ago

stephenjude commented 3 years ago

I tried using this Plaid SDK and I ran into BadRequest exception that doesn't give full error details and received from Plaid. I had to debug the code and found out that the exceptions were not implemented properly.

I went through the Plaid Error Page and I have implemented all the errors.

I believe this will really help everyone who uses this SDK.

stephenjude commented 3 years ago

@brentscheffler you can review this PR now. I have fixed all the failing tests.

brentscheffler commented 3 years ago

@stephenjude Thanks, I'll spend some time looking over this.

brentscheffler commented 3 years ago

First, this PR would introduce a major breaking change with the deletion of the PlaidRequestException class. Secondly, can you describe what problem this solves?

stephenjude commented 3 years ago

@brentscheffler the problem this PR solves is displaying clear error the way Plaid sent it so developers can easily locate what exactly is wrong with their request.

Yester day I was seeing a BadRequest error but there was no detail of the error. I got stock over hours until I went into the package itself inside the vendor folder to dump error only to find out that PlaidRequestException class didn't return the error_type, the error_code nor the error_message sent from Plaid endpoints.

For example: I sent a token request specifying no plaid product and it returned an error response.

Here is a screenshot of PlaidRequestException

Screenshot 2021-08-20 at 11 39 30 AM

Here is a screenshot of the implementation on my PR

Screenshot 2021-08-20 at 11 58 48 AM
brentscheffler commented 3 years ago

There is a getResponse() method on the PlaidRequestException instance that returns the complete deserialized response body from the error returned by Plaid. You can use that to determine the error type, the detailed message, etc. Additionally, the PlaidRequestException will have its code set to the HTTP response status code. For example, a 400 Bad Request would set the exception code to 400.

try {
    $link = $plaid->tokens->create(
        "AcemeWidgets",
        "zz",
        ["US"],
        new User("84b54a38-6e88-4262-87aa-d62b24dbe490"),
        ["auth", "transactions"],
        "https://link.example.com/"
    );
}
catch( PlaidRequestException $e ){
    dd($e->getResponse());
}

Would produce:

^ {#10
  +"display_message": null
  +"documentation_url": "https://plaid.com/docs/#create-link-token"
  +"error_code": "INVALID_FIELD"
  +"error_message": "provided language is unsupported"
  +"error_type": "INVALID_REQUEST"
  +"request_id": "Uc6hTsmrcaftJra"
}
brentscheffler commented 3 years ago

I would be open to adding getter methods on the PlaidRequestException for each of the properties of the error response. For example: getDisplayMessage(), getDocumentationUrl(), etc. That would not introduce a breaking change to the current behavior, simply adding new functionality.

However, as it stands, I will not merge this PR at this point.

stephenjude commented 3 years ago

Okay. I I don't really understand how you want it to be implemented mean while I will maintain the forked version.

stephenjude commented 3 years ago

Exceptions have properties. I don't think anyone would be looking for getResponse() method inside an exception class.

Valid exception methods are

        $exception->getMessage();
        $exception->getCode();
        $exception->getTrace();
        $exception->getFile();
        $exception->getLine();
        $exception->getPrevious();
        $exception->getTraceAsString();