From 7eab49f850070e1a2df5f7a2e38baad031aa1d54 Mon Sep 17 00:00:00 2001
From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com>
Date: Wed, 22 Nov 2023 09:30:08 +1300
Subject: [PATCH] FIX Ensure environment is checked before enabling
 deprecations (#11055)

---
 src/Dev/Deprecation.php           |  17 +++--
 tests/php/Dev/DeprecationTest.php | 102 ++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 4 deletions(-)

diff --git a/src/Dev/Deprecation.php b/src/Dev/Deprecation.php
index 6df48894a..5afa962d7 100644
--- a/src/Dev/Deprecation.php
+++ b/src/Dev/Deprecation.php
@@ -234,14 +234,23 @@ class Deprecation
 
     public static function isEnabled(): bool
     {
-        if (Environment::hasEnv('SS_DEPRECATION_ENABLED')) {
-            $envVar = Environment::getEnv('SS_DEPRECATION_ENABLED');
-            return self::varAsBoolean($envVar);
+        $hasEnv = Environment::hasEnv('SS_DEPRECATION_ENABLED');
+
+        // Return early if disabled
+        if ($hasEnv && !Environment::getEnv('SS_DEPRECATION_ENABLED')) {
+            return false;
         }
+        if (!$hasEnv && !static::$currentlyEnabled) {
+            // Static property is ignored if SS_DEPRECATION_ENABLED was set
+            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
index 04f053170..30d6b2018 100644
--- a/tests/php/Dev/DeprecationTest.php
+++ b/tests/php/Dev/DeprecationTest.php
@@ -4,12 +4,14 @@ namespace SilverStripe\Dev\Tests;
 
 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 @@ class DeprecationTest extends SapphireTest
 
         $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();
+        }
+    }
 }