Separate core error logging from standard LoggerInterface

[robbieaverill]

Up for discussion!

At a high level:

• Rename the existing error handler implementation
• Register the default handler implementation as LoggerInterface for general purpose use
• Allow MonologErrorHandler to accept multiple loggers (includes deprecation of setHandler in favour of pushHandler)
• Update documentation, fix some typos in related docs/code

I've tested the following situations:

• Injector::inst()->get(LoggerInterface::class)->debug('Hello') levels are all logged to whatever handlers are defined for LoggerInterface (default: none)
  • Tested with file stream logger configured
  • Tested with "Messages" tab log collection in lekoala/silverstripe-debugbar
• Throwing an exception is handled by the SilverStripe error handler (CLI/browser)
• Throwing an exception is sent to the defined loggers for LoggerInterface (e.g. if using a file/stream logger)
• Logging PSR-3 errors via LoggerInterface doesn't get included in SilverStripe's error handlers
• Explicit log entries via LoggerInterface.errorhandler do get included in SilverStripe's error handler (previously the default behaviour)

Considerations:

• Do we want to provide a NoopLoggerTestState test state in framework that is configured by default to replace any custom log handlers that user code has defined, e.g. file logs? We could push a NullHandler by default

Resolves #8044

[dhensby]

Do we want to provide a NoopLoggerTestState test state in framework that is configured by default to replace any custom log handlers that user code has defined, e.g. file logs? We could push a NullHandler by default

quite possibly, yes - but maybe it would be good to actually log the test logs somewhere?

I'd probably just log it to standard out...

[robbieaverill]

quite possibly, yes - but maybe it would be good to actually log the test logs somewhere?
I'd probably just log it to standard out...

The problem with that is that your phpunit runs fill up with stdout log calls from various parts of the codebase.

How about we configure a NullHandler for unit tests by default, which devs can remove via config if they want to. We might even be able to use the --verbose flag from phpunit to control this as well...

[robbieaverill]

Rebased on top of 4 since there were some changes underneath. I've also added a ->setLoggers(array $loggers) method in case someone wants to clear them

[maxime-rainville]

I like the change conceptually.

Did not review the detail of the PR or try running it locally. If there's a consensus this is a good thing, I'd be keen to peer review it.

[robbieaverill]

Thanks for having a look. Remaining would be to add something for unit test logging config/handlers. Not sure it'd block review and merge of this though - it could be done as a follow up

[sminnee]

Just going to flag that right now logging is broken if you use Raygun. Gonna fix that with some bandaids but we should get this into 4.4!

[robbieaverill]

Thanks @sminnee. To recap, I think the follow up action required is to decide whether to automatically configure a null handler for user configured logging when running unit tests (override user configured handlers) or to log test output somewhere by default, e.g. a /var/tmp/silverstripe-tests.log file or something like that.

I don't think stdout logging is a good idea TBH because it'll pollute phpunit test results.

Can we get a vote on it from @silverstripe/core-team?

🚀 : disable test logging by default
👀 : enable test logging to a file by default

[robbieaverill]

Two weeks for a vote on that, seems like the consensus is to remove (user configured project) logging from tests by default.

[ScopeyNZ]

Looks like there's general good thoughts towards this PR in 4.4. I'll merge it now then :)