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

Provide option to retain full stack traces in ApiErrorException #1561

Closed
Firehed opened this issue Aug 25, 2023 · 2 comments · Fixed by #1562
Closed

Provide option to retain full stack traces in ApiErrorException #1561

Firehed opened this issue Aug 25, 2023 · 2 comments · Fixed by #1562

Comments

@Firehed
Copy link

Firehed commented Aug 25, 2023

Is your feature request related to a problem? Please describe.

The Stripe\ApiErrorException class overrides the default Throwable::__toString() implementation to supplement the exception message with request status and ID information when it's available. This added information is great! However, it means that the default behavior that renders a stack trace is lost, which makes tracking down and debugging errors significantly more challenging.

This can be exacerbated when using a PSR-3 error logger, providing it to exception context value. Such loggers (as well as non-PSR loggers) often rely on __toString(), since the default behavior handles all sorts of fiddly formatting (adding line numbers, checking for and displaying previous exceptions, etc).

Since the Stripe API exceptions override that method, it means that a common setup becomes significantly less valuable and harder to use.

Describe the solution you'd like

Ideally, I'd prefer the original Throwable::__toString() behavior in place by default - though I recognize that this could be disruptive to people that are expecting the current behavior.

Given that, an option for the same would be really beneficial. I could envision this as an option passed when creating a new Stripe\StripeClient or (somewhat less ideal, but possibly more straightforward to implement) something like Stripe\ApiErrorException::setDisplayStackTraces(bool)

Describe alternatives you've considered

If loggers are built in-house, it's possible to modify them to avoid reliance on __toString() and instead rebuild equivalent logic based on the other accessors on Throwables. However, in addition to being a whole lot of redundant work, we'd still have to special-case the Stripe exception to re-add the data provided in the current implementation (if it's so desired).

For third-party loggers, including those bundled with frameworks, this isn't really a practical option.

Additional context

No response

@richardm-stripe
Copy link
Contributor

richardm-stripe commented Aug 28, 2023

Hello @Firehed, thank you for the report.

If you need the trace in __toString() you can subclass StripeClient and wrap the exceptions to add it.

class MyLoggableException extends \Stripe\Exception\ApiErrorException {
  public function __toString() {
    return "MyLoggableException: " . $this->getMessage() . $this->getPrevious()->getTraceAsString();
  }
}

class MyStripeClient extends \Stripe\StripeClient {
  public function request($method, $path, $params, $opts) {
    try {
      return parent::request($method, $path, $params, $opts);
    } catch (\Stripe\Exception\ApiErrorException $e) {
      throw new MyLoggableException($e->getMessage(), 0, $e);
    }
  }
}

$stripe = new MyStripeClient([
...
]);

That said, I don't see much reason why we would override the default and omit the trace from __toString() in the first place, so we'll consider making the change you suggest. I don't think it should be dangerous just to start including the trace. __toString() is for human consumption. Anybody needing to programmatically get the requestId or code should be using the getters e.g. $e->getRequestId().

@richardm-stripe
Copy link
Contributor

This is released in https://github.com/stripe/stripe-php/releases/tag/v12.1.0! Thanks for the feedback @Firehed!

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

Successfully merging a pull request may close this issue.

2 participants