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

[7.x] Add the exceptionContext method to the exception handler #30784

Closed
wants to merge 1 commit into from
Closed

[7.x] Add the exceptionContext method to the exception handler #30784

wants to merge 1 commit into from

Conversation

AbdelElrafa
Copy link
Contributor

This PR will add the exceptionContext method to the exception handler to allow exception specific data to be added to the logging context.

Currently if you need to get context off of an exception to pass it to the logger you would need to override the whole report method and do it there.

Adding this method allows us to add context off of an exception by simply implementing the method in the applications Handler class. This is not a breaking change.

Example:

throw (new CustomException())->setCustomProperty('foo');

// App/Exceptions/Handler.php
protected function exceptionContext(Exception $e)
{
    if ($e instanceof CustomException) {
        return ['custom_context' => $e->getCustomProperty()];
    }
}

Now the logger can get the custom exception context and handle it just like it handles the other context.

@driesvints
Copy link
Member

@AbdelElrafa can you rebase with master? Tests on master have been fixed.

@GrahamCampbell
Copy link
Member

I think we also need tests for this feature too, so that it doesn't get broken in the future.

…eption specific data to be added to the logging context.
@AbdelElrafa
Copy link
Contributor Author

@driesvints I rebased.

@GrahamCampbell I had looked into it but dismissed the tests because I didn't find any tests for the context method. The only test that's there is to test that the exception is passed in the context. Let me know if I still should find a way to test it.

@taylorotwell
Copy link
Member

I'll just handle this when I merge 6.x forward this week.

@taylorotwell
Copy link
Member

taylorotwell commented Dec 9, 2019

Hey @AbdelElrafa, after looking at this again. I think my intention was that you could already override the context method and return array_merge(parent::context(), ['my-context' => 'foo']). Does that not work for you?

@taylorotwell
Copy link
Member

Ah, never mind. You are wanting exception specific context.

@AbdelElrafa AbdelElrafa deleted the master branch December 9, 2019 15:09
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.

None yet

4 participants