From a97935ca0141f525b9993eca32395d32c9327c81 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Wed, 13 Mar 2024 15:38:21 +0100 Subject: [PATCH] =?UTF-8?q?N=C2=B07336=20DeprecatedCallsLog=20robustness?= =?UTF-8?q?=20(#632)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Was creating PHP notices when deprecated method caller wasn't a class/method Found in iTop 3.2.0-dev with itop-object-copier copy.php : it is calling WebPage::add_linked_script directly inside the copy.php script (no class nor function) Co-authored-by: odain --- core/log.class.inc.php | 54 ++++++--- .../core/Log/DeprecatedCallsLogTest.php | 107 +++++++++++++++++- 2 files changed, 141 insertions(+), 20 deletions(-) diff --git a/core/log.class.inc.php b/core/log.class.inc.php index 20344db7a..9e6ecc90d 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -3,7 +3,7 @@ // // This file is part of iTop. // -// iTop is free software; you can redistribute it and/or modify +// iTop is free software; you can redistribute it and/or modify // it under the terms of the GNU Affero General Public License as published by // the Free Software Foundation, either version 3 of the License, or // (at your option) any later version. @@ -1238,19 +1238,7 @@ class DeprecatedCallsLog extends LogAPI } $aStack = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3); - $iStackDeprecatedMethodLevel = 1; // level 0 = current method, level 1 = method containing the `NotifyDeprecatedPhpMethod` call - $sDeprecatedObject = $aStack[$iStackDeprecatedMethodLevel]['class']; - $sDeprecatedMethod = $aStack[$iStackDeprecatedMethodLevel]['function']; - $sCallerFile = $aStack[$iStackDeprecatedMethodLevel]['file']; - $sCallerLine = $aStack[$iStackDeprecatedMethodLevel]['line']; - $sMessage = "Call to {$sDeprecatedObject}::{$sDeprecatedMethod} in {$sCallerFile}#L{$sCallerLine}"; - - $iStackCallerMethodLevel = $iStackDeprecatedMethodLevel + 1; // level 2 = caller of the deprecated method - if (array_key_exists($iStackCallerMethodLevel, $aStack)) { - $sCallerObject = $aStack[$iStackCallerMethodLevel]['class']; - $sCallerMethod = $aStack[$iStackCallerMethodLevel]['function']; - $sMessage .= " ({$sCallerObject}::{$sCallerMethod})"; - } + $sMessage = self::GetMessageFromStack($aStack); if (!is_null($sAdditionalMessage)) { $sMessage .= ' : '.$sAdditionalMessage; @@ -1259,6 +1247,44 @@ class DeprecatedCallsLog extends LogAPI static::Warning($sMessage, self::ENUM_CHANNEL_PHP_METHOD); } + /** + * @param array $aDebugBacktrace data from {@see debug_backtrace()} + * @return string message to print to the log + */ + private static function GetMessageFromStack(array $aDebugBacktrace): string + { + // level 0 = current method + // level 1 = deprecated method, containing the `NotifyDeprecatedPhpMethod` call + $sMessage = 'Call' . self::GetMessageForCurrentStackLevel($aDebugBacktrace[1], " to "); + + // level 2 = caller of the deprecated method + if (array_key_exists(2, $aDebugBacktrace)) { + $sMessage .= ' (from '; + $sMessage .= self::GetMessageForCurrentStackLevel($aDebugBacktrace[2]); + $sMessage .= ')'; + } + + return $sMessage; + } + + private static function GetMessageForCurrentStackLevel(array $aCurrentLevelDebugTrace, ?string $sPrefix=""): string + { + $sMessage = ""; + if (array_key_exists('class', $aCurrentLevelDebugTrace)) { + $sDeprecatedObject = $aCurrentLevelDebugTrace['class']; + $sDeprecatedMethod = $aCurrentLevelDebugTrace['function'] ?? ""; + $sMessage = "{$sPrefix}{$sDeprecatedObject}::{$sDeprecatedMethod} in "; + } + + if (array_key_exists('file', $aCurrentLevelDebugTrace)) { + $sCallerFile = $aCurrentLevelDebugTrace['file']; + $sCallerLine = $aCurrentLevelDebugTrace['line'] ?? ""; + $sMessage .= "{$sCallerFile}#L{$sCallerLine}"; + } + + return $sMessage; + } + public static function Log($sLevel, $sMessage, $sChannel = null, $aContext = array()): void { if (true === utils::IsDevelopmentEnvironment()) { diff --git a/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php b/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php index 055e67669..589dcb948 100644 --- a/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php +++ b/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php @@ -10,26 +10,28 @@ namespace Combodo\iTop\Test\UnitTest\Core\Log; use Combodo\iTop\Test\UnitTest\ItopTestCase; use DeprecatedCallsLog; -class DeprecatedCallsLogTest extends ItopTestCase { +class DeprecatedCallsLogTest extends ItopTestCase +{ /** * We are testing for a undefined offset error. This was throwing a Notice, but starting with PHP 8.0 it was converted to a Warning ! Also the message was changed :( * * @link https://www.php.net/manual/en/migration80.incompatible.php check "A number of notices have been converted into warnings:" */ - private function SetUndefinedOffsetExceptionToExpect(): void { + private function SetUndefinedOffsetExceptionToExpect(): void + { /** @noinspection ConstantCanBeUsedInspection Preferring the function call as it is easier to read and won't cost that much in this PHPUnit context */ if (version_compare(PHP_VERSION, '8.0', '>=')) { $this->expectWarning(); $sUndefinedOffsetExceptionMessage = 'Undefined array key "tutu"'; - } - else { + } else { $this->expectNotice(); $sUndefinedOffsetExceptionMessage = 'Undefined index: tutu'; } $this->expectExceptionMessage($sUndefinedOffsetExceptionMessage); } - public function testPhpNoticeWithoutDeprecatedCallsLog(): void { + public function testPhpNoticeWithoutDeprecatedCallsLog(): void + { $this->SetUndefinedOffsetExceptionToExpect(); $aArray = []; @@ -48,7 +50,8 @@ class DeprecatedCallsLogTest extends ItopTestCase { * @since 3.0.4 N°6274 * @covers DeprecatedCallsLog::DeprecatedNoticesErrorHandler */ - public function testPhpNoticeWithDeprecatedCallsLog(): void { + public function testPhpNoticeWithDeprecatedCallsLog(): void + { $this->RequireOnceItopFile('core/log.class.inc.php'); DeprecatedCallsLog::Enable(); // will set error handler $this->SetUndefinedOffsetExceptionToExpect(); @@ -58,4 +61,96 @@ class DeprecatedCallsLogTest extends ItopTestCase { //Do nothing, just raising a undefined offset warning } } + + /** + * @dataProvider GetMessageFromStackProvider + */ + public function testGetMessageFromStack($aDebugBacktrace, $sExpectedMessage): void + { + $sActualMessage = $this->InvokeNonPublicStaticMethod(DeprecatedCallsLog::class, 'GetMessageFromStack', [$aDebugBacktrace]); + $this->assertEquals($sExpectedMessage, $sActualMessage); + } + + public function GetMessageFromStackProvider() + { + return [ + 'Call in a file outside of a function or class' => [ + [ + [ + 'file' => 'C:\Dev\wamp64\www\itop-32\sources\Application\WebPage\WebPage.php', + 'line' => '866', + 'function' => 'NotifyDeprecatedPhpMethod', + 'class' => 'DeprecatedCallsLog', + 'type' => '::', + ], + [ + 'file' => 'C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php', + 'line' => '130', + 'function' => 'add_linked_script', + 'class' => 'Combodo\iTop\Application\WebPage\WebPage', + 'type' => '->', + ], + [ + 'file' => 'C:\Dev\wamp64\www\itop-32\pages\exec.php', + 'line' => '102', + 'args' => ['C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php'], + 'function' => 'require_once', + ], + ], + 'Call to Combodo\iTop\Application\WebPage\WebPage::add_linked_script in C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php#L130 (from C:\Dev\wamp64\www\itop-32\pages\exec.php#L102)', + ], + + 'Call in a file function, outside of a class' => [ + [ + [ + 'file' => 'C:\\Dev\\wamp64\\www\\itop-32\\sources\\Application\\WebPage\\WebPage.php', + 'line' => 866, + 'function' => 'NotifyDeprecatedPhpMethod', + 'class' => 'DeprecatedCallsLog', + 'type' => '::', + ], + [ + 'file' => 'C:\\Dev\\wamp64\\www\\itop-32\\extensions\\itop-object-copier\\copy.php', + 'line' => 81, + 'function' => 'add_linked_script', + 'class' => 'Combodo\\iTop\\Application\\WebPage\\WebPage', + 'type' => '->', + ], + [ + 'file' => 'C:\\Dev\\wamp64\\www\\itop-32\\extensions\\itop-object-copier\\copy.php', + 'line' => 123, + 'function' => 'myFunction', + ], + ], + 'Call to Combodo\iTop\Application\WebPage\WebPage::add_linked_script in C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php#L81 (from C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php#L123)', + ], + + 'Call from a class method' => [ + [ + [ + 'file' => 'C:\\Dev\\wamp64\\www\\itop-32\\sources\\Application\\WebPage\\WebPage.php', + 'line' => 866, + 'function' => 'NotifyDeprecatedPhpMethod', + 'class' => 'DeprecatedCallsLog', + 'type' => '::', + ], + [ + 'file' => 'C:\\Dev\\wamp64\\www\\itop-32\\extensions\\itop-object-copier\\copy.php', + 'line' => 82, + 'function' => 'add_linked_script', + 'class' => 'Combodo\\iTop\\Application\\WebPage\\WebPage', + 'type' => '->', + ], + [ + 'file' => 'C:\\Dev\\wamp64\\www\\itop-32\\extensions\\itop-object-copier\\copy.php', + 'line' => 125, + 'function' => 'MyMethod', + 'class' => 'MyClass', + 'type' => '::', + ], + ], + 'Call to Combodo\iTop\Application\WebPage\WebPage::add_linked_script in C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php#L82 (from MyClass::MyMethod in C:\Dev\wamp64\www\itop-32\extensions\itop-object-copier\copy.php#L125)', + ], + ]; + } }