silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Error Logging is extremely frustrating in version 4+ #10158

Open clarkepaul opened 2 years ago

clarkepaul commented 2 years ago

Affected Version

V4+

Description (Feedback provided by community member)

Error Logging is extremely frustrating in version 4 and above, I have read some tutorials, copied some code, some errors log, some errors email, but it's completely hit and miss. How about an option to log everything, or email everything, instead of the way it is. Even in dev mode, why not display errors? It seems I can only do by putting a die after every line until I find which line isn't working.

How about keeping it simple: SS_ERROR_LOG="/var/log/silverstripe.log" logs an error to /var/log/silverstripe.log?

xini commented 2 years ago

Could not agree more. This is in my /app/_config.php to make logging configurable in .env:

if ($errorLog = Environment::getEnv('CUST_ERROR_LOG')) {
    ini_set("error_log", BASE_PATH . DIRECTORY_SEPARATOR . $errorLog);
    $logger = Injector::inst()->get(LoggerInterface::class);
    $logger->pushHandler(new StreamHandler(BASE_PATH . DIRECTORY_SEPARATOR . $errorLog, Logger::ERROR));
}
if ($errorReporting = Environment::getEnv('CUST_ERROR_REPORTING')) {
    $errorReporting = preg_replace('/[^A-Z_&~| ]/', '', $errorReporting);
    ini_set("error_reporting", $errorReporting);
}
if ($logErrors = Environment::getEnv('CUST_LOG_ERRORS')) {
    ini_set("log_errors", $logErrors);
}
if ($displayErrors = Environment::getEnv('CUST_DISPLAY_ERRORS')) {
    ini_set("display_errors", $displayErrors);
}
if ($emailErrors = Environment::getEnv('CUST_EMAIL_ERRORS_TO')) {
    $logger = Injector::inst()->get(LoggerInterface::class);
    if ($logger instanceof Logger) {
        $handler = new NativeMailerHandler(
            $emailErrors,
            'Error on ' . Director::absoluteBaseURL(),
            Email::config()->admin_email,
            Logger::ERROR
        );
        $handler = $handler->setContentType('text/html');
        $handler = $handler->setFormatter(new DetailedErrorFormatter());
        $logger->pushHandler($handler);
    } else {
        trigger_error("CUST_EMAIL_ERRORS_TO setting only works with Monolog, you are using another logger", E_USER_ERROR);
    }
}
if ($debugLog = Environment::getEnv('CUST_DEBUG_LOG')) {
    $logger = Injector::inst()->get(LoggerInterface::class);
    $logger->pushHandler(new StreamHandler(BASE_PATH . DIRECTORY_SEPARATOR . $debugLog, Logger::DEBUG));
}
NightJar commented 2 years ago

How about keeping it simple: SS_ERROR_LOG="/var/log/silverstripe.log" logs an error to /var/log/silverstripe.log?

SS_ERROR_LOG still works. The only difference to the described behaviour is that the log file is limited/relative to the installation directory as a root (c.f. $this->basePath in the above link). It seems like this has always been the case.

The runner (e.g. webserver) must have write access to the identified file (this is nothing new), this can cause expected setups to fail.

Even in dev mode, why not display errors?

Error reporting is enabled in dev mode. If the application is unable to set the logging level, then this is a system configuration issue and Silverstripe Framework cannot do anything about it (as with file permissions).


What is it you are trying to achieve with that level of customisation @xini ? Silverstripe Framework attempts to make no alteration to PHP settings error_log or log_errors. ErrorControlChain does some weird hard to understand things with display_errors I'll admit - are you experiencing it unexpectedly blocking output?

I understand Injector via Config can be a little confusing at times, but I've always found it to work as per the documentation, which isn't too overly verbose. This desired level of customisation usually requires either a comparible level of configuration or code as per the last few lines in your example, which again is neither too many lines, nor too hard to understand.

Is there something I'm missing? A deeper but not too uncommon use case that shouldn't be as difficult as it is?


ErrorControlChain is to be removed in the upcoming CMS 5 release, so there's that, anyway :)

xini commented 2 years ago

Thank @NightJar for your reply.

I use CUST_ERROR_LOG because the built-in SS_ERROR_LOG logs warnings as well, not just errors, see https://github.com/silverstripe/silverstripe-framework/blob/4.0.0/src/Core/CoreKernel.php#L532-L536.

I have built the _config.php snippet to unify all settings around error handling in one place, .env. I can set error and debug log files, I can set the PHP reporting level, I can get errors emailed, etc. It might look complicated, but it really makes error handling for a project so much easier if you have everything in one place.

I liked the simplicity of the SS3 logging settings, especially the comparison parameter of SS_Log::add_writer() where you could set what errors you wanted to include: https://github.com/silverstripe/silverstripe-framework/blob/3/dev/Log.php#L140.

I don't think there is something missing with the current implementation. It's just not quite as simple to configure.

Silverstripe is currently using Monolog 1.x. Is SS5 going to upgrade to Monolog 2 or 3?

NightJar commented 2 years ago

I see, that's interesting 👍 It's a pity the StreamHandler isn't also fetched via Injector, so Config (or an environment variable if set) could control the log level.

I would hope that all dependencies get upgraded in CMS 5, yes. The StripeConEU talk from Max makes it sound that way, in the least (not too much else chanigng other than removing flags for BC support).

I agree that Monolog can take a little bit go get ones head around with regards to streams, transports, etc. and the relationship all the parts have. Layered on top is how Injector works Layered on top is how Config works

Ultimately though one is a very ubiquitious system and the other two are core concepts on how Silverstripe operates. I feel that time invested learning the intracasies won't be wasted.

Instead of relying on SS_ERROR_LOG many projects use config instead to set this as part of their skeleton/installer setup.