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
GitHub Issue #166: capture_error_stacktraces is not used #192
GitHub Issue #166: capture_error_stacktraces is not used #192
Conversation
…al vars dump config
Ready for review. |
@@ -84,7 +84,19 @@ private static function getScrubFields() | |||
|
|||
public function sendMessageTrace($sendMessageTrace = null) | |||
{ | |||
return $sendMessageTrace ? $sendMessageTrace : $this->defaultSendMessageTrace; | |||
return $sendMessageTrace !== null ? $sendMessageTrace : $this->defaultSendMessageTrace; |
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.
👍
src/Defaults.php
Outdated
|
||
public function localVarsDump($localVarsDump = null) | ||
{ | ||
return $localVarsDump ? $localVarsDump : $this->defaultLocalVarsDump; |
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.
Shouldn't we follow the same pattern and use
return $localVarsDump !== null ? $localVarsDump : $this->defaultLocalVarsDump;
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.
It doesn't particularly matter considering the default is false
, but if we wanted to change that in the future this would matter.
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 changed this. $localVarsDump ? $localVarsDump : $this->defaultLocalVarsDump
is buggy
if ($this->captureErrorStacktraces) { | ||
$backTrace = array_slice( | ||
debug_backtrace($this->localVarsDump ? 0 : DEBUG_BACKTRACE_IGNORE_ARGS), | ||
2 |
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.
Is this still 2 or should it be 3? I am not sure if we added a level of indirection here.
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.
Just tested this. Slicing at 2 still produces correct trace depth.
LGTM, a couple minor questions but this looks basically good to go. |
Implementation of
capture_error_stacktraces
in 1.2.0.This PR is based on PR #178 to pull in changes to
generateErrorWrapper