From 7401bcf02e4ad3fbd00ed9a1eab6cef03df9c13c Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Mon, 21 Oct 2024 10:20:44 +1300 Subject: [PATCH] ENH Do not output core code deprecation messages by default --- composer.json | 3 +- src/Control/Controller.php | 1 - src/Dev/Deprecation.php | 109 +++++++++++++++++++++++++++- tests/php/Dev/DeprecationTest.php | 117 ++++++++++++++++++++++++++---- 4 files changed, 210 insertions(+), 20 deletions(-) diff --git a/composer.json b/composer.json index 80e8c658cd2..0b000071b02 100644 --- a/composer.json +++ b/composer.json @@ -36,8 +36,9 @@ "psr/container": "^1.1 || ^2.0", "psr/http-message": "^1", "sebastian/diff": "^4.0", - "silverstripe/config": "^2", + "silverstripe/config": "^2.2", "silverstripe/assets": "^2.3", + "silverstripe/supported-modules": "^1.1", "silverstripe/vendor-plugin": "^2", "sminnee/callbacklist": "^0.1.1", "symfony/cache": "^6.1", diff --git a/src/Control/Controller.php b/src/Control/Controller.php index 299ce72a7f4..ef7d9c2bbeb 100644 --- a/src/Control/Controller.php +++ b/src/Control/Controller.php @@ -19,7 +19,6 @@ */ class Controller extends RequestHandler implements TemplateGlobalProvider { - /** * An array of arguments extracted from the URL. * diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php index b5e9cf935b2..748ae45a0bd 100644 --- a/src/Dev/Deprecation.php +++ b/src/Dev/Deprecation.php @@ -3,11 +3,14 @@ namespace SilverStripe\Dev; use BadMethodCallException; +use RuntimeException; use SilverStripe\Control\Director; use SilverStripe\Core\Environment; use SilverStripe\Core\Injector\InjectionCreator; use SilverStripe\Core\Injector\InjectorLoader; use SilverStripe\Core\Manifest\Module; +use SilverStripe\Core\Path; +use SilverStripe\SupportedModules\MetaData; /** * Handles raising an notice when accessing a deprecated method, class, configuration, or behaviour. @@ -77,6 +80,18 @@ class Deprecation */ private static bool $showNoReplacementNotices = false; + /** + * @internal + */ + private static bool $showNoticesCalledFromSupportedCode = false; + + /** + * Cache of supported module directories, read from silverstripe/supported-modules repositories.json + * + * @internal + */ + private static array $supportedModuleDirectories = []; + /** * Enable throwing deprecation warnings. By default, this excludes warnings for * deprecated code which is called by core Silverstripe modules. @@ -146,6 +161,12 @@ protected static function get_called_method_from_trace($backtrace, $level = 1) if (!$level) { $level = 1; } + $called = Deprecation::get_called_from_trace($backtrace, $level); + return ($called['class'] ?? '') . ($called['type'] ?? '') . ($called['function'] ?? ''); + } + + private static function get_called_from_trace(array $backtrace, int $level): array + { $newLevel = $level; // handle closures inside withSuppressedNotice() if (Deprecation::$insideNoticeSuppression @@ -163,8 +184,51 @@ protected static function get_called_method_from_trace($backtrace, $level = 1) if ($level == 4 && ($backtrace[$newLevel]['class'] ?? '') === InjectionCreator::class) { $newLevel = $newLevel + 4; } + // handle noticeWithNoReplacment() + foreach ($backtrace as $trace) { + if (($trace['class'] ?? '') === Deprecation::class + && ($trace['function'] ?? '') === 'noticeWithNoReplacment' + ) { + $newLevel = $newLevel + 1; + break; + } + } $called = $backtrace[$newLevel] ?? []; - return ($called['class'] ?? '') . ($called['type'] ?? '') . ($called['function'] ?? ''); + return $called; + } + + private static function isCalledFromSupportedCode(array $backtrace): bool + { + $called = Deprecation::get_called_from_trace($backtrace, 1); + $file = $called['file'] ?? ''; + if (!$file) { + return false; + } + return Deprecation::fileIsInSupportedModule($file); + } + + /** + * Check whether a file (path to file) is in a supported module + */ + public static function fileIsInSupportedModule(string $file): bool + { + // Cache the supported modules list + if (count(Deprecation::$supportedModuleDirectories) === 0) { + // Do not make a network request when fetching metadata which could slow down a website + // While there is a small risk of the list being out of date, there is minimal downside to this + $metaData = MetaData::getAllRepositoryMetaData(fromRemote: false); + $dirs = array_map(fn($module) => "/vendor/{$module['packagist']}/", $metaData['supportedModules']); + // This is a special case for silverstripe-framework when running in CI + // Needed because module is run in the root folder rather than in the vendor folder + $dirs[] = '/silverstripe-framework/'; + Deprecation::$supportedModuleDirectories = $dirs; + } + foreach (Deprecation::$supportedModuleDirectories as $dir) { + if (str_contains($file, $dir)) { + return true; + } + } + return false; } public static function isEnabled(): bool @@ -245,6 +309,22 @@ public static function shouldShowForCli(): bool return Deprecation::$shouldShowForCli; } + /** + * If true, deprecation warnings will be shown for deprecated code which is called by core Silverstripe modules. + */ + public static function getShowNoticesCalledFromSupportedCode(): bool + { + return Deprecation::$showNoticesCalledFromSupportedCode; + } + + /** + * Set whether deprecation warnings will be shown for deprecated code which is called by core Silverstripe modules. + */ + public static function setShowNoticesCalledFromSupportedCode(bool $value): void + { + Deprecation::$showNoticesCalledFromSupportedCode = $value; + } + public static function outputNotices(): void { if (!Deprecation::isEnabled()) { @@ -258,9 +338,13 @@ public static function outputNotices(): void $arr = array_shift(Deprecation::$userErrorMessageBuffer); $message = $arr['message']; $calledWithNoticeSuppression = $arr['calledWithNoticeSuppression']; + $isCalledFromSupportedCode = $arr['isCalledFromSupportedCode']; if ($calledWithNoticeSuppression && !Deprecation::$showNoReplacementNotices) { continue; } + if ($isCalledFromSupportedCode && !Deprecation::$showNoticesCalledFromSupportedCode) { + continue; + } Deprecation::$isTriggeringError = true; user_error($message, E_USER_DEPRECATED); Deprecation::$isTriggeringError = false; @@ -294,6 +378,10 @@ public static function notice($atVersion, $string = '', $scope = Deprecation::SC $data = [ 'key' => sha1($string), 'message' => $string, + // Setting to `false` as here as any SCOPE_CONFIG notices from supported modules have + // already been filtered out if needed if they came from a supported module in + // SilverStripe\Config\Transformer\YamlTransformer::checkForDeprecatedConfig() + 'isCalledFromSupportedCode' => false, 'calledWithNoticeSuppression' => Deprecation::$insideNoticeSuppression ]; } else { @@ -322,13 +410,13 @@ public static function notice($atVersion, $string = '', $scope = Deprecation::SC $level = Deprecation::$insideNoticeSuppression ? 4 : 2; $string .= " Called from " . Deprecation::get_called_method_from_trace($backtrace, $level) . '.'; - if ($caller) { $string = $caller . ' is deprecated.' . ($string ? ' ' . $string : ''); } $data = [ 'key' => sha1($string), 'message' => $string, + 'isCalledFromSupportedCode' => Deprecation::isCalledFromSupportedCode($backtrace), 'calledWithNoticeSuppression' => Deprecation::$insideNoticeSuppression ]; } @@ -360,6 +448,23 @@ public static function notice($atVersion, $string = '', $scope = Deprecation::SC } } + /** + * Shorthand method to create a suppressed notice for something with no immediate replacement. + * If $message is empty, then a standardised message will be used + */ + public static function noticeWithNoReplacment( + string $atVersion, + string $message = '', + int $scope = Deprecation::SCOPE_METHOD + ): void { + if ($message === '') { + $message = 'Will be removed without equivalent functionality to replace it.'; + } + Deprecation::withSuppressedNotice( + fn() => Deprecation::notice($atVersion, $message, $scope) + ); + } + private static function varAsBoolean($val): bool { if (is_string($val)) { diff --git a/tests/php/Dev/DeprecationTest.php b/tests/php/Dev/DeprecationTest.php index e4b3a97eec4..9642356d605 100644 --- a/tests/php/Dev/DeprecationTest.php +++ b/tests/php/Dev/DeprecationTest.php @@ -23,6 +23,8 @@ class DeprecationTest extends SapphireTest private bool $noticesWereEnabled = false; + private bool $showSupportedNoticesWasEnabled = false; + protected function setup(): void { // Use custom error handler for two reasons: @@ -31,6 +33,7 @@ protected function setup(): void // https://github.com/laminas/laminas-di/pull/30#issuecomment-927585210 parent::setup(); $this->noticesWereEnabled = Deprecation::isEnabled(); + $this->showSupportedNoticesWasEnabled = Deprecation::getShowNoticesCalledFromSupportedCode(); $this->oldHandler = set_error_handler(function (int $errno, string $errstr, string $errfile, int $errline) { if ($errno === E_USER_DEPRECATED) { if (str_contains($errstr, 'SilverStripe\\Dev\\Tests\\DeprecationTest')) { @@ -46,6 +49,8 @@ protected function setup(): void // Fallback to default PHP error handler return false; }); + // This is required to clear out the notice from instantiating DeprecationTestObject in TableBuilder::buildTables(). + Deprecation::outputNotices(); } protected function tearDown(): void @@ -55,6 +60,7 @@ protected function tearDown(): void } else { Deprecation::disable(); } + Deprecation::setShowNoticesCalledFromSupportedCode($this->showSupportedNoticesWasEnabled); restore_error_handler(); $this->oldHandler = null; parent::tearDown(); @@ -66,6 +72,18 @@ private function myDeprecatedMethod(): string return 'abc'; } + private function myDeprecatedMethodNoReplacement(): string + { + Deprecation::noticeWithNoReplacment('1.2.3'); + return 'abc'; + } + + private function enableDeprecationNotices(bool $showNoReplacementNotices = false): void + { + Deprecation::enable($showNoReplacementNotices); + Deprecation::setShowNoticesCalledFromSupportedCode(true); + } + public function testNotice() { $message = implode(' ', [ @@ -75,7 +93,7 @@ public function testNotice() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); $ret = $this->myDeprecatedMethod(); $this->assertSame('abc', $ret); // call outputNotices() directly because the regular shutdown function that emits @@ -83,6 +101,29 @@ public function testNotice() Deprecation::outputNotices(); } + public function testNoticeNoReplacement() + { + $message = implode(' ', [ + 'SilverStripe\Dev\Tests\DeprecationTest->myDeprecatedMethodNoReplacement is deprecated.', + 'Will be removed without equivalent functionality to replace it.', + 'Called from SilverStripe\Dev\Tests\DeprecationTest->testNoticeNoReplacement.' + ]); + $this->expectDeprecation(); + $this->expectDeprecationMessage($message); + $this->enableDeprecationNotices(true); + $ret = $this->myDeprecatedMethodNoReplacement(); + $this->assertSame('abc', $ret); + Deprecation::outputNotices(); + } + + public function testNoticeNoReplacementNoSupressed() + { + $this->enableDeprecationNotices(); + $ret = $this->myDeprecatedMethodNoReplacement(); + $this->assertSame('abc', $ret); + Deprecation::outputNotices(); + } + public function testCallUserFunc() { $message = implode(' ', [ @@ -92,7 +133,7 @@ public function testCallUserFunc() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); $ret = call_user_func([$this, 'myDeprecatedMethod']); $this->assertSame('abc', $ret); Deprecation::outputNotices(); @@ -107,7 +148,7 @@ public function testCallUserFuncArray() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); $ret = call_user_func_array([$this, 'myDeprecatedMethod'], []); $this->assertSame('abc', $ret); Deprecation::outputNotices(); @@ -115,7 +156,7 @@ public function testCallUserFuncArray() public function testwithSuppressedNoticeDefault() { - Deprecation::enable(); + $this->enableDeprecationNotices(); $ret = Deprecation::withSuppressedNotice(function () { return $this->myDeprecatedMethod(); }); @@ -132,7 +173,7 @@ public function testwithSuppressedNoticeTrue() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(true); + $this->enableDeprecationNotices(true); $ret = Deprecation::withSuppressedNotice(function () { return $this->myDeprecatedMethod(); }); @@ -149,7 +190,7 @@ public function testwithSuppressedNoticeTrueCallUserFunc() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(true); + $this->enableDeprecationNotices(true); $ret = Deprecation::withSuppressedNotice(function () { return call_user_func([$this, 'myDeprecatedMethod']); }); @@ -166,7 +207,7 @@ public function testNoticewithSuppressedNoticeTrue() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(true); + $this->enableDeprecationNotices(true); Deprecation::withSuppressedNotice(function () { Deprecation::notice('123', 'My message.'); }); @@ -182,7 +223,7 @@ public function testClasswithSuppressedNotice() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(true); + $this->enableDeprecationNotices(true); // using this syntax because my IDE was complaining about DeprecationTestObject not existing // when trying to use `new DeprecationTestObject();` $class = DeprecationTestObject::class; @@ -199,7 +240,7 @@ public function testClassWithInjectorwithSuppressedNotice() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(true); + $this->enableDeprecationNotices(true); Injector::inst()->get(DeprecationTestObject::class); Deprecation::outputNotices(); } @@ -217,6 +258,50 @@ public function testDisabled() Deprecation::outputNotices(); } + public function testshowNoticesCalledFromSupportedCode() + { + $this->expectNotToPerformAssertions(); + $this->enableDeprecationNotices(true); + // showNoticesCalledFromSupportedCode is set to true by default for these unit tests + // as it is testing code within vendor/silverstripe + // This test is to ensure that the method works as expected when we disable this + // and we should expect no exceptions to be thrown + // + // Note specifically NOT testing the following because it's counted as being called + // from phpunit itself, which is not considered supported code + // Deprecation::withSuppressedNotice(function () { + // Deprecation::notice('123', 'My message.'); + // }); + Deprecation::setShowNoticesCalledFromSupportedCode(false); + // notice() + $this->myDeprecatedMethod(); + // noticeNoReplacement() + $this->myDeprecatedMethodNoReplacement(); + // callUserFunc() + call_user_func([$this, 'myDeprecatedMethod']); + // callUserFuncArray() + call_user_func_array([$this, 'myDeprecatedMethod'], []); + // withSuppressedNotice() + Deprecation::withSuppressedNotice( + fn() => $this->myDeprecatedMethod() + ); + // withSuppressedNoticeTrue() + Deprecation::withSuppressedNotice(function () { + $this->myDeprecatedMethod(); + }); + // withSuppressedNoticeTrueCallUserFunc() + Deprecation::withSuppressedNotice(function () { + call_user_func([$this, 'myDeprecatedMethod']); + }); + // classWithSuppressedNotice() + $class = DeprecationTestObject::class; + new $class(); + // classWithInjectorwithSuppressedNotice() + Injector::inst()->get(DeprecationTestObject::class); + // Output notices - there should be none + Deprecation::outputNotices(); + } + // The following tests would be better to put in the silverstripe/config module, however this is not // possible to do in a clean way as the config for the DeprecationTestObject will not load if it // is inside the silverstripe/config directory, as there is no _config.php file or _config folder. @@ -231,7 +316,7 @@ public function testConfigGetFirst() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); Config::inst()->get(DeprecationTestObject::class, 'first_config'); Deprecation::outputNotices(); } @@ -244,7 +329,7 @@ public function testConfigGetSecond() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); Config::inst()->get(DeprecationTestObject::class, 'second_config'); Deprecation::outputNotices(); } @@ -254,7 +339,7 @@ public function testConfigGetThird() $message = 'Config SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject.third_config is deprecated.'; $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); Config::inst()->get(DeprecationTestObject::class, 'third_config'); Deprecation::outputNotices(); } @@ -267,7 +352,7 @@ public function testConfigSet() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); Config::modify()->set(DeprecationTestObject::class, 'first_config', 'abc'); Deprecation::outputNotices(); } @@ -280,7 +365,7 @@ public function testConfigMerge() ]); $this->expectDeprecation(); $this->expectDeprecationMessage($message); - Deprecation::enable(); + $this->enableDeprecationNotices(); Config::modify()->merge(DeprecationTestObject::class, 'array_config', ['abc']); Deprecation::outputNotices(); } @@ -366,7 +451,7 @@ private function runConfigVsEnvTest(string $varName, $envVal, bool $configVal, b switch ($varName) { case 'SS_DEPRECATION_ENABLED': if ($configVal) { - Deprecation::enable(); + $this->enableDeprecationNotices(); } else { Deprecation::disable(); } @@ -542,7 +627,7 @@ public function testIsEnabled(string $envMode, ?bool $envEnabled, bool $staticEn private function setEnabledViaStatic(bool $enabled): void { if ($enabled) { - Deprecation::enable(); + $this->enableDeprecationNotices(); } else { Deprecation::disable(); }