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

Add support for laravel 5.6 log channels #122

Merged

Conversation

patrickbrouwers
Copy link
Contributor

As #119 states Laravel 5.6 adds a nice new way of having log channels.

Integration would be as easy as:

'sentry' => [
    'driver' => 'sentry',
    'level'  => 'notice',
],

@antoniopaisfernandes
Copy link

antoniopaisfernandes commented Feb 10, 2018

Please replace $this->app::VERSION with $app::VERSION for Travis sake :)

$app = app();

@dustingraham
Copy link

Laravel 5.6 requries PHP ^7.1.3 hence the travis failure.

@stayallive
Copy link
Collaborator

Hey @patrickbrouwers, thanks for taking the time to create this PR 👍 sorry it took so long to get here :)

I am reading through the docs for custom log channels and seeing this:

If you would like to define an entirely custom channel in which you have full control over Monolog's instantiation and configuration, you may specify a custom driver type in your config/logging.php configuration file. Additionally, your configuration should include a via option which specifies the class that should be invoked to create the Monolog instance:

https://laravel.com/docs/5.6/logging#creating-custom-channels

Wouldn't it be nicer to simply have a \Sentry\SentryLaravel\SentryLogChannel::class that a user can add using the custom driver?

@patrickbrouwers
Copy link
Contributor Author

@stayallive that's also an option, but I'd personally prefer to enter a short string there. Also looking at how other packages handle this (https://docs.bugsnag.com/platforms/php/laravel/#reporting-unhandled-exceptions) it seems that's kind of the way to go.

@stayallive
Copy link
Collaborator

stayallive commented Apr 24, 2018

@patrickbrouwers 👍

Would you be willing to rename CreateSentryLogChannel to SentryLogChannel?

Apart from that LGTM! Thanks again!

@patrickbrouwers
Copy link
Contributor Author

@stayallive I can if you want, but it's not really a channel, but more of a factory. Let me know what you prefer.

@stayallive
Copy link
Collaborator

Yes, you are totally right. However I think in the Laravel context this a how a channel is defined. So that's why Create sounds a bit weird to me.

Also for people (for whatever reason) using the class directly in their config it would "look" better without the Create prefix IMHO.

So my vote would be to leave it out.

@patrickbrouwers
Copy link
Contributor Author

@stayallive , sure! I changed it.

@patrickbrouwers
Copy link
Contributor Author

@stayallive not sure why cs-fixer fails for php5.6 ?

@stayallive
Copy link
Collaborator

stayallive commented Apr 24, 2018

You branch was a bit behind with older Travis / dependency thingies (I thought) 😄

It was phpcs not liking the ?? operator being used 😅

@patrickbrouwers
Copy link
Contributor Author

At least we have good old fashion isset checks still :)

@stayallive stayallive merged commit a360980 into getsentry:master Apr 24, 2018
@stayallive
Copy link
Collaborator

Cool, that worked, will release this in the coming days (release for sentry-php base package also incoming, will release at the same time).

@patrickbrouwers
Copy link
Contributor Author

@stayallive thanks!

@patrickbrouwers patrickbrouwers deleted the laravel-56-log-channels-support branch April 24, 2018 11:23
@royduin
Copy link

royduin commented Jul 7, 2018

Nice, but who's going to update the readme? It's still mentioning the "old" way...

@stayallive
Copy link
Collaborator

@royduin what do you mean? It is documented...? Or is it missing things?

https://github.com/getsentry/sentry-laravel#using-laravel-56-log-channels

@royduin
Copy link

royduin commented Jul 7, 2018

Lol, didn't saw it in this PR. I didn't scroll all the way down, sorry! And thanks 😎

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.

5 participants