From 584cfa8cbf0d33d945ac0ed984a14f6b4a28e32e Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Thu, 4 May 2023 17:03:10 +0200 Subject: [PATCH 1/3] =?UTF-8?q?N=C2=B06274=20Fix=20PHP=20Notices=20not=20c?= =?UTF-8?q?aught=20in=20ItopDataTestCase=20PHPUnit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was caused by the set_error_handler() done in DeprecatedCallsLog during startup Now we are : * not registering the handler if a PHPUnit test is running (based on a constant set in ItopTestCase::setUp) * on registration only do it for the required notices --- core/log.class.inc.php | 12 ++++-- tests/php-unit-tests/ItopTestCase.php | 19 ++++++--- .../core/Log/DeprecatedCallsLogTest.php | 41 +++++++++++++++++++ 3 files changed, 62 insertions(+), 10 deletions(-) create mode 100644 tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php diff --git a/core/log.class.inc.php b/core/log.class.inc.php index 9c9fbc396..c41b4e31f 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -15,6 +15,7 @@ // // You should have received a copy of the GNU Affero General Public License // along with iTop. If not, see +use Combodo\iTop\Test\UnitTest\ItopTestCase; /** @@ -1071,16 +1072,19 @@ class DeprecatedCallsLog extends LogAPI * @uses \set_error_handler() to catch deprecated notices * * @since 3.0.0 N°3002 logs deprecated notices in called code + * @since 3.0.4 N°6274 do not set handler when in PHPUnit context (otherwise PHP notices won't be caught) */ - public static function Enable($sTargetFile = null): void - { + public static function Enable($sTargetFile = null): void { if (empty($sTargetFile)) { $sTargetFile = APPROOT.'log/deprecated-calls.log'; } parent::Enable($sTargetFile); - if (static::IsLogLevelEnabledSafe(self::LEVEL_WARNING, self::ENUM_CHANNEL_PHP_LIBMETHOD)) { - set_error_handler([static::class, 'DeprecatedNoticesErrorHandler']); + if ( + (false === defined(ItopTestCase::ITOP_PHPUNIT_RUNNING_CONSTANT_NAME)) + && static::IsLogLevelEnabledSafe(self::LEVEL_WARNING, self::ENUM_CHANNEL_PHP_LIBMETHOD) + ) { + set_error_handler([static::class, 'DeprecatedNoticesErrorHandler'], E_DEPRECATED | E_USER_DEPRECATED); } } diff --git a/tests/php-unit-tests/ItopTestCase.php b/tests/php-unit-tests/ItopTestCase.php index 633e8884f..5dec2a274 100644 --- a/tests/php-unit-tests/ItopTestCase.php +++ b/tests/php-unit-tests/ItopTestCase.php @@ -32,13 +32,20 @@ use SetupUtils; define('DEBUG_UNIT_TEST', true); -class ItopTestCase extends TestCase -{ - const TEST_LOG_DIR = 'test'; +class ItopTestCase extends TestCase { + /** + * @since 3.0.4 N°6274 Allow to test if PHPUnit is currently running. Starting with PHPUnit 9.5 we'll be able to replace it with $GLOBALS['phpunit_version'] + */ + public const ITOP_PHPUNIT_RUNNING_CONSTANT_NAME = 'ITOP_PHPUNIT_RUNNING'; + + public const TEST_LOG_DIR = 'test'; + + protected function setUp(): void { + if (false === defined(static::ITOP_PHPUNIT_RUNNING_CONSTANT_NAME)) { + // setUp might be called multiple times, so protecting the define() call ! + define(static::ITOP_PHPUNIT_RUNNING_CONSTANT_NAME, true); + } - /** @noinspection UsingInclusionOnceReturnValueInspection avoid errors for approot includes */ - protected function setUp(): void - { $sAppRootRelPath = 'approot.inc.php'; $sDepthSeparator = '../'; for ($iDepth = 0; $iDepth < 8; $iDepth++) { diff --git a/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php b/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php new file mode 100644 index 000000000..6d01ec173 --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php @@ -0,0 +1,41 @@ +expectNotice(); + + $aArray = []; + if ('toto' === $aArray['tutu']) { + //Do nothing, just raising a undefined offset warning + } + } + + /** + * The error handler set by DeprecatedCallsLog during startup was causing PHPUnit to miss PHP notices like "undefined offset" + * + * The error handler is now disabled when running PHPUnit + * + * @since 3.0.4 N°6274 + * @covers DeprecatedCallsLog::DeprecatedNoticesErrorHandler + */ + public function testPhpNoticeWithDeprecatedCallsLog(): void { + $this->RequireOnceItopFile('core/log.class.inc.php'); + DeprecatedCallsLog::Enable(); // will set error handler + $this->expectNotice(); + + $aArray = []; + if ('toto' === $aArray['tutu']) { + //Do nothing, just raising a undefined offset warning + } + } +} From 1884596ecd0c439470297c8e8f1a2651dea890e3 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Thu, 4 May 2023 17:33:25 +0200 Subject: [PATCH 2/3] =?UTF-8?q?N=C2=B06274=20Fix=20log.class.inc.php=20cra?= =?UTF-8?q?shing=20cause=20cannot=20load=20ItopTestCase=20class=20Now=20th?= =?UTF-8?q?e=20constant=20name=20is=20defined=20in=20approot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- approot.inc.php | 5 +++++ core/log.class.inc.php | 3 +-- tests/php-unit-tests/ItopTestCase.php | 15 +++++---------- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/approot.inc.php b/approot.inc.php index f2a70685b..36a984ab1 100644 --- a/approot.inc.php +++ b/approot.inc.php @@ -25,4 +25,9 @@ define('ITOP_DESIGN_LATEST_VERSION', '3.0'); */ define('ITOP_CORE_VERSION', '3.0.3'); +/** + * @since 3.0.4 N°6274 Allow to test if PHPUnit is currently running. Starting with PHPUnit 9.5 we'll be able to replace it with $GLOBALS['phpunit_version'] + */ +define('ITOP_PHPUNIT_RUNNING_CONSTANT_NAME', 'ITOP_PHPUNIT_RUNNING'); + require_once APPROOT.'bootstrap.inc.php'; diff --git a/core/log.class.inc.php b/core/log.class.inc.php index c41b4e31f..29e382f20 100644 --- a/core/log.class.inc.php +++ b/core/log.class.inc.php @@ -15,7 +15,6 @@ // // You should have received a copy of the GNU Affero General Public License // along with iTop. If not, see -use Combodo\iTop\Test\UnitTest\ItopTestCase; /** @@ -1081,7 +1080,7 @@ class DeprecatedCallsLog extends LogAPI parent::Enable($sTargetFile); if ( - (false === defined(ItopTestCase::ITOP_PHPUNIT_RUNNING_CONSTANT_NAME)) + (false === defined(ITOP_PHPUNIT_RUNNING_CONSTANT_NAME)) && static::IsLogLevelEnabledSafe(self::LEVEL_WARNING, self::ENUM_CHANNEL_PHP_LIBMETHOD) ) { set_error_handler([static::class, 'DeprecatedNoticesErrorHandler'], E_DEPRECATED | E_USER_DEPRECATED); diff --git a/tests/php-unit-tests/ItopTestCase.php b/tests/php-unit-tests/ItopTestCase.php index 5dec2a274..c2792bf22 100644 --- a/tests/php-unit-tests/ItopTestCase.php +++ b/tests/php-unit-tests/ItopTestCase.php @@ -33,19 +33,9 @@ use SetupUtils; define('DEBUG_UNIT_TEST', true); class ItopTestCase extends TestCase { - /** - * @since 3.0.4 N°6274 Allow to test if PHPUnit is currently running. Starting with PHPUnit 9.5 we'll be able to replace it with $GLOBALS['phpunit_version'] - */ - public const ITOP_PHPUNIT_RUNNING_CONSTANT_NAME = 'ITOP_PHPUNIT_RUNNING'; - public const TEST_LOG_DIR = 'test'; protected function setUp(): void { - if (false === defined(static::ITOP_PHPUNIT_RUNNING_CONSTANT_NAME)) { - // setUp might be called multiple times, so protecting the define() call ! - define(static::ITOP_PHPUNIT_RUNNING_CONSTANT_NAME, true); - } - $sAppRootRelPath = 'approot.inc.php'; $sDepthSeparator = '../'; for ($iDepth = 0; $iDepth < 8; $iDepth++) { @@ -56,6 +46,11 @@ class ItopTestCase extends TestCase { $sAppRootRelPath = $sDepthSeparator.$sAppRootRelPath; } + + if (false === defined(ITOP_PHPUNIT_RUNNING_CONSTANT_NAME)) { + // setUp might be called multiple times, so protecting the define() call ! + define(ITOP_PHPUNIT_RUNNING_CONSTANT_NAME, true); + } } /** From 959ac7e3be1a5b85bc24e59681da36aa5092a1d8 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Fri, 5 May 2023 09:13:03 +0200 Subject: [PATCH 3/3] =?UTF-8?q?N=C2=B06274=20DeprecatedCallsLogTest=20:=20?= =?UTF-8?q?fix=20expected=20exception=20mismatch=20in=20PHP=208.0+=20Undef?= =?UTF-8?q?ined=20offset=20notice=20was=20changed=20to=20a=20warning=20in?= =?UTF-8?q?=20PHP=208.0...=20Also=20message=20was=20changed=20:(?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../core/Log/DeprecatedCallsLogTest.php | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) 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 6d01ec173..906246ff4 100644 --- a/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php +++ b/tests/php-unit-tests/unitary-tests/core/Log/DeprecatedCallsLogTest.php @@ -9,10 +9,30 @@ namespace Combodo\iTop\Test\UnitTest\Core\Log; use Combodo\iTop\Test\UnitTest\ItopTestCase; use DeprecatedCallsLog; +use PHPUnit\Framework\Error\Notice; +use PHPUnit\Framework\Error\Warning; 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 { + if (version_compare(phpversion(), '8.0', '>=')) { + $sUndefinedOffsetExceptionClass = Warning::class; + $sUndefinedOffsetExceptionMessage = 'Undefined array key "tutu"'; + } + else { + $sUndefinedOffsetExceptionClass = Notice::class; + $sUndefinedOffsetExceptionMessage = 'Undefined index: tutu'; + } + $this->expectException($sUndefinedOffsetExceptionClass); + $this->expectExceptionMessage($sUndefinedOffsetExceptionMessage); + } + public function testPhpNoticeWithoutDeprecatedCallsLog(): void { - $this->expectNotice(); + $this->SetUndefinedOffsetExceptionToExpect(); $aArray = []; if ('toto' === $aArray['tutu']) { @@ -31,7 +51,7 @@ class DeprecatedCallsLogTest extends ItopTestCase { public function testPhpNoticeWithDeprecatedCallsLog(): void { $this->RequireOnceItopFile('core/log.class.inc.php'); DeprecatedCallsLog::Enable(); // will set error handler - $this->expectNotice(); + $this->SetUndefinedOffsetExceptionToExpect(); $aArray = []; if ('toto' === $aArray['tutu']) {