-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[5.6] Rework Logging #22635
[5.6] Rework Logging #22635
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I agree on a watch
method – I don't see why our application should react to any kind of log message, instead you'd react to an exception thrown or similar. Can you explain the use-case for watch
src/Illuminate/Log/LogManager.php
Outdated
return tap($this->createEmergencyLogger(), function ($logger) use ($e) { | ||
$logger->emergency('Unable to create configured logger. Using emergency logger.'); | ||
|
||
$logger->emergency($e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be advisable to pass the exception in the context as monolog provides special exception rendering:
$logger->emergency('Unable to create configured logger. Using emergency logger.', ['exception' => $e]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK, Thanks
foreach ($this->configurationFor($name)['tap'] ?? [] as $tap) { | ||
list($class, $arguments) = $this->parseTap($tap); | ||
|
||
$this->app->make($class)->__invoke($logger, ...explode(',', $arguments)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we should call __invoke()
manually here, seems dirty if we're not using __invoke
for its purpose (treating an object as a method essentially), might as well use another method name then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a method name suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle
would follow the middleware standard. process
or execute
could be options too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The why I learnt to expect how handle
works, is that it's arguments were resolved by the Container and injected (think: Console commands and Jobs).
Could you include an example of what the new config file will look like @taylorotwell? |
Is it possible to keep the old Writer class, mark it deprecated, and have it forward calls to the new code? Just to give people some time to migrate their old code? |
@sisve I don’t plan on doing that. The migration isn’t large enough to warrant it. |
Laravel 5.5 LTS will still be around after 5.6 is released, so people have some time if they need it. |
@phroggyy I'm fine with removing |
Here is what the basic logging configuration file would look like: <?php
return [
'default' => env('LOG_CHANNEL', 'single'),
'channels' => [
'single' => [
'driver' => 'single',
'tap' => [App\Log\ConfigureLogger::class],
'path' => storage_path('logs/laravel.log'),
'level' => 'debug',
],
'daily' => [
'driver' => 'daily',
'path' => storage_path('logs/laravel.log'),
'level' => 'debug',
'days' => 3,
],
'syslog' => [
'driver' => 'syslog',
'level' => 'debug',
],
'errorlog' => [
'driver' => 'errorlog',
'level' => 'debug',
],
],
]; |
Removed |
This is a re-write / re-work of many of @phroggyy's ideas to improve logging.
Implemented:
configureMonologUsing
maintained usingcustom
driver type withfactory
invokable that returns any PSR3 compatible logger. This allows full customization of entire logging channel.I decided not to implement
Log::event
functionality from PR because I think that would be better suited for something like aShouldLog
interface on the event itself (to complementShouldBroadcast
, etc.).Primary breaking changes:
configureMonologUsing
nowcustom
driver type.Illuminate\Contracts\Log
removed. Was literal duplication of PSR3 logging interface.Illuminate\Log\Writer
renamed toIlluminate\Log\Logger
.