@tsteur opened this Pull Request on November 18th 2020 Member

Description:

Prevents an issue where it assumes that a command had an important log even when $log->info() is called under circumstances.

This is my DI config:

'log.handlers' => \DI\decorate(function ($handlers, ContainerInterface $container) {
        if (\Piwik\SettingsServer::isArchivePhpTriggered()
            || (\Piwik\Common::isPhpCliMode() && isset($_SERVER['argv']) && in_array('core:archive', $_SERVER['argv']))) {
            $handler = new \Piwik\Plugins\Monolog\Handler\EchoHandler();
            $passthruLevel = \Piwik\Container\StaticContainer::get('log.level');

            $handler->setLevel(\Monolog\Logger::DEBUG);

            $handler = new \Monolog\Handler\FingersCrossedHandler($handler, $activationStrategy = null, $bufferSize = 0,
                $bubble = true, false, $passthruLevel);

            $handlers = array_merge([$handler], $handlers ?: []);
        }
        return $handlers;
    }),

Basically my handlers look like this:

image

What is happening is that FingersCrossedHandler is the first handler and always returns true in $handler->isHandling() therefore the isHandling method on the FailureLogMessageDetector is never called because Monolog stops as soon as one handler is returning true for isHandling see https://github.com/Seldaek/monolog/blob/1.25.5/src/Monolog/Logger.php#L303-L306

By adding this extra line we make sure to only show this message (Error: error or warning logs detected, exit 1 ) and only change exit code if there's actually a warning or error no matter what handlers are defined.

This is needed for the cloud otherwise our exit codes are no longer reliable as many commands simply log an info etc. Eg before $this->logger->info("Archiving stopped by stop archiver exception"); would cause the exit code to be changed to 1 when this should not be the case.

We could maybe workaround it by adding fingerscrossedhandler to the end of handlers but not sure if this has other side effects and this way it always simply works.

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on November 18th 2020 Member

looks good

This Pull Request was closed on November 18th 2020
Powered by GitHub Issue Mirror