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

[5.6] Rework Logging #22635

Merged
merged 17 commits into from
Jan 4, 2018
Merged

[5.6] Rework Logging #22635

merged 17 commits into from
Jan 4, 2018

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Jan 4, 2018

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.

Copy link
Contributor

@phroggyy phroggyy left a 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

return tap($this->createEmergencyLogger(), function ($logger) use ($e) {
$logger->emergency('Unable to create configured logger. Using emergency logger.');

$logger->emergency($e);
Copy link
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
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
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
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
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
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
Copy link
Contributor

phroggyy commented Jan 4, 2018

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
Copy link
Contributor

sisve commented Jan 4, 2018

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
Member Author

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

@GrahamCampbell
Copy link
Member

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
Member Author

@phroggyy I'm fine with removing watch method.

@taylorotwell
Copy link
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
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