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

[6.x] Add the exceptionContext method to the exception handler #30780

Merged
merged 1 commit into from
Dec 9, 2019
Merged

[6.x] Add the exceptionContext method to the exception handler #30780

merged 1 commit into from
Dec 9, 2019

Conversation

AbdelElrafa
Copy link
Contributor

@AbdelElrafa AbdelElrafa commented Dec 6, 2019

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.

…eption specific data to be added to the logging context.
@AbdelElrafa AbdelElrafa changed the title Add the exceptionContext method to the exception handler. [6.x] Add the exceptionContext method to the exception handler. Dec 6, 2019
@GrahamCampbell GrahamCampbell changed the title [6.x] Add the exceptionContext method to the exception handler. [6.x] Add the exceptionContext method to the exception handler Dec 6, 2019
@GrahamCampbell
Copy link
Member

I think this would be best aimed at Laravel 7. ;)

@AbdelElrafa
Copy link
Contributor Author

@GrahamCampbell Since it's not a breaking change I thought it should go in 6.x. Why should it target 7?

@GrahamCampbell
Copy link
Member

Because the exception handler has already changed in 7.x, and I don't see the rush to get this into 6.x.

@GrahamCampbell
Copy link
Member

That is, if this is merged at all. I'm not sure it's actually worth adding into the core, since this can already be achieved within apps.

@AbdelElrafa
Copy link
Contributor Author

AbdelElrafa commented Dec 7, 2019

I submitted another PR to 7.x (#30784). I think it can still be merged into 6.x so I'll keep this open to see what Taylor says.

As for "this can already be achieved within apps", in my description I did mention that it can be achieved by overriding the whole report method, which isn't ideal for a app because it can make future updates problematic when something in the report method changes and they are overriding the whole method in their handler. Adding the exceptionContext method allows users a portal into adding context with the exception as a reference. Where as now they only have a portal to add context without access to the exception.

@taylorotwell taylorotwell merged commit 0b41342 into laravel:6.x Dec 9, 2019
@AbdelElrafa AbdelElrafa deleted the allow-context-to-be-added-from-an-exception 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

3 participants