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

Let Laravel decide whether to report or not #97

Merged
merged 1 commit into from
Mar 15, 2016
Merged

Let Laravel decide whether to report or not #97

merged 1 commit into from
Mar 15, 2016

Conversation

philbates35
Copy link

In Laravel 5.2, Illuminate\Foundation\Exceptions\Handler::shouldntReport() was modified to never report Illuminate\Http\Exception\HttpResponseException exceptions. This updated logic was never captured in the BugsnagExceptionHandler, so in a Laravel 5.2 application, all HttpResponseExceptions are reported by default when they shouldn't be.

It makes sense to just use $this->shouldReport($e) to determine if the exception should be reported - that way no logic is duplicated, and if shouldReport() is modified again in the future, there will be no need to update this package.

The shouldReport() method has been present since 5.0 so this change is compatible with 5.0-5.2.

@philbates35
Copy link
Author

Also just saw that this change was proposed in #69, although in that case it was purely to remove repetitive code. I agree with that entirely, but the main point in this PR is to stop HttpResponseExceptions unnecessarily filling up the logs.

@kattrali
Copy link
Contributor

This looks good, @philbates35. Thank you for the contribution. Merging.

kattrali added a commit that referenced this pull request Mar 15, 2016
Let Laravel decide whether to report or not

Closes #69
@kattrali kattrali merged commit 9c4f2a3 into bugsnag:master Mar 15, 2016
@browner12
Copy link
Contributor

glad it got merged. thanks!

@philbates35
Copy link
Author

@kattrali Any chance we can get a new tagged release containing this fix? It's been a while, and my Bugsnag is littered with HttpResponseException's! Thanks.

@philbates35
Copy link
Author

@GrahamCampbell I've seen you've been working on this library in the last couple of days. Are you planning on tagging a new release soon? The HttpResponseExceptions are becoming annoying. I know I could use dev-master but as it's been months since the last release a new release would be preferable.

@GrahamCampbell
Copy link
Contributor

I'll add a branch alias here for you so you don't have to use dev-master. I'll look into getting a release soon too for you. :)

@GrahamCampbell
Copy link
Contributor

You should be able to get the current version using the constraint ^1.7, or ^1.7@dev, if your minimum stability is set to stable. Please let me know if you see any issues. :)

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.

4 participants