-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: remove unnecessary $isUncaught param #518
Conversation
When the application logging handles an otherwise uncaught error, exception, or fatality, the logging path enters the "uncaught" flow. The implementation for the uncaught flow went through the usual logging mechanism (PSR-3, `log()`) but appended the uncaught state value to the `log()` signature. While not a violation of PHP LSP rules, it nonetheless could be an error when Rollbar is used as the handler, but a user-defined logger with a different `log()` API is shimmed in-front of the Rollbar logger. For broadest compatibility, and keeping with the principal of least surprise, we remove that extra parameter and instead pass the state information as part of the exception object itself. BREAKING CHANGE
/** | ||
* @since 3.0.0 | ||
*/ | ||
public static function logUncaught($level, Throwable $toLog, $extra = array()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about this method.
It makes for an easy upgrade path: code doing Rollbar::log($level, $data, $context, true)
just gets rewritten to Rollbar::logUncaught($level, $data, $context)
, provided that the $data
is an instance of Throwable
(which it must be, since the only "uncaught" things derive logically from Throwable
). However, if no-one is using Rollbar::log
with the fourth parameter, then this is just code bloat.
Without metrics, I'm erring on the side of caution and providing an upgrade path for the breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was at least one person who intended to use it 3 years ago. #314
In general the SDKs allow specifying "uncaught", and it's a common pattern to use this flag in a check ignore handler, if not elsewhere. It makes sense to continue to provide a way to set it.
I'd agree this should only apply to Throwable. Hopefully no one had been using it differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, on the basis that this is treated as a breaking change.
@bishopb I didn't notice this PR until now (10 days too late!), but the one comment I'll have is that we're adding a new property,
|
@jamesaspence True! It's kind of gross as implemented, but I did consider the decorator. I decided against it for two reasons. First, concision: this property is an internal detail, whose line-of-code existence is very short and well-defined. Where it touches the user, it's by a well-documented API ( Your proposed implementation looks good, and we're open to all changes that improve the code, so feel free to carry it forward if you'd like. |
Description of the change
When the application logging handles an otherwise uncaught error, exception, or fatality, the logging path enters the "uncaught" flow. The implementation for the uncaught flow went through the usual logging mechanism (PSR-3,
log()
) but appended the uncaught state value to thelog()
signature.While not a violation of PHP LSP rules, it nonetheless could be an error when Rollbar is used as the handler, but a user-defined logger with a different
log()
API is shimmed in-front of the Rollbar logger.For broadest compatibility, and keeping with the principal of least surprise, we remove that extra parameter and instead pass the state information as part of the exception object itself.
BREAKING CHANGE
Type of change
Related issues
Checklists
Development
Code review