FIX Ensure environment is checked before enabling deprecations

silverstripe · Nov 19, 2023 · 36c6394 · 36c6394
1 parent bbc6167
commit 36c6394
Show file tree

Hide file tree

Showing 2 changed files with 117 additions and 3 deletions.
diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php
@@ -234,14 +234,26 @@ public static function get_enabled()
 
     public static function isEnabled(): bool
     {
+        // Return early if disabled via env variable
+        $envEnabled = false;
         if (Environment::hasEnv('SS_DEPRECATION_ENABLED')) {
-            $envVar = Environment::getEnv('SS_DEPRECATION_ENABLED');
-            return self::varAsBoolean($envVar);
+            $envEnabled = Environment::getEnv('SS_DEPRECATION_ENABLED');
+            if (!$envEnabled) {
+                return false;
+            }
+        }
+
+        // Environment enabled overrides static disabled - return early if disabled.
+        if (!$envEnabled && !static::$currentlyEnabled) {
+            return false;
         }
+
+        // If it's enabled, explicitly don't allow for non-dev environments
         if (!Director::isDev()) {
             return false;
         }
-        return static::$currentlyEnabled;
+
+        return true;
     }
 
     /**

diff --git a/tests/php/Dev/DeprecationTest.php b/tests/php/Dev/DeprecationTest.php
@@ -4,12 +4,14 @@
 
 use PHPUnit\Framework\Error\Deprecated;
 use ReflectionMethod;
+use ReflectionProperty;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Environment;
 use SilverStripe\Dev\Deprecation;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\Dev\Tests\DeprecationTest\DeprecationTestObject;
 use SilverStripe\Core\Injector\Injector;
+use SilverStripe\Core\Kernel;
 
 class DeprecationTest extends SapphireTest
 {
@@ -445,4 +447,104 @@ public function testVarAsBoolean($rawValue, bool $expected)
 
         $this->assertSame($expected, $reflectionVarAsBoolean->invoke(null, $rawValue));
     }
+
+    public function provideIsEnabled()
+    {
+        return [
+            'dev, explicitly enabled' => [
+                'envMode' => 'dev',
+                'envEnabled' => true,
+                'staticEnabled' => true,
+                'expected' => true,
+            ],
+            'dev, explicitly enabled override static' => [
+                'envMode' => 'dev',
+                'envEnabled' => true,
+                'staticEnabled' => false,
+                'expected' => true,
+            ],
+            'dev, explicitly disabled override static' => [
+                'envMode' => 'dev',
+                'envEnabled' => false,
+                'staticEnabled' => true,
+                'expected' => false,
+            ],
+            'dev, explicitly disabled' => [
+                'envMode' => 'dev',
+                'envEnabled' => false,
+                'staticEnabled' => false,
+                'expected' => false,
+            ],
+            'dev, statically disabled' => [
+                'envMode' => 'dev',
+                'envEnabled' => null,
+                'staticEnabled' => true,
+                'expected' => true,
+            ],
+            'dev, statically disabled' => [
+                'envMode' => 'dev',
+                'envEnabled' => null,
+                'staticEnabled' => false,
+                'expected' => false,
+            ],
+            'live, explicitly enabled' => [
+                'envMode' => 'live',
+                'envEnabled' => true,
+                'staticEnabled' => true,
+                'expected' => false,
+            ],
+            'live, explicitly disabled' => [
+                'envMode' => 'live',
+                'envEnabled' => false,
+                'staticEnabled' => false,
+                'expected' => false,
+            ],
+            'live, statically disabled' => [
+                'envMode' => 'live',
+                'envEnabled' => null,
+                'staticEnabled' => true,
+                'expected' => false,
+            ],
+            'live, statically disabled' => [
+                'envMode' => 'live',
+                'envEnabled' => null,
+                'staticEnabled' => false,
+                'expected' => false,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideIsEnabled
+     */
+    public function testIsEnabled(string $envMode, ?bool $envEnabled, bool $staticEnabled, bool $expected)
+    {
+        /** @var Kernel $kernel */
+        $kernel = Injector::inst()->get(Kernel::class);
+        $origMode = $kernel->getEnvironment();
+        $origEnvEnabled = Environment::getEnv('SS_DEPRECATION_ENABLED');
+        $reflectionEnabled = new ReflectionProperty(Deprecation::class, 'currentlyEnabled');
+        $reflectionEnabled->setAccessible(true);
+        $origStaticEnabled = $reflectionEnabled->getValue();
+
+        try {
+            $kernel->setEnvironment($envMode);
+            Environment::setEnv('SS_DEPRECATION_ENABLED', $envEnabled);
+            $this->setEnabledViaStatic($staticEnabled);
+            $this->assertSame($expected, Deprecation::isEnabled());
+        } finally {
+            $kernel->setEnvironment($origMode);
+            Environment::setEnv('SS_DEPRECATION_ENABLED', $origEnvEnabled);
+            $this->setEnabledViaStatic($origStaticEnabled);
+        }
+    }
+
+    private function setEnabledViaStatic(bool $enabled): void
+    {
+        if ($enabled) {
+            Deprecation::enable();
+        } else {
+            Deprecation::disable();
+        }
+    }
 }