From 197778899b759d50bd53d7831385068b6e6270e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Tue, 26 Nov 2024 10:24:08 +0100 Subject: [PATCH] Prevent vendor deprecations to break the test suite --- phpunit/functional/TicketTest.php | 3 + phpunit/functional/ToolboxTest.php | 12 +++ src/Glpi/Application/ErrorHandler.php | 44 ++++++---- tests/functional/DBmysqlIterator.php | 6 ++ .../Glpi/Application/ErrorHandler.php | 80 ++++++++++--------- .../Glpi/Http/LegacyAssetsListener.php | 2 + .../Glpi/Http/LegacyRouterListener.php | 2 + 7 files changed, 96 insertions(+), 53 deletions(-) diff --git a/phpunit/functional/TicketTest.php b/phpunit/functional/TicketTest.php index 25a1ac4e76d..6196a7b5c1d 100644 --- a/phpunit/functional/TicketTest.php +++ b/phpunit/functional/TicketTest.php @@ -7579,7 +7579,10 @@ public function testDynamicProperties(): void { $ticket = new \Ticket(); + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations $ticket->plugin_xxx_data = 'test'; + \error_reporting($reporting_level); // restore previous level + $this->hasPhpLogRecordThatContains( 'Creation of dynamic property Ticket::$plugin_xxx_data is deprecated', LogLevel::INFO diff --git a/phpunit/functional/ToolboxTest.php b/phpunit/functional/ToolboxTest.php index 7f9225cbb01..4b82fd36da0 100644 --- a/phpunit/functional/ToolboxTest.php +++ b/phpunit/functional/ToolboxTest.php @@ -1080,7 +1080,10 @@ public function testIsValidWebUrl(string $url, bool $result) public function testDeprecated() { + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations \Toolbox::deprecated('Calling this function is deprecated'); + \error_reporting($reporting_level); // restore previous level + $this->hasPhpLogRecordThatContains( 'Calling this function is deprecated', LogLevel::INFO @@ -1090,7 +1093,10 @@ public function testDeprecated() public function testDeprecatedPast() { // Test planned deprecation in the past + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations \Toolbox::deprecated('Calling this function is deprecated', true, '10.0'); + \error_reporting($reporting_level); // restore previous level + $this->hasPhpLogRecordThatContains( 'Calling this function is deprecated', LogLevel::INFO @@ -1100,7 +1106,10 @@ public function testDeprecatedPast() public function testDeprecatedCurrent() { // Test planned deprecation in current version + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations \Toolbox::deprecated('Calling this function is deprecated', true, GLPI_VERSION); + \error_reporting($reporting_level); // restore previous level + $this->hasPhpLogRecordThatContains( 'Calling this function is deprecated', LogLevel::INFO @@ -1110,7 +1119,10 @@ public function testDeprecatedCurrent() public function testFutureDeprecated() { // Test planned deprecation in the future does NOT throw an error + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations \Toolbox::deprecated('Calling this function is deprecated', true, '99.0'); + \error_reporting($reporting_level); // restore previous level + $this->assertTrue(true); //non empty test } diff --git a/src/Glpi/Application/ErrorHandler.php b/src/Glpi/Application/ErrorHandler.php index 0d083588502..9a3914bd6bd 100644 --- a/src/Glpi/Application/ErrorHandler.php +++ b/src/Glpi/Application/ErrorHandler.php @@ -63,7 +63,7 @@ class ErrorHandler E_USER_ERROR => LogLevel::ERROR, E_USER_WARNING => LogLevel::WARNING, E_USER_NOTICE => LogLevel::NOTICE, - E_STRICT => LogLevel::NOTICE, + 2048 => LogLevel::NOTICE, // 2048 = deprecated E_STRICT E_RECOVERABLE_ERROR => LogLevel::ERROR, E_DEPRECATED => LogLevel::INFO, E_USER_DEPRECATED => LogLevel::INFO, @@ -227,9 +227,34 @@ public function register(): void set_exception_handler([$this, 'handleException']); } - // Force reporting of all errors, to ensure that all log levels that are supposed to be - // pushed in logs according to `GLPI_LOG_LVL`/`GLPI_ENVIRONMENT_TYPE` are actually reported. - error_reporting(E_ALL); + // Adjust reporting level to the environment, to ensure that all the errors supposed to be logged are + // actually reported, and to prevent reporting other errors. + $reporting_level = E_ALL; + foreach (self::ERROR_LEVEL_MAP as $value => $log_level) { + if ( + $this->env !== GLPI::ENV_DEVELOPMENT + && in_array($log_level, [LogLevel::DEBUG, LogLevel::INFO]) + ) { + // Do not report debug and info messages unless in development env. + // Suppressing the INFO level will prevent deprecations to be pushed in other environments logs. + // + // Suppressing the deprecations in the testing environment is mandatory to prevent deprecations + // triggered in vendor code to make our test suite fail. + // We may review this part once we will have migrate all our test suite on PHPUnit. + // For now, we rely on PHPStan to detect usages of deprecated code. + $reporting_level = $reporting_level & ~$value; + } + + if ( + !in_array($this->env, [GLPI::ENV_DEVELOPMENT, GLPI::ENV_TESTING], true) + && $log_level === LogLevel::NOTICE + ) { + // Do not report notice messages unless in development/testing env. + // Notices are errors with no functional impact, so we do not want people to report them as issues. + $reporting_level = $reporting_level & ~$value; + } + } + error_reporting($reporting_level); // Disable native error displaying as it will be handled by `self::outputDebugMessage()`. ini_set('display_errors', 'Off'); @@ -435,15 +460,6 @@ private function outputDebugMessage(string $error_type, string $message, string return; } - if ( - in_array($this->env, [GLPI::ENV_STAGING, GLPI::ENV_PRODUCTION]) - && in_array($log_level, [LogLevel::DEBUG, LogLevel::INFO]) - ) { - // On staging/production environment, do not output debug/info messages. - // These messages are not supposed to be intended for end users, as they have no functional impact. - return; - } - $is_dev_env = $this->env === GLPI::ENV_DEVELOPMENT; $is_debug_mode = isset($_SESSION['glpi_use_mode']) && $_SESSION['glpi_use_mode'] == \Session::DEBUG_MODE; $is_console_context = $this->output_handler instanceof OutputInterface; @@ -513,7 +529,7 @@ private function codeToString(int $error_code): string E_USER_ERROR => 'User Error', E_USER_WARNING => 'User Warning', E_USER_NOTICE => 'User Notice', - E_STRICT => 'Runtime Notice', + 2048 => 'Runtime Notice', // 2048 = deprecated E_STRICT E_RECOVERABLE_ERROR => 'Catchable Fatal Error', E_DEPRECATED => 'Deprecated function', E_USER_DEPRECATED => 'User deprecated function', diff --git a/tests/functional/DBmysqlIterator.php b/tests/functional/DBmysqlIterator.php index a7dde31ee69..ad9715e904b 100644 --- a/tests/functional/DBmysqlIterator.php +++ b/tests/functional/DBmysqlIterator.php @@ -1635,7 +1635,9 @@ public function testConvertOldRequestArgsToCriteria(array $params, array $expect $result = null; $this->when( function () use ($iterator, $params, &$result) { + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations $result = $this->callPrivateMethod($iterator, 'convertOldRequestArgsToCriteria', $params, 'test'); + \error_reporting($reporting_level); // restore previous level } ) ->error @@ -1665,7 +1667,9 @@ public function testExecuteWithOldSignature(array $params, array $expected, stri $iterator = new \DBmysqlIterator($db); $this->when( function () use ($iterator, $params) { + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations $iterator = $iterator->execute(...$params); + \error_reporting($reporting_level); // restore previous level } ) ->error @@ -1701,7 +1705,9 @@ public function testBuildQueryWithOldSignature(array $params, array $expected, s $iterator = new \DBmysqlIterator($db); $this->when( function () use ($iterator, $params) { + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations $iterator = $iterator->buildQuery(...$params); + \error_reporting($reporting_level); // restore previous level } ) ->error diff --git a/tests/functional/Glpi/Application/ErrorHandler.php b/tests/functional/Glpi/Application/ErrorHandler.php index d59840bf776..ad35b5c065b 100644 --- a/tests/functional/Glpi/Application/ErrorHandler.php +++ b/tests/functional/Glpi/Application/ErrorHandler.php @@ -52,6 +52,9 @@ protected function errorProvider(): iterable foreach ([Session::NORMAL_MODE, Session::DEBUG_MODE] as $session_mode) { foreach ([GLPI::ENV_DEVELOPMENT, GLPI::ENV_TESTING, GLPI::ENV_STAGING, GLPI::ENV_PRODUCTION] as $env) { + $suppress_info = $env !== GLPI::ENV_DEVELOPMENT; + $suppress_notice = !in_array($env, [GLPI::ENV_DEVELOPMENT, GLPI::ENV_TESTING], true); + yield [ 'session_mode' => $session_mode, 'env' => $env, @@ -82,8 +85,8 @@ protected function errorProvider(): iterable 'error_call' => function () { trigger_error('some notice', E_USER_NOTICE); }, - 'expected_log_level' => Level::Notice, - 'expected_msg_pattern' => $log_prefix + 'expected_log_level' => $suppress_notice ? null : Level::Notice, + 'expected_msg_pattern' => $suppress_notice ? null : $log_prefix . preg_quote('PHP User Notice (' . E_USER_NOTICE . '): some notice', '/') . $log_suffix, ]; @@ -94,8 +97,8 @@ protected function errorProvider(): iterable 'error_call' => function () { trigger_error('this method is deprecated', E_USER_DEPRECATED); }, - 'expected_log_level' => Level::Info, - 'expected_msg_pattern' => $log_prefix + 'expected_log_level' => $suppress_info ? null : Level::Info, + 'expected_msg_pattern' => $suppress_info ? null : $log_prefix . preg_quote('PHP User deprecated function (' . E_USER_DEPRECATED . '): this method is deprecated', '/') . $log_suffix, ]; @@ -107,8 +110,8 @@ protected function errorProvider(): iterable $param = new \ReflectionParameter([\Config::class, 'getTypeName'], 0); $param->isCallable(); }, - 'expected_log_level' => Level::Info, - 'expected_msg_pattern' => $log_prefix + 'expected_log_level' => $suppress_info ? null : Level::Info, + 'expected_msg_pattern' => $suppress_info ? null : $log_prefix . preg_quote('PHP Deprecated function (' . E_DEPRECATED . '): Method ReflectionParameter::isCallable() is deprecated', '/') . $log_suffix, ]; @@ -128,13 +131,13 @@ public function testRegisteredErrorHandler( int $session_mode, string $env, callable $error_call, - Level $expected_log_level, - string $expected_msg_pattern + ?Level $expected_log_level, + ?string $expected_msg_pattern ) { $handler = new TestHandler(); $logger = $this->newMockInstance('Monolog\\Logger', null, null, ['test-logger', [$handler]]); - $previous_use_mode = $_SESSION['glpi_use_mode']; + $previous_use_mode = $_SESSION['glpi_use_mode']; $error_handler = $this->callPrivateConstructor( \Glpi\Application\ErrorHandler::class, @@ -154,15 +157,13 @@ public function testRegisteredErrorHandler( $_SESSION['glpi_use_mode'] = $session_mode; $error_call(); $_SESSION['glpi_use_mode'] = $previous_use_mode; - $this->integer(count($handler->getRecords()))->isEqualTo(1); - $this->boolean($handler->hasRecordThatMatches($expected_msg_pattern, $expected_log_level)); - if ( - $env === GLPI::ENV_DEVELOPMENT - || ( - $session_mode === Session::DEBUG_MODE - && ($expected_log_level->isHigherThan(Level::Info) || !in_array($env, [GLPI::ENV_STAGING, GLPI::ENV_PRODUCTION])) - ) - ) { + if ($expected_log_level !== null) { + $this->integer(count($handler->getRecords()))->isEqualTo(1); + $this->boolean($handler->hasRecordThatMatches($expected_msg_pattern, $expected_log_level)); + } else { + $this->integer(count($handler->getRecords()))->isEqualTo(0); + } + if ($expected_msg_pattern !== null && ($env === GLPI::ENV_DEVELOPMENT || $session_mode === Session::DEBUG_MODE)) { $this->output->matches($expected_msg_pattern); } else { $this->output->isEmpty(); @@ -177,6 +178,9 @@ protected function handleErrorProvider(): iterable foreach ([Session::NORMAL_MODE, Session::DEBUG_MODE] as $session_mode) { foreach ([GLPI::ENV_DEVELOPMENT, GLPI::ENV_TESTING, GLPI::ENV_STAGING, GLPI::ENV_PRODUCTION] as $env) { + $suppress_info = $env !== GLPI::ENV_DEVELOPMENT; + $suppress_notice = !in_array($env, [GLPI::ENV_DEVELOPMENT, GLPI::ENV_TESTING], true); + yield [ 'session_mode' => $session_mode, 'env' => $env, @@ -190,8 +194,8 @@ protected function handleErrorProvider(): iterable 'session_mode' => $session_mode, 'env' => $env, 'error_code' => E_NOTICE, - 'expected_log_level' => Level::Notice, - 'expected_msg_pattern' => $log_prefix . 'PHP Notice' . $log_suffix, + 'expected_log_level' => $suppress_notice ? null : Level::Notice, + 'expected_msg_pattern' => $suppress_notice ? null : $log_prefix . 'PHP Notice' . $log_suffix, 'is_fatal_error' => false, ]; @@ -226,8 +230,8 @@ protected function handleErrorProvider(): iterable 'session_mode' => $session_mode, 'env' => $env, 'error_code' => E_USER_NOTICE, - 'expected_log_level' => Level::Notice, - 'expected_msg_pattern' => $log_prefix . 'PHP User Notice' . $log_suffix, + 'expected_log_level' => $suppress_notice ? null : Level::Notice, + 'expected_msg_pattern' => $suppress_notice ? null : $log_prefix . 'PHP User Notice' . $log_suffix, 'is_fatal_error' => false, ]; @@ -235,8 +239,8 @@ protected function handleErrorProvider(): iterable 'session_mode' => $session_mode, 'env' => $env, 'error_code' => E_DEPRECATED, - 'expected_log_level' => Level::Info, - 'expected_msg_pattern' => $log_prefix . 'PHP Deprecated function' . $log_suffix, + 'expected_log_level' => $suppress_info ? null : Level::Info, + 'expected_msg_pattern' => $suppress_info ? null : $log_prefix . 'PHP Deprecated function' . $log_suffix, 'is_fatal_error' => false, ]; @@ -244,8 +248,8 @@ protected function handleErrorProvider(): iterable 'session_mode' => $session_mode, 'env' => $env, 'error_code' => E_USER_DEPRECATED, - 'expected_log_level' => Level::Info, - 'expected_msg_pattern' => $log_prefix . 'PHP User deprecated function' . $log_suffix, + 'expected_log_level' => $suppress_info ? null : Level::Info, + 'expected_msg_pattern' => $suppress_info ? null : $log_prefix . 'PHP User deprecated function' . $log_suffix, 'is_fatal_error' => false, ]; } @@ -261,20 +265,21 @@ public function testHandleError( int $session_mode, string $env, int $error_code, - Level $expected_log_level, - string $expected_msg_pattern, + ?Level $expected_log_level, + ?string $expected_msg_pattern, bool $is_fatal_error ) { $handler = new TestHandler(); $logger = $this->newMockInstance('Monolog\\Logger', null, null, ['test-logger', [$handler]]); - $previous_use_mode = $_SESSION['glpi_use_mode']; + $previous_use_mode = $_SESSION['glpi_use_mode']; $error_handler = $this->callPrivateConstructor( \Glpi\Application\ErrorHandler::class, [$logger, $env] ); $error_handler->setForwardToInternalHandler(false); + $error_handler->register(); // Assert that nothing is logged when using '@' operator $_SESSION['glpi_use_mode'] = $session_mode; @@ -305,16 +310,13 @@ public function testHandleError( $_SESSION['glpi_use_mode'] = $previous_use_mode; } - $this->integer(count($handler->getRecords()))->isEqualTo(1); - $this->boolean($handler->hasRecordThatMatches($expected_msg_pattern, $expected_log_level)); - - if ( - $env === GLPI::ENV_DEVELOPMENT - || ( - $session_mode === Session::DEBUG_MODE - && ($expected_log_level->isHigherThan(Level::Info) || !in_array($env, [GLPI::ENV_STAGING, GLPI::ENV_PRODUCTION])) - ) - ) { + if ($expected_log_level !== null) { + $this->integer(count($handler->getRecords()))->isEqualTo(1); + $this->boolean($handler->hasRecordThatMatches($expected_msg_pattern, $expected_log_level)); + } else { + $this->integer(count($handler->getRecords()))->isEqualTo(0); + } + if ($expected_msg_pattern !== null && ($env === GLPI::ENV_DEVELOPMENT || $session_mode === Session::DEBUG_MODE)) { $this->output->matches($expected_msg_pattern); } else { $this->output->isEmpty(); diff --git a/tests/functional/Glpi/Http/LegacyAssetsListener.php b/tests/functional/Glpi/Http/LegacyAssetsListener.php index 71a1a781739..56352fe78a8 100644 --- a/tests/functional/Glpi/Http/LegacyAssetsListener.php +++ b/tests/functional/Glpi/Http/LegacyAssetsListener.php @@ -594,7 +594,9 @@ public function testServeLegacyAssetsFromDeprecatedMarketplacePath(): void $response = null; $this->when( function () use ($request, &$response) { + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations $response = $this->callPrivateMethod($this->testedInstance, 'serveLegacyAssets', $request); + \error_reporting($reporting_level); // restore previous level } )->error ->withMessage('Accessing the plugins resources from the `/marketplace/` path is deprecated. Use the `/plugins/` path instead.') diff --git a/tests/functional/Glpi/Http/LegacyRouterListener.php b/tests/functional/Glpi/Http/LegacyRouterListener.php index 649aea79ba4..9a54e960c61 100644 --- a/tests/functional/Glpi/Http/LegacyRouterListener.php +++ b/tests/functional/Glpi/Http/LegacyRouterListener.php @@ -594,7 +594,9 @@ public function testRunLegacyRouterFromDeprecatedMarketplacePath(): void $this->when( function () use ($event) { + $reporting_level = \error_reporting(E_ALL); // be sure to report deprecations $this->testedInstance->onKernelRequest($event); + \error_reporting($reporting_level); // restore previous level } )->error ->withMessage('Accessing the plugins resources from the `/marketplace/` path is deprecated. Use the `/plugins/` path instead.')