tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-10-22 14:05:37 +02:00
Code Issues Projects Releases Wiki Activity

Merge pull request #10697 from creative-commoners/pulls/4/nicer-deprecations

Browse Source 
ENH Improve deprecation logging
This commit is contained in:
Guy Sartorelli 2023-03-09 14:39:51 +13:00 committed by GitHub
commit a4929a171e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 572 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/Core/Environment.php
View File

@ -177,7 +177,10 @@ class Environment
}
/**
* Get value of environment variable
* Get value of environment variable.
* If the value is false, you should check Environment::hasEnv() to see
* if the value is an actual environment variable value or if the variable
* simply hasn't been set.
*
* @param string $name
* @return mixed Value of the environment variable, or false if not set
@ -223,6 +226,17 @@ class Environment
static::$env[$name] = $value;
}
/**
* Check if an environment variable is set
*/
public static function hasEnv(string $name): bool
{
return array_key_exists($name, static::$env)
|| is_array($_ENV) && array_key_exists($name, $_ENV)
|| is_array($_SERVER) && array_key_exists($name, $_SERVER)
|| getenv($name) !== false;
}
/**
* Returns true if this script is being run from the command line rather than the web server
*

163
src/Dev/Deprecation.php
View File

@ -61,12 +61,22 @@ class Deprecation
* Must be configured outside of the config API, as deprecation API
* must be available before this to avoid infinite loops.
*
* This will be overriden by the SS_DEPRECATION_ENABLED environment if present
* This will be overriden by the SS_DEPRECATION_ENABLED environment variable if present
*
* @internal - Marked as internal so this and other private static's are not treated as config
*/
private static bool $currentlyEnabled = false;
/**
* @internal
*/
private static bool $shouldShowForHttp = false;
/**
* @internal
*/
private static bool $shouldShowForCli = true;
/**
* @internal
*/
@ -77,6 +87,11 @@ class Deprecation
*/
private static bool $insideWithNoReplacement = false;
/**
* @internal
*/
private static bool $isTriggeringError = false;
/**
* Buffer of user_errors to be raised
*
@ -94,12 +109,26 @@ class Deprecation
*/
private static bool $showNoReplacementNotices = false;
/**
* Enable throwing deprecation warnings. By default, this excludes warnings for
* deprecated code which is called by core Silverstripe modules.
*
* This will be overriden by the SS_DEPRECATION_ENABLED environment variable if present.
*
* @param bool $showNoReplacementNotices If true, deprecation warnings will also be thrown
* for deprecated code which is called by core Silverstripe modules.
*/
public static function enable(bool $showNoReplacementNotices = false): void
{
static::$currentlyEnabled = true;
static::$showNoReplacementNotices = $showNoReplacementNotices;
}
/**
* Disable throwing deprecation warnings.
*
* This will be overriden by the SS_DEPRECATION_ENABLED environment variable if present.
*/
public static function disable(): void
{
static::$currentlyEnabled = false;
@ -208,7 +237,11 @@ class Deprecation
if (!Director::isDev()) {
return false;
}
return static::$currentlyEnabled || Environment::getEnv('SS_DEPRECATION_ENABLED');
$envVar = Environment::getEnv('SS_DEPRECATION_ENABLED');
if (Environment::hasEnv('SS_DEPRECATION_ENABLED')) {
return self::varAsBoolean($envVar);
}
return static::$currentlyEnabled;
}
/**
@ -223,29 +256,86 @@ class Deprecation
// noop
}
/**
* If true, any E_USER_DEPRECATED errors should be treated as coming
* directly from this class.
*/
public static function isTriggeringError(): bool
{
return self::$isTriggeringError;
}
/**
* Determine whether deprecation warnings should be included in HTTP responses.
* Does not affect logging.
*
* This will be overriden by the SS_DEPRECATION_SHOW_HTTP environment variable if present.
*/
public static function setShouldShowForHttp(bool $value): void
{
self::$shouldShowForHttp = $value;
}
/**
* Determine whether deprecation warnings should be included in CLI responses.
* Does not affect logging.
*
* This will be overriden by the SS_DEPRECATION_SHOW_CLI environment variable if present.
*/
public static function setShouldShowForCli(bool $value): void
{
self::$shouldShowForCli = $value;
}
/**
* If true, deprecation warnings should be included in HTTP responses.
* Does not affect logging.
*/
public static function shouldShowForHttp(): bool
{
$envVar = Environment::getEnv('SS_DEPRECATION_SHOW_HTTP');
if (Environment::hasEnv('SS_DEPRECATION_SHOW_HTTP')) {
return self::varAsBoolean($envVar);
}
return self::$shouldShowForHttp;
}
/**
* If true, deprecation warnings should be included in CLI responses.
* Does not affect logging.
*/
public static function shouldShowForCli(): bool
{
$envVar = Environment::getEnv('SS_DEPRECATION_SHOW_CLI');
if (Environment::hasEnv('SS_DEPRECATION_SHOW_CLI')) {
return self::varAsBoolean($envVar);
}
return self::$shouldShowForCli;
}
public static function outputNotices(): void
{
if (!self::isEnabled()) {
return;
}
$outputMessages = [];
// using a while loop with array_shift() to ensure that self::$userErrorMessageBuffer will have
// have values removed from it before calling user_error()
while (count(self::$userErrorMessageBuffer)) {
$count = 0;
$origCount = count(self::$userErrorMessageBuffer);
while ($origCount > $count) {
$count++;
$arr = array_shift(self::$userErrorMessageBuffer);
$message = $arr['message'];
// often the same deprecation message appears dozens of times, which isn't helpful
// only need to show a single instance of each message
if (in_array($message, $outputMessages)) {
continue;
}
$calledInsideWithNoReplacement = $arr['calledInsideWithNoReplacement'];
if ($calledInsideWithNoReplacement && !self::$showNoReplacementNotices) {
continue;
}
self::$isTriggeringError = true;
user_error($message, E_USER_DEPRECATED);
$outputMessages[] = $message;
self::$isTriggeringError = false;
}
// Make absolutely sure the buffer is empty - array_shift seems to leave an item in the array
// if we're not using numeric keys.
self::$userErrorMessageBuffer = [];
}
/**
@ -265,10 +355,12 @@ class Deprecation
// try block needs to wrap all code in case anything inside the try block
// calls something else that calls Deprecation::notice()
try {
$data = null;
if ($scope === self::SCOPE_CONFIG) {
// Deprecated config set via yaml will only be shown in the browser when using ?flush=1
// It will not show in CLI when running dev/build flush=1
self::$userErrorMessageBuffer[] = [
$data = [
'key' => sha1($string),
'message' => $string,
'calledInsideWithNoReplacement' => self::$insideWithNoReplacement
];
@ -302,20 +394,26 @@ class Deprecation
if ($caller) {
$string = $caller . ' is deprecated.' . ($string ? ' ' . $string : '');
}
self::$userErrorMessageBuffer[] = [
$data = [
'key' => sha1($string),
'message' => $string,
'calledInsideWithNoReplacement' => self::$insideWithNoReplacement
];
}
if (!self::$haveSetShutdownFunction && self::isEnabled()) {
if ($data && !array_key_exists($data['key'], self::$userErrorMessageBuffer)) {
// Store de-duplicated data in a buffer to be outputted when outputNotices() is called
self::$userErrorMessageBuffer[$data['key']] = $data;
// Use a shutdown function rather than immediately calling user_error() so that notices
// do not interfere with setting session varibles i.e. headers already sent error
// it also means the deprecation notices appear below all phpunit output in CI
// which is far nicer than having it spliced between phpunit output
register_shutdown_function(function () {
self::outputNotices();
});
self::$haveSetShutdownFunction = true;
if (!self::$haveSetShutdownFunction && self::isEnabled()) {
register_shutdown_function(function () {
self::outputNotices();
});
self::$haveSetShutdownFunction = true;
}
}
} catch (BadMethodCallException $e) {
if ($e->getMessage() === InjectorLoader::NO_MANIFESTS_AVAILABLE) {
@ -353,4 +451,31 @@ class Deprecation
static::notice('4.12.0', 'Will be removed without equivalent functionality to replace it');
// noop
}
private static function varAsBoolean($val): bool
{
if (is_string($val)) {
$truthyStrings = [
'on',
'true',
'1',
];
if (in_array(strtolower($val), $truthyStrings, true)) {
return true;
}
$falsyStrings = [
'off',
'false',
'0',
];
if (in_array(strtolower($val), $falsyStrings, true)) {
return false;
}
}
return (bool) $val;
}
}

27
src/Logging/HTTPOutputHandler.php
View File

@ -7,6 +7,7 @@ use Monolog\Handler\AbstractProcessingHandler;
use SilverStripe\Control\Controller;
use SilverStripe\Control\Director;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Dev\Deprecation;
/**
* Output the error to the browser, with the given HTTP status code.
@ -137,6 +138,16 @@ class HTTPOutputHandler extends AbstractProcessingHandler
return $this;
}
protected function shouldShowError(int $errorCode): bool
{
// show all non-E_USER_DEPRECATED errors
// or E_USER_DEPRECATED errors when not triggering from the Deprecation class
// or our deprecations when the relevant shouldShow method returns true
return $errorCode !== E_USER_DEPRECATED
|| !Deprecation::isTriggeringError()
|| ($this->isCli() ? Deprecation::shouldShowForCli() : Deprecation::shouldShowForHttp());
}
/**
* @param array $record
* @return bool
@ -145,6 +156,14 @@ class HTTPOutputHandler extends AbstractProcessingHandler
{
ini_set('display_errors', 0);
// Suppress errors that should be suppressed
if (isset($record['context']['code'])) {
$errorCode = $record['context']['code'];
if (!$this->shouldShowError($errorCode)) {
return;
}
}
// TODO: This coupling isn't ideal
// See https://github.com/silverstripe/silverstripe-framework/issues/4484
if (Controller::has_curr()) {
@ -167,4 +186,12 @@ class HTTPOutputHandler extends AbstractProcessingHandler
return false === $this->getBubble();
}
/**
* This method is required and must be protected for unit testing, since we can't mock static or private methods
*/
protected function isCli(): bool
{
return Director::is_cli();
}
}

9
src/Logging/MonologErrorHandler.php
View File

@ -5,6 +5,7 @@ namespace SilverStripe\Logging;
use InvalidArgumentException;
use Psr\Log\LoggerInterface;
use Monolog\ErrorHandler as MonologHandler;
use Psr\Log\LogLevel;
use SilverStripe\Dev\Deprecation;
class MonologErrorHandler implements ErrorHandler
@ -92,7 +93,13 @@ class MonologErrorHandler implements ErrorHandler
}
foreach ($loggers as $logger) {
MonologHandler::register($logger);
// Log deprecation warnings as WARNING, not NOTICE
// see https://github.com/Seldaek/monolog/blob/1.x/doc/01-usage.md#log-levels
$errorLevelMap = [
E_DEPRECATED => LogLevel::WARNING,
E_USER_DEPRECATED => LogLevel::WARNING,
];
MonologHandler::register($logger, $errorLevelMap);
}
}
}

78
tests/php/Core/EnvironmentTest.php
View File

@ -2,6 +2,7 @@
namespace SilverStripe\Core\Tests;
use ReflectionProperty;
use SilverStripe\Core\Environment;
use SilverStripe\Dev\SapphireTest;
@ -66,4 +67,81 @@ class EnvironmentTest extends SapphireTest
$this->assertEquals('fail', $vars['test']);
$this->assertEquals('global', $GLOBALS['test']);
}
public function provideHasEnv()
{
$setAsOptions = [
'.env',
'_ENV',
'_SERVER',
'putenv',
];
$valueOptions = [
true,
false,
null,
0,
1,
1.75,
'',
'0',
'some-value',
];
$scenarios = [];
foreach ($setAsOptions as $setAs) {
foreach ($valueOptions as $value) {
$scenarios[] = [
'setAs' => $setAs,
'value' => $value,
'expected' => true,
];
}
}
$scenarios[] = [
'setAs' => null,
'value' => null,
'expected' => false,
];
return $scenarios;
}
/**
* @dataProvider provideHasEnv
*/
public function testHasEnv(?string $setAs, $value, bool $expected)
{
$name = '_ENVTEST_HAS_ENV';
// Set the value
switch ($setAs) {
case '.env':
Environment::setEnv($name, $value);
break;
case '_ENV':
$_ENV[$name] = $value;
break;
case '_SERVER':
$_SERVER[$name] = $value;
break;
case 'putenv':
$val = is_string($value) ? $value : json_encode($value);
putenv("$name=$val");
break;
default:
// null is no-op, to validate not setting it works as expected.
if ($setAs !== null) {
$this->fail("setAs value $setAs isn't taken into account correctly for this test.");
}
}
$this->assertSame($expected, Environment::hasEnv($name));
// unset the value
$reflectionEnv = new ReflectionProperty(Environment::class, 'env');
$reflectionEnv->setAccessible(true);
$reflectionEnv->setValue(array_diff($reflectionEnv->getValue(), [$name => $value]));
unset($_ENV[$name]);
unset($_SERVER[$name]);
putenv("$name");
}
}

173
tests/php/Dev/DeprecationTest.php
View File

@ -3,7 +3,9 @@
namespace SilverStripe\Dev\Tests;
use PHPUnit\Framework\Error\Deprecated;
use ReflectionMethod;
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Environment;
use SilverStripe\Dev\Deprecation;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject;
@ -22,7 +24,7 @@ class DeprecationTest extends SapphireTest
protected function setup(): void
{
// Use custom error handler for two reasons:
// - Filter out errors for deprecated class properities unrelated to this unit test
// - Filter out errors for deprecated class properties unrelated to this unit test
// - Allow the use of expectDeprecation(), which doesn't work with E_USER_DEPRECATION by default
// https://github.com/laminas/laminas-di/pull/30#issuecomment-927585210
parent::setup();
@ -32,7 +34,7 @@ class DeprecationTest extends SapphireTest
if (str_contains($errstr, 'SilverStripe\\Dev\\Tests\\DeprecationTest')) {
throw new Deprecated($errstr, $errno, '', 1);
} else {
// Surpress any E_USER_DEPRECATED unrelated to this unit-test
// Suppress any E_USER_DEPRECATED unrelated to this unit-test
return true;
}
}
@ -46,7 +48,9 @@ class DeprecationTest extends SapphireTest
protected function tearDown(): void
{
if (!$this->noticesWereEnabled) {
if ($this->noticesWereEnabled) {
Deprecation::enable();
} else {
Deprecation::disable();
}
restore_error_handler();
@ -278,4 +282,167 @@ class DeprecationTest extends SapphireTest
Config::modify()->merge(DeprecationTestObject::class, 'array_config', ['abc']);
Deprecation::outputNotices();
}
public function provideConfigVsEnv()
{
return [
// env var not set - config setting is respected
[
// false is returned when the env isn't set, so this simulates simply not having
// set the variable in the first place
'envVal' => 'notset',
'configVal' => false,
'expected' => false,
],
[
'envVal' => 'notset',
'configVal' => true,
'expected' => true,
],
// env var is set and truthy, config setting is ignored
[
'envVal' => true,
'configVal' => false,
'expected' => true,
],
[
'envVal' => true,
'configVal' => true,
'expected' => true,
],
// env var is set and falsy, config setting is ignored
[
'envVal' => false,
'configVal' => false,
'expected' => false,
],
[
'envVal' => false,
'configVal' => true,
'expected' => false,
],
];
}
/**
* @dataProvider provideConfigVsEnv
*/
public function testEnabledConfigVsEnv($envVal, bool $configVal, bool $expected)
{
$this->runConfigVsEnvTest('SS_DEPRECATION_ENABLED', $envVal, $configVal, $expected);
}
/**
* @dataProvider provideConfigVsEnv
*/
public function testShowHttpConfigVsEnv($envVal, bool $configVal, bool $expected)
{
$this->runConfigVsEnvTest('SS_DEPRECATION_SHOW_HTTP', $envVal, $configVal, $expected);
}
/**
* @dataProvider provideConfigVsEnv
*/
public function testShowCliConfigVsEnv($envVal, bool $configVal, bool $expected)
{
$this->runConfigVsEnvTest('SS_DEPRECATION_SHOW_CLI', $envVal, $configVal, $expected);
}
private function runConfigVsEnvTest(string $varName, $envVal, bool $configVal, bool $expected)
{
$oldVars = Environment::getVariables();
if ($envVal === 'notset') {
if (Environment::hasEnv($varName)) {
$this->markTestSkipped("$varName is set, so we can't test behaviour when it's not");
return;
}
} else {
Environment::setEnv($varName, $envVal);
}
switch ($varName) {
case 'SS_DEPRECATION_ENABLED':
if ($configVal) {
Deprecation::enable();
} else {
Deprecation::disable();
}
$result = Deprecation::isEnabled();
break;
case 'SS_DEPRECATION_SHOW_HTTP':
$oldShouldShow = Deprecation::shouldShowForHttp();
Deprecation::setShouldShowForHttp($configVal);
$result = Deprecation::shouldShowForHttp();
Deprecation::setShouldShowForHttp($oldShouldShow);
break;
case 'SS_DEPRECATION_SHOW_CLI':
$oldShouldShow = Deprecation::shouldShowForCli();
Deprecation::setShouldShowForCli($configVal);
$result = Deprecation::shouldShowForCli();
Deprecation::setShouldShowForCli($oldShouldShow);
break;
}
Environment::setVariables($oldVars);
$this->assertSame($expected, $result);
}
public function provideVarAsBoolean()
{
return [
[
'rawValue' => true,
'expected' => true,
],
[
'rawValue' => 'true',
'expected' => true,
],
[
'rawValue' => 1,
'expected' => true,
],
[
'rawValue' => '1',
'expected' => true,
],
[
'rawValue' => 'on',
'expected' => true,
],
[
'rawValue' => false,
'expected' => false,
],
[
'rawValue' => 'false',
'expected' => false,
],
[
'rawValue' => 0,
'expected' => false,
],
[
'rawValue' => '0',
'expected' => false,
],
[
'rawValue' => 'off',
'expected' => false,
],
];
}
/**
* @dataProvider provideVarAsBoolean
*/
public function testVarAsBoolean($rawValue, bool $expected)
{
$reflectionVarAsBoolean = new ReflectionMethod(Deprecation::class, 'varAsBoolean');
$reflectionVarAsBoolean->setAccessible(true);
$this->assertSame($expected, $reflectionVarAsBoolean->invoke(null, $rawValue));
}
}

130
tests/php/Logging/HTTPOutputHandlerTest.php
View File

@ -3,8 +3,11 @@
namespace SilverStripe\Logging\Tests;
use Monolog\Handler\HandlerInterface;
use ReflectionMethod;
use ReflectionProperty;
use SilverStripe\Control\Director;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\Deprecation;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Logging\DebugViewFriendlyErrorFormatter;
use SilverStripe\Logging\DetailedErrorFormatter;
@ -53,4 +56,131 @@ class HTTPOutputHandlerTest extends SapphireTest
$this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getDefaultFormatter());
$this->assertInstanceOf(DetailedErrorFormatter::class, $handler->getFormatter());
}
public function provideShouldShowError()
{
$provide = [];
// See https://www.php.net/manual/en/errorfunc.constants.php
$errors = [
E_ERROR,
E_WARNING,
E_PARSE,
E_NOTICE,
E_CORE_ERROR,
E_CORE_WARNING,
E_COMPILE_ERROR,
E_COMPILE_WARNING,
E_USER_ERROR,
E_USER_WARNING,
E_USER_NOTICE,
E_RECOVERABLE_ERROR,
E_DEPRECATED,
E_USER_DEPRECATED,
];
foreach ($errors as $errorCode) {
// Display all errors regardless of type in this scenario
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => true,
'isCli' => true,
'shouldShow' => true,
'expected' => true,
];
// Don't display E_USER_DEPRECATED that we're triggering if shouldShow is false
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => true,
'isCli' => true,
'shouldShow' => false,
'expected' => ($errorCode !== E_USER_DEPRECATED) || false
];
// Display all errors regardless of type in this scenario
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => true,
'isCli' => false,
'shouldShow' => true,
'expected' => true
];
// Don't display E_USER_DEPRECATED that we're triggering if shouldShow is false
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => true,
'isCli' => false,
'shouldShow' => false,
'expected' => ($errorCode !== E_USER_DEPRECATED) || false
];
// All of the below have triggeringError set to false, in which case
// all errors should be displayed.
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => false,
'isCli' => true,
'shouldShow' => true,
'expected' => true
];
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => false,
'isCli' => false,
'shouldShow' => true,
'expected' => true
];
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => false,
'isCli' => true,
'shouldShow' => false,
'expected' => true
];
$provide[] = [
'errorCode' => $errorCode,
'triggeringError' => false,
'isCli' => false,
'shouldShow' => false,
'expected' => true
];
}
return $provide;
}
/**
* @dataProvider provideShouldShowError
*/
public function testShouldShowError(
int $errorCode,
bool $triggeringError,
bool $isCli,
bool $shouldShow,
bool $expected
) {
$reflectionShouldShow = new ReflectionMethod(HTTPOutputHandler::class, 'shouldShowError');
$reflectionShouldShow->setAccessible(true);
$reflectionTriggeringError = new ReflectionProperty(Deprecation::class, 'isTriggeringError');
$reflectionTriggeringError->setAccessible(true);
$cliShouldShowOrig = Deprecation::shouldShowForCli();
$httpShouldShowOrig = Deprecation::shouldShowForHttp();
$triggeringErrorOrig = Deprecation::isTriggeringError();
// Set the relevant item using $shouldShow, and the other always as true
// to show that these don't interfere with each other
if ($isCli) {
Deprecation::setShouldShowForCli($shouldShow);
Deprecation::setShouldShowForHttp(true);
} else {
Deprecation::setShouldShowForCli(true);
Deprecation::setShouldShowForHttp($shouldShow);
}
$reflectionTriggeringError->setValue($triggeringError);
$mockHandler = $this->getMockBuilder(HTTPOutputHandler::class)->onlyMethods(['isCli'])->getMock();
$mockHandler->method('isCli')->willReturn($isCli);
$result = $reflectionShouldShow->invoke($mockHandler, $errorCode);
$this->assertSame($expected, $result);
Deprecation::setShouldShowForCli($cliShouldShowOrig);
Deprecation::setShouldShowForHttp($httpShouldShowOrig);
$reflectionTriggeringError->setValue($triggeringErrorOrig);
}
}