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

Add support for code attribute on all Stripe exceptions #450

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe
cc @stripe/api-libraries

PHP does not support overloading constructors, so I've had to resort to a slightly hackish solution to avoid modifying the constructor's signature and preserve BC. There was already a precedent for it though, for the decline_code attribute on card errors:

// This one is not like the others because it was added later and we're
// trying to do our best not to change the public interface of this class'
// constructor.
// TODO: make this a proper constructor argument in the next major
// release.
$this->declineCode = isset($jsonBody["error"]["decline_code"]) ? $jsonBody["error"]["decline_code"] : null;

@brandur-stripe
Copy link
Contributor

LGTM.

I think for the next breaking release we should possibly instead consider just changing the constructor to take one associative array parameter ... real arguments are usually better, but when you have this many and so little typing, I'm not sure if there's any real advantage over simulating named parameters with an array.

It's too bad — Googling around a bit it seems like there's a fair bit of demand for named parameters in the language, but the only format RFC for it seems to have stalled out: https://wiki.php.net/rfc/named_params

@ob-stripe
Copy link
Contributor Author

Yeah, good point. We could also simply pass the HTTP status code and the JSON body, and deduce all the other attributes from that. It's probably a little unconventional to do anything but store some values in an exception constructor, but since our exceptions are basically models for our errors, it kind of makes sense that they'd know how the JSON is structured.

@ob-stripe ob-stripe merged commit 54c9fac into master Feb 23, 2018
@ob-stripe ob-stripe deleted the ob-error-code branch February 23, 2018 17:31
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