Merge pull request #10730 from creative-commoners/pulls/4.13/fix-depr

FIX Reduce array method calls
silverstripe · Mar 21, 2023 · 0d041e7 · 0d041e7
2 parents e47eedf + 41bb35f
commit 0d041e7
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 15 deletions.
diff --git a/src/Core/Environment.php b/src/Core/Environment.php
@@ -187,16 +187,22 @@ public static function getTimeLimitMax()
      */
     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 @@ public static function setEnv($name, $value)
      */
     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
@@ -237,8 +237,8 @@ public static function isEnabled(): bool
         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 @@ public static function setShouldShowForCli(bool $value): void
      */
     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 @@ public static function shouldShowForHttp(): bool
      */
     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
@@ -79,7 +79,6 @@ public function provideHasEnv()
         $valueOptions = [
             true,
             false,
-            null,
             0,
             1,
             1.75,
@@ -97,6 +96,12 @@ public function provideHasEnv()
                 ];
             }
         }
+        // `null` isn't a supported value outside of using the `.env` option.
+        $scenarios[] = [
+            'setAs' => '.env',
+            'value' => null,
+            'expected' => true,
+        ];
         $scenarios[] = [
             'setAs' => null,
             'value' => null,