Fix logger injection in DIC and handling and logging

dependency_injection/services.php

@@ -36,7 +36,6 @@
 
 use Glpi\DependencyInjection\PublicService;
 use Glpi\Error\ErrorHandler;
-use Glpi\Log\LegacyGlobalLogger;
 
 return static function (ContainerConfigurator $container): void {
     $projectDir = dirname(__DIR__);
@@ -66,5 +65,5 @@
      * Override Symfony's logger.
      * @see \Symfony\Component\HttpKernel\DependencyInjection\LoggerPass
      */
-    $services->set('logger', LegacyGlobalLogger::class);
+    $services->set('logger')->synthetic();
 };

phpunit/functional/Glpi/Exception/LogLineFormatterTest.php

@@ -34,7 +34,7 @@
 
 namespace tests\units\Glpi\Exception;
 
-use Glpi\Error\LogLineFormatter;
+use Glpi\Log\LogLineFormatter;
 use PHPUnit\Framework\TestCase;
 
 class LogLineFormatterTest extends TestCase

src/CommonGLPI.php

@@ -1197,7 +1197,6 @@ public function getHeaderName(): string
      */
     public function display($options = [])
     {
-        trigger_error('Error triggerred', E_USER_WARNING);
         // Item might already be loaded, skip load and rights checks
         $item_loaded = $options['loaded'] ?? false;
         unset($options['loaded']);

src/GLPI.php

@@ -33,7 +33,7 @@
  * ---------------------------------------------------------------------
  */
 
-use Glpi\Error\LogLineFormatter;
+use Glpi\Log\LogLineFormatter;
 use Monolog\Handler\StreamHandler;
 use Monolog\Logger;
 use Psr\Log\LogLevel;
@@ -66,50 +66,4 @@ class GLPI
      */
     public const ENV_DEVELOPMENT = 'development';
 
-    /**
-     * Init logger
-     *
-     * @return void
-     */
-    public function initLogger()
-    {
-        $errorHandler = new \Glpi\Error\ErrorHandler();
-        $errorHandler::register($errorHandler);
-
-        /**
-         * @var \Psr\Log\LoggerInterface $PHPLOGGER
-         */
-        global $PHPLOGGER;
-
-        if (defined('GLPI_LOG_LVL')) {
-            $log_level = GLPI_LOG_LVL;
-        } else {
-            switch (GLPI_ENVIRONMENT_TYPE) {
-                case self::ENV_DEVELOPMENT:
-                    // All error/messages are logs, including deprecations.
-                    $log_level = LogLevel::DEBUG;
-                    break;
-                case self::ENV_TESTING:
-                    // Silent deprecation and info, as they should have no functional impact.
-                    // Keep notices as they have may indicate that code is not correctly handling a specific case.
-                    $log_level = LogLevel::NOTICE;
-                    break;
-                case self::ENV_STAGING:
-                case self::ENV_PRODUCTION:
-                default:
-                    // Keep only warning/error messages.
-                    $log_level = LogLevel::WARNING;
-                    break;
-            }
-        }
-
-        $PHPLOGGER = new Logger('glpiphplog');
-        $handler = new StreamHandler(
-            GLPI_LOG_DIR . "/php-errors.log",
-            $log_level
-        );
-        $handler->setFormatter(new LogLineFormatter());
-
-        $PHPLOGGER->pushHandler($handler);
-    }
 }

src/Glpi/Application/SystemConfigurator.php

@@ -35,16 +35,25 @@
 
 namespace Glpi\Application;
 
+use Glpi\Error\ErrorHandler;
+use Glpi\Log\GlpiLogHandler;
+use Monolog\Logger;
+use Psr\Log\LoggerInterface;
+
 final class SystemConfigurator
 {
+    private LoggerInterface $logger;
+
     public function __construct(private string $root_dir, private ?string $env)
     {
+        $this->computeConstants();
+        $this->setSessionConfiguration();
+        $this->initLogger();
     }
 
-    public function __invoke(): void
+    public function getLogger(): LoggerInterface
     {
-        $this->computeConstants();
-        $this->setSessionConfiguration();
+        return $this->logger;
     }
 
     private function computeConstants(): void
@@ -245,4 +254,21 @@ private function setSessionConfiguration(): void
         // for every GLPI instance, enven if they are served by the same server (mostly for dev envs).
         session_name('glpi_' . \hash('sha512', $this->root_dir . ($_SERVER['HTTP_HOST'] ?? '') . ($_SERVER['SERVER_PORT'] ?? '')));
     }
+
+    private function initLogger(): void
+    {
+        /**
+         * @var LoggerInterface $PHPLOGGER
+         */
+        global $PHPLOGGER;
+
+        $PHPLOGGER = new Logger('glpi');
+        $PHPLOGGER->pushHandler(new GlpiLogHandler());
+
+        $this->logger = $PHPLOGGER;
+
+        $errorHandler = new ErrorHandler($PHPLOGGER);
+        $errorHandler::register($errorHandler);
+
+    }
 }

src/Glpi/Controller/CentralController.php

@@ -69,6 +69,8 @@ public function __invoke(Request $request): Response
             return new RedirectResponse($url);
         }
 
+        throw new \RuntimeException('Noop');
+
         return $this->render('pages/central/index.html.twig', [
             'central' => new Central(),
         ]);

src/Glpi/Controller/ErrorController.php

@@ -36,7 +36,6 @@
 
 use Config;
 use DBConnection;
-use Glpi\Application\ErrorUtils;
 use Html;
 use Session;
 use Symfony\Component\ErrorHandler\Error\OutOfMemoryError;
@@ -57,12 +56,17 @@ public function __invoke(Request $request, ?\Throwable $exception = null): Respo
             return new Response('', 500);
         }
 
-        $this->logException($exception, $request);
+        $this->logHttpException($exception, $request);
 
         return $this->getErrorResponse($exception, $request);
     }
 
-    private function logException(\Throwable $exception, Request $request): void
+    /**
+     * @TODO: create a specific handler in our logger so that HTTP errors are logged differently according to this method ⬇
+     *
+     * @see \Glpi\Log\GlpiLogHandler::canHandle
+     */
+    private function logHttpException(\Throwable $exception, Request $request): void
     {
         if (
             $exception instanceof HttpExceptionInterface
@@ -78,29 +82,23 @@ private function logException(\Throwable $exception, Request $request): void
 
             $user_id = Session::getLoginUserID() ?: 'Anonymous';
 
-            switch ($exception::class) {
-                case AccessDeniedHttpException::class:
-                    $message = sprintf(
-                        'User ID: `%s` tried to access or perform an action on `%s` with insufficient rights.',
-                        $user_id,
-                        $requested_uri
-                    );
-                    break;
-                case NotFoundHttpException::class:
-                    $message = sprintf(
-                        'User ID: `%s` tried to access a non-existent item on `%s`.',
-                        $user_id,
-                        $requested_uri
-                    );
-                    break;
-                default:
-                    $message = sprintf(
-                        'User ID: `%s` tried to execute an invalid request on `%s`.',
-                        $user_id,
-                        $requested_uri
-                    );
-                    break;
-            }
+            $message = match ($exception::class) {
+                AccessDeniedHttpException::class => sprintf(
+                    'User ID: `%s` tried to access or perform an action on `%s` with insufficient rights.',
+                    $user_id,
+                    $requested_uri
+                ),
+                NotFoundHttpException::class => sprintf(
+                    'User ID: `%s` tried to access a non-existent item on `%s`.',
+                    $user_id,
+                    $requested_uri
+                ),
+                default => sprintf(
+                    'User ID: `%s` tried to execute an invalid request on `%s`.',
+                    $user_id,
+                    $requested_uri
+                ),
+            };
 
             if (($exception_message = $exception->getMessage()) !== '') {
                 $message .= sprintf('Additional information: %s', $exception_message);
@@ -119,10 +117,6 @@ private function logException(\Throwable $exception, Request $request): void
             }
 
             Toolbox::logInFile('access-errors', $message);
-        } else {
-            // Other errors are logged in the `php-errors` log
-            ErrorUtils::logException($exception);
-            ErrorUtils::outputExceptionMessage($exception);
         }
     }
 

src/Glpi/Error/ErrorDisplayHandler/HtmlErrorDisplayHandler.php

@@ -47,20 +47,30 @@ public static function setCurrentRequest(Request $request): void
 
     public function canOutput(string $log_level, string $env): bool
     {
-        return self::$currentRequest !== null;
+        if (!self::$currentRequest) {
+            return false;
+        }
+        $is_dev_env         = $env === \GLPI::ENV_DEVELOPMENT;
+        $is_debug_mode      = isset($_SESSION['glpi_use_mode']) && $_SESSION['glpi_use_mode'] == \Session::DEBUG_MODE;
+
+        if (
+            !$is_dev_env             // error messages are always displayed in development environment
+            && !$is_debug_mode        // error messages are always displayed in debug mode
+        ) {
+            return false;
+        }
+
+        /**
+         * @see Request::initializeFormats
+         */
+        $type = self::$currentRequest->getPreferredFormat();
+
+        return $type === 'html';
     }
 
     public function displayErrorMessage(string $error_type, string $message, string $log_level, mixed $env): void
     {
-        $req = self::$currentRequest;
-
-        $types = $req->getAcceptableContentTypes();
-
-        if (in_array('text/html', $types)) {
-            echo '<div class="alert alert-important alert-danger glpi-debug-alert" style="z-index:10000">'
-                . '<span class="b">' . \htmlescape($error_type) . ': </span>' . \htmlescape($message) . '</div>';
-        } else {
-            \trigger_error('Cannot display error in HTTP context for requests that do not accept HTML as a response.', \E_USER_WARNING);
-        }
+        echo '<div class="alert alert-important alert-danger glpi-debug-alert" style="z-index:10000">'
+            . '<span class="b">' . \htmlescape($error_type) . ': </span>' . \htmlescape($message) . '</div>';
     }
 }

src/Glpi/Error/ErrorDisplayHandler/EchoDisplayHandler.php → src/Glpi/Error/ErrorDisplayHandler/LegacyCliDisplayHandler.php

@@ -34,11 +34,11 @@
 
 namespace Glpi\Error\ErrorDisplayHandler;
 
-final class EchoDisplayHandler implements ErrorDisplayHandler
+final class LegacyCliDisplayHandler implements ErrorDisplayHandler
 {
     public function canOutput(string $log_level, string $env): bool
     {
-        return true;
+        return \isCommandLine();
     }
 
     public function displayErrorMessage(string $error_type, string $message, string $log_level, mixed $env): void