Skip to content

[5.6] Rework Logging#22635

Merged
taylorotwell merged 17 commits into
5.6from
logging
Jan 4, 2018
Merged

[5.6] Rework Logging#22635
taylorotwell merged 17 commits into
5.6from
logging

Conversation

@taylorotwell

@taylorotwell taylorotwell commented Jan 4, 2018

Copy link
Copy Markdown
Member

This is a re-write / re-work of many of @phroggyy's ideas to improve logging.

Implemented:

  • Multi-driver logging support so you can have multiple logging channels per app.
  • "Pipes" in @phroggyy's pull request have been re-worked as "taps". Allows customization of logging channel post-creation.
  • Continued support for functionality provided by configureMonologUsing maintained using custom driver type with factory 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 a ShouldLog interface on the event itself (to complement ShouldBroadcast, etc.).

Primary breaking changes:

  • New log configuration file / settings.
  • configureMonologUsing now custom driver type.
  • Illuminate\Contracts\Log removed. Was literal duplication of PSR3 logging interface.
  • Illuminate\Log\Writer renamed to Illuminate\Log\Logger.

@phroggyy phroggyy left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread 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);

Copy link
Copy Markdown
Contributor

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]);

Copy link
Copy Markdown
Member Author

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));

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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).

@phroggyy

phroggyy commented Jan 4, 2018

Copy link
Copy Markdown
Contributor

Could you include an example of what the new config file will look like @taylorotwell?

@GrahamCampbell GrahamCampbell changed the title Rework Logging [5.6] Rework Logging Jan 4, 2018
@sisve

sisve commented Jan 4, 2018

Copy link
Copy Markdown
Contributor

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?

@taylorotwell

Copy link
Copy Markdown
Member Author

@sisve I don’t plan on doing that. The migration isn’t large enough to warrant it.

@GrahamCampbell

Copy link
Copy Markdown
Collaborator

Laravel 5.5 LTS will still be around after 5.6 is released, so people have some time if they need it.

@taylorotwell

Copy link
Copy Markdown
Member Author

@phroggyy I'm fine with removing watch method.

@taylorotwell

Copy link
Copy Markdown
Member Author

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',
        ],
    ],

];

@taylorotwell

Copy link
Copy Markdown
Member Author

Removed watch method and made other recommended changes.

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