From 128b327c6dd974bd8a61643d9acb625d9a7ea46c Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 2 Mar 2023 13:47:13 +1300 Subject: [PATCH 01/11] API Add method to check if env var is set --- src/Core/Environment.php | 16 +++++- tests/php/Core/EnvironmentTest.php | 84 ++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/src/Core/Environment.php b/src/Core/Environment.php index ccae510e6..84bf2cfe7 100644 --- a/src/Core/Environment.php +++ b/src/Core/Environment.php @@ -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 * diff --git a/tests/php/Core/EnvironmentTest.php b/tests/php/Core/EnvironmentTest.php index cf92c8787..e5875fd41 100644 --- a/tests/php/Core/EnvironmentTest.php +++ b/tests/php/Core/EnvironmentTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Core\Tests; +use ReflectionProperty; use SilverStripe\Core\Environment; use SilverStripe\Dev\SapphireTest; @@ -66,4 +67,87 @@ 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': + if ($value === null) { + $val = 'null'; + } elseif (is_bool($value)) { + $val = $value ? 'true' : 'false'; + } else { + $val = (string)$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"); + } } From 046befc4ba3c87810bf195cfacbf106d59a9d9ba Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 21 Feb 2023 14:41:02 +1300 Subject: [PATCH 02/11] ENH Improve deprecation logging --- src/Dev/Deprecation.php | 163 +++++++++++++++--- src/Logging/HTTPOutputHandler.php | 27 +++ src/Logging/MonologErrorHandler.php | 9 +- tests/php/Core/EnvironmentTest.php | 8 +- tests/php/Dev/DeprecationTest.php | 173 +++++++++++++++++++- tests/php/Logging/HTTPOutputHandlerTest.php | 130 +++++++++++++++ 6 files changed, 480 insertions(+), 30 deletions(-) diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php index 66b39c070..7e3a545dd 100644 --- a/src/Dev/Deprecation.php +++ b/src/Dev/Deprecation.php @@ -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; + } } diff --git a/src/Logging/HTTPOutputHandler.php b/src/Logging/HTTPOutputHandler.php index b09a671f3..c5115f43d 100644 --- a/src/Logging/HTTPOutputHandler.php +++ b/src/Logging/HTTPOutputHandler.php @@ -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(); + } } diff --git a/src/Logging/MonologErrorHandler.php b/src/Logging/MonologErrorHandler.php index d9bbd3ba9..0d0c10285 100644 --- a/src/Logging/MonologErrorHandler.php +++ b/src/Logging/MonologErrorHandler.php @@ -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); } } } diff --git a/tests/php/Core/EnvironmentTest.php b/tests/php/Core/EnvironmentTest.php index e5875fd41..221b2563a 100644 --- a/tests/php/Core/EnvironmentTest.php +++ b/tests/php/Core/EnvironmentTest.php @@ -124,13 +124,7 @@ class EnvironmentTest extends SapphireTest $_SERVER[$name] = $value; break; case 'putenv': - if ($value === null) { - $val = 'null'; - } elseif (is_bool($value)) { - $val = $value ? 'true' : 'false'; - } else { - $val = (string)$value; - } + $val = is_string($value) ? $value : json_encode($value); putenv("$name=$val"); break; default: diff --git a/tests/php/Dev/DeprecationTest.php b/tests/php/Dev/DeprecationTest.php index 8b0cf8184..04f053170 100644 --- a/tests/php/Dev/DeprecationTest.php +++ b/tests/php/Dev/DeprecationTest.php @@ -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)); + } } diff --git a/tests/php/Logging/HTTPOutputHandlerTest.php b/tests/php/Logging/HTTPOutputHandlerTest.php index 4dfa8009d..e72eaf95a 100644 --- a/tests/php/Logging/HTTPOutputHandlerTest.php +++ b/tests/php/Logging/HTTPOutputHandlerTest.php @@ -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); + } } From a387c9b9e4bb69e04b3350a8ecb14672d0952a58 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 10 Mar 2023 12:21:27 +1300 Subject: [PATCH 03/11] MNT Update development dependencies --- composer.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index e71bcc6cc..9f582536b 100644 --- a/composer.json +++ b/composer.json @@ -33,8 +33,8 @@ "monolog/monolog": "^1.16", "nikic/php-parser": "^4.10.5", "psr/container": "^1", - "silverstripe/config": "^1@dev", - "silverstripe/assets": "^1@dev", + "silverstripe/config": "1.6.x-dev", + "silverstripe/assets": "1.13.x-dev", "silverstripe/vendor-plugin": "^1.6", "sminnee/callbacklist": "^0.1", "swiftmailer/swiftmailer": "^6.2", @@ -115,4 +115,4 @@ }, "minimum-stability": "dev", "prefer-stable": true -} +} \ No newline at end of file From 75b7622a2144000e96e4ab04f7032ef3def04a4e Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 10 Mar 2023 16:29:40 +1300 Subject: [PATCH 04/11] MNT Update release dependencies --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 9f582536b..53b3dd05e 100644 --- a/composer.json +++ b/composer.json @@ -33,8 +33,8 @@ "monolog/monolog": "^1.16", "nikic/php-parser": "^4.10.5", "psr/container": "^1", - "silverstripe/config": "1.6.x-dev", - "silverstripe/assets": "1.13.x-dev", + "silverstripe/config": "1.6.0-beta1", + "silverstripe/assets": "1.13.0-beta1", "silverstripe/vendor-plugin": "^1.6", "sminnee/callbacklist": "^0.1", "swiftmailer/swiftmailer": "^6.2", From 77cbe20ba9dbe9343c522666eaca5b8dc27c7a48 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Fri, 10 Mar 2023 16:29:44 +1300 Subject: [PATCH 05/11] MNT Update development dependencies --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 53b3dd05e..9f582536b 100644 --- a/composer.json +++ b/composer.json @@ -33,8 +33,8 @@ "monolog/monolog": "^1.16", "nikic/php-parser": "^4.10.5", "psr/container": "^1", - "silverstripe/config": "1.6.0-beta1", - "silverstripe/assets": "1.13.0-beta1", + "silverstripe/config": "1.6.x-dev", + "silverstripe/assets": "1.13.x-dev", "silverstripe/vendor-plugin": "^1.6", "sminnee/callbacklist": "^0.1", "swiftmailer/swiftmailer": "^6.2", From 5b8d61b55b49ccc84d6005eb503c0c4f5a417b52 Mon Sep 17 00:00:00 2001 From: zemiacsik Date: Mon, 13 Mar 2023 13:51:48 +0100 Subject: [PATCH 06/11] FIX property_exists() parameters mixup property_exists() has first parameter "object_or_class" and second is a property to check --- src/ORM/ArrayList.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index 83e448b93..8483c50c2 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -604,7 +604,7 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L $firstRecord = $this->first(); - return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($by, $firstRecord ?? ''); + return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($firstRecord ?? '', $by); } /** From d60af9d16e4a118b6fdca5d601e2b163ca186979 Mon Sep 17 00:00:00 2001 From: zemiacsik Date: Tue, 14 Mar 2023 08:42:22 +0100 Subject: [PATCH 07/11] FIX property_exists() parameters mixup ensure that property parameter is a string --- src/ORM/ArrayList.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index 8483c50c2..c5c478326 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -604,7 +604,7 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L $firstRecord = $this->first(); - return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($firstRecord ?? '', $by); + return is_array($firstRecord) ? array_key_exists($by, $firstRecord) : property_exists($firstRecord, $by ?? ''); } /** From 241b58cd65c9300061c17c31bb252f95f5068e39 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Tue, 21 Mar 2023 14:22:01 +1300 Subject: [PATCH 08/11] MNT Use gha-dispatch-ci --- .github/workflows/ci.yml | 5 ----- .github/workflows/dispatch-ci.yml | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/dispatch-ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f681e1e32..b2f9a0ba4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,15 +4,10 @@ on: push: pull_request: workflow_dispatch: - # Every Tuesday at 2:20pm UTC - schedule: - - cron: '20 14 * * 2' jobs: ci: name: CI - # Only run cron on the silverstripe account - if: (github.event_name == 'schedule' && github.repository_owner == 'silverstripe') || (github.event_name != 'schedule') uses: silverstripe/gha-ci/.github/workflows/ci.yml@v1 with: # Turn phpcoverage off because it causes a segfault diff --git a/.github/workflows/dispatch-ci.yml b/.github/workflows/dispatch-ci.yml new file mode 100644 index 000000000..d9091c496 --- /dev/null +++ b/.github/workflows/dispatch-ci.yml @@ -0,0 +1,16 @@ +name: Dispatch CI + +on: + # At 2:20 PM UTC, only on Tuesday and Wednesday + schedule: + - cron: '20 14 * * 2,3' + +jobs: + dispatch-ci: + name: Dispatch CI + # Only run cron on the silverstripe account + if: (github.event_name == 'schedule' && github.repository_owner == 'silverstripe') || (github.event_name != 'schedule') + runs-on: ubuntu-latest + steps: + - name: Dispatch CI + uses: silverstripe/gha-dispatch-ci@v1 From 41bb35f3f39d567f720a88ac7e58264a58f24ee5 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Wed, 22 Mar 2023 11:06:23 +1300 Subject: [PATCH 09/11] FIX Reduce array method calls --- src/Core/Environment.php | 29 ++++++++++++++++++----------- src/Dev/Deprecation.php | 6 +++--- tests/php/Core/EnvironmentTest.php | 7 ++++++- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/src/Core/Environment.php b/src/Core/Environment.php index 84bf2cfe7..a6b0fc7ef 100644 --- a/src/Core/Environment.php +++ b/src/Core/Environment.php @@ -187,16 +187,22 @@ class Environment */ public static function getEnv($name) { - switch (true) { - case is_array(static::$env) && array_key_exists($name, static::$env): - return static::$env[$name]; - case is_array($_ENV) && array_key_exists($name, $_ENV): - return $_ENV[$name]; - case is_array($_SERVER) && array_key_exists($name, $_SERVER): - return $_SERVER[$name]; - default: - return getenv($name); + if (array_key_exists($name, static::$env)) { + return static::$env[$name]; } + // isset() is used for $_ENV and $_SERVER instead of array_key_exists() to fix a very strange issue that + // occured in CI running silverstripe/recipe-kitchen-sink where PHP would timeout due apparently due to an + // excessively high number of array method calls. isset() is not used for static::$env above because + // values there may be null, and isset() will return false for null values + // Symfony also uses isset() for reading $_ENV and $_SERVER values + // https://github.com/symfony/dependency-injection/blob/6.2/EnvVarProcessor.php#L148 + if (isset($_ENV[$name])) { + return $_ENV[$name]; + } + if (isset($_SERVER[$name])) { + return $_SERVER[$name]; + } + return getenv($name); } /** @@ -231,9 +237,10 @@ class Environment */ public static function hasEnv(string $name): bool { + // See getEnv() for an explanation of why isset() is used for $_ENV and $_SERVER return array_key_exists($name, static::$env) - || is_array($_ENV) && array_key_exists($name, $_ENV) - || is_array($_SERVER) && array_key_exists($name, $_SERVER) + || isset($_ENV[$name]) + || isset($_SERVER[$name]) || getenv($name) !== false; } diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php index 7e3a545dd..d74b7bf45 100644 --- a/src/Dev/Deprecation.php +++ b/src/Dev/Deprecation.php @@ -237,8 +237,8 @@ class Deprecation if (!Director::isDev()) { return false; } - $envVar = Environment::getEnv('SS_DEPRECATION_ENABLED'); if (Environment::hasEnv('SS_DEPRECATION_ENABLED')) { + $envVar = Environment::getEnv('SS_DEPRECATION_ENABLED'); return self::varAsBoolean($envVar); } return static::$currentlyEnabled; @@ -293,8 +293,8 @@ class Deprecation */ public static function shouldShowForHttp(): bool { - $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_HTTP'); if (Environment::hasEnv('SS_DEPRECATION_SHOW_HTTP')) { + $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_HTTP'); return self::varAsBoolean($envVar); } return self::$shouldShowForHttp; @@ -306,8 +306,8 @@ class Deprecation */ public static function shouldShowForCli(): bool { - $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_CLI'); if (Environment::hasEnv('SS_DEPRECATION_SHOW_CLI')) { + $envVar = Environment::getEnv('SS_DEPRECATION_SHOW_CLI'); return self::varAsBoolean($envVar); } return self::$shouldShowForCli; diff --git a/tests/php/Core/EnvironmentTest.php b/tests/php/Core/EnvironmentTest.php index 221b2563a..7f864710b 100644 --- a/tests/php/Core/EnvironmentTest.php +++ b/tests/php/Core/EnvironmentTest.php @@ -79,7 +79,6 @@ class EnvironmentTest extends SapphireTest $valueOptions = [ true, false, - null, 0, 1, 1.75, @@ -97,6 +96,12 @@ class EnvironmentTest extends SapphireTest ]; } } + // `null` isn't a supported value outside of using the `.env` option. + $scenarios[] = [ + 'setAs' => '.env', + 'value' => null, + 'expected' => true, + ]; $scenarios[] = [ 'setAs' => null, 'value' => null, From 0f40cc38ecf17f572610efdc0e3e1edf253ce0a8 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 23 Mar 2023 10:57:03 +1300 Subject: [PATCH 10/11] FIX Respect searchable_fields --- src/Forms/GridField/GridFieldFilterHeader.php | 24 ++++++++++------- .../GridField/GridFieldFilterHeaderTest.php | 26 +++++++++++++++++++ .../GridFieldFilterHeaderTest/Team.php | 5 ++++ 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/Forms/GridField/GridFieldFilterHeader.php b/src/Forms/GridField/GridFieldFilterHeader.php index 58599b2fe..6292985be 100755 --- a/src/Forms/GridField/GridFieldFilterHeader.php +++ b/src/Forms/GridField/GridFieldFilterHeader.php @@ -254,21 +254,25 @@ class GridFieldFilterHeader extends AbstractGridFieldComponent implements GridFi public function canFilterAnyColumns($gridField) { $list = $gridField->getList(); - - if (!$this->checkDataType($list)) { + if (!($list instanceof Filterable) || !$this->checkDataType($list)) { return false; } - - $columns = $gridField->getColumns(); - foreach ($columns as $columnField) { - $metadata = $gridField->getColumnMetadata($columnField); - $title = $metadata['title']; - - if ($title && $list->canFilterBy($columnField)) { + $modelClass = $gridField->getModelClass(); + // note: searchableFields() will return summary_fields if there are no searchable_fields on the model + $searchableFields = array_keys($modelClass::singleton()->searchableFields()); + $summaryFields = array_keys($modelClass::singleton()->summaryFields()); + sort($searchableFields); + sort($summaryFields); + // searchable_fields has been explictily defined i.e. searchableFields() is not falling back to summary_fields + if ($searchableFields !== $summaryFields) { + return true; + } + // we have fallen back to summary_fields, check they are filterable + foreach ($searchableFields as $searchableField) { + if ($list->canFilterBy($searchableField)) { return true; } } - return false; } diff --git a/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php b/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php index db70ddddb..bf6c95b8c 100644 --- a/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php +++ b/tests/php/Forms/GridField/GridFieldFilterHeaderTest.php @@ -182,4 +182,30 @@ class GridFieldFilterHeaderTest extends SapphireTest $this->assertEquals('ReallyCustomSearch', $this->component->getSearchField()); } + + public function testCanFilterAnyColumns() + { + $gridField = $this->gridField; + $filterHeader = $gridField->getConfig()->getComponentByType(GridFieldFilterHeader::class); + + // test that you can filter by something if searchable_fields is not defined + // silverstripe will scaffold db columns that are in the gridfield to be + // searchable by default + Config::modify()->remove(Team::class, 'searchable_fields'); + $this->assertTrue($filterHeader->canFilterAnyColumns($gridField)); + + // test that you can filterBy if searchable_fields is defined + Config::modify()->set(Team::class, 'searchable_fields', ['Name']); + $this->assertTrue($filterHeader->canFilterAnyColumns($gridField)); + + // test that you can filterBy if searchable_fields even if it is not a legit field + // this is because we're making a blind assumption it will be filterable later in a SearchContext + Config::modify()->set(Team::class, 'searchable_fields', ['WhatIsThis']); + $this->assertTrue($filterHeader->canFilterAnyColumns($gridField)); + + // test that you cannot filter by non-db field when it falls back to summary_fields + Config::modify()->remove(Team::class, 'searchable_fields'); + Config::modify()->set(Team::class, 'summary_fields', ['MySummaryField']); + $this->assertFalse($filterHeader->canFilterAnyColumns($gridField)); + } } diff --git a/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php b/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php index 98d587975..b39eefe56 100644 --- a/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php +++ b/tests/php/Forms/GridField/GridFieldFilterHeaderTest/Team.php @@ -24,4 +24,9 @@ class Team extends DataObject implements TestOnly 'Cheerleader' => Cheerleader::class, 'CheerleadersMom' => Mom::class ]; + + public function getMySummaryField() + { + return 'MY SUMMARY FIELD'; + } } From 600f188287d6a20680a0c82da3da4c560a6b5278 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Tue, 28 Mar 2023 16:46:46 +1300 Subject: [PATCH 11/11] MNT Revert erroneous dependency changes (#10739) --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 9f582536b..4aa81241e 100644 --- a/composer.json +++ b/composer.json @@ -33,8 +33,8 @@ "monolog/monolog": "^1.16", "nikic/php-parser": "^4.10.5", "psr/container": "^1", - "silverstripe/config": "1.6.x-dev", - "silverstripe/assets": "1.13.x-dev", + "silverstripe/config": "^1@dev", + "silverstripe/assets": "^1@dev", "silverstripe/vendor-plugin": "^1.6", "sminnee/callbacklist": "^0.1", "swiftmailer/swiftmailer": "^6.2",