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.

Sign up for GitHub

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

Jump to bottom

Change backtrace arguments to use class name rather than full object #89

Merged
merged 5 commits into from
Aug 14, 2019
Merged

Change backtrace arguments to use class name rather than full object #89

merged 5 commits into from
Aug 14, 2019

Conversation

sixlive
Copy link
Collaborator

@sixlive sixlive commented Aug 8, 2019 •
edited
Loading

Description

This PR fixes a potential issue with v1.6.0 where invalid JSON gets passed to Honeybadger's API. I can't re-create the issue but I have a theory that there is an issue converting some classes to JSON. The library now only passes the class name if a class is a parameter.

Route::get('/', function (Request $request) {
    throwTestError('foo', $request);

    return view('welcome');
});

function throwTestError($something, $classThing)
{
    throw new \Exception('whoops');
}

Screen Shot 2019-08-08 at 9 58 26 AM

Screen Shot 2019-08-08 at 9 58 33 AM

@sixlive sixlive requested a review from joshuap August 8, 2019 13:47
@sixlive sixlive self-assigned this Aug 8, 2019
joshuap
joshuap approved these changes Aug 8, 2019
View reviewed changes
Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

joshuap
joshuap reviewed Aug 8, 2019
View reviewed changes
src/BacktraceFactory.php
@@ -90,11 +90,28 @@ private function formatBacktrace(array $backtrace) : array

return array_merge($context, [
'method' => $frame['function'] ?? null,
'args' => $frame['args'] ?? null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it matters—does the null value behave the same way in the new method?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't. Now it throws:

"Undefined index: args", "/vendor/honeybadger-io/honeybadger-php/src/BacktraceFactory.php", 93

Found during Laravel Forge deployments since updating to honeybadger-laravel 1.7.2
Ref: Faults 54680936 and 54680937
/cc @sixlive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll push a patch today, could you open an issue for it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#90 opened.

@sixlive sixlive changed the title Use class name if argument is an object Change backtrace arguments to use class name rather than full object Aug 13, 2019
@sixlive sixlive mentioned this pull request Aug 13, 2019
Closed
@sixlive sixlive merged commit de8a52a into honeybadger-io:master Aug 14, 2019
@sixlive sixlive deleted the class-backtrace-args branch August 14, 2019 00:04
@drbyte drbyte mentioned this pull request Sep 10, 2019
Closed
@shalvah shalvah mentioned this pull request Feb 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drbyte drbyte drbyte left review comments

@joshuap joshuap joshuap approved these changes

Assignees

@sixlive sixlive

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sixlive @drbyte @joshuap