From 7375a5082b93d8706738bfb5be1c6be28ffd879c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sun, 23 Oct 2022 20:38:47 +0200 Subject: [PATCH 1/3] Refactor handling environment variables to avoid duplicate logic --- src/Container.php | 24 +++++++++++++++--------- tests/ContainerTest.php | 2 +- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/Container.php b/src/Container.php index 23175ba..dc10643 100644 --- a/src/Container.php +++ b/src/Container.php @@ -98,12 +98,12 @@ public function getEnv(string $name): ?string { assert(\preg_match('/^[A-Z][A-Z0-9_]+$/', $name) === 1); - if (\is_array($this->container) && \array_key_exists($name, $this->container)) { - $value = $this->loadVariable($name, 'mixed', true, 64); - } elseif ($this->container instanceof ContainerInterface && $this->container->has($name)) { + if ($this->container instanceof ContainerInterface && $this->container->has($name)) { $value = $this->container->get($name); + } elseif ($this->hasVariable($name)) { + $value = $this->loadVariable($name, 'mixed', true, 64); } else { - $value = $_SERVER[$name] ?? null; + return null; } if (!\is_string($value) && $value !== null) { @@ -257,7 +257,7 @@ private function loadParameter(\ReflectionParameter $parameter, int $depth, bool // load container variables if parameter name is known assert($type === null || $type instanceof \ReflectionNamedType); - if ($allowVariables && (\array_key_exists($parameter->getName(), $this->container) || (isset($_SERVER[$parameter->getName()]) && \preg_match('/^[A-Z][A-Z0-9_]+$/', $parameter->getName())))) { + if ($allowVariables && $this->hasVariable($parameter->getName())) { return $this->loadVariable($parameter->getName(), $type === null ? 'mixed' : $type->getName(), $parameter->allowsNull(), $depth); } @@ -294,15 +294,21 @@ private function loadParameter(\ReflectionParameter $parameter, int $depth, bool return $this->loadObject($type->getName(), $depth - 1); } + private function hasVariable(string $name): bool + { + return (\is_array($this->container) && \array_key_exists($name, $this->container)) || (\is_string($_SERVER[$name] ?? null) && \preg_match('/^[A-Z][A-Z0-9_]+$/', $name)); + } + /** * @return object|string|int|float|bool|null * @throws \BadMethodCallException if $name is not a valid container variable */ private function loadVariable(string $name, string $type, bool $nullable, int $depth) /*: object|string|int|float|bool|null (PHP 8.0+) */ { - assert(\is_array($this->container) && (\array_key_exists($name, $this->container) || isset($_SERVER[$name]))); + assert($this->hasVariable($name)); + assert(\is_array($this->container) || !$this->container->has($name)); - if (($this->container[$name] ?? null) instanceof \Closure) { + if (\is_array($this->container) && ($this->container[$name] ?? null) instanceof \Closure) { if ($depth < 1) { throw new \BadMethodCallException('Container variable $' . $name . ' is recursive'); } @@ -321,10 +327,10 @@ private function loadVariable(string $name, string $type, bool $nullable, int $d } $this->container[$name] = $value; - } elseif (\array_key_exists($name, $this->container)) { + } elseif (\is_array($this->container) && \array_key_exists($name, $this->container)) { $value = $this->container[$name]; } else { - assert(isset($_SERVER[$name]) && \is_string($_SERVER[$name])); + assert(\is_string($_SERVER[$name] ?? null)); $value = $_SERVER[$name]; } diff --git a/tests/ContainerTest.php b/tests/ContainerTest.php index 26423a4..42f7b89 100644 --- a/tests/ContainerTest.php +++ b/tests/ContainerTest.php @@ -2077,7 +2077,7 @@ public function testGetEnvReturnsNullIfPsrContainerHasNoEntry(): void public function testGetEnvReturnsStringFromGlobalServerIfPsrContainerHasNoEntry(): void { $psr = $this->createMock(ContainerInterface::class); - $psr->expects($this->once())->method('has')->with('X_FOO')->willReturn(false); + $psr->expects($this->atLeastOnce())->method('has')->with('X_FOO')->willReturn(false); $psr->expects($this->never())->method('get'); assert($psr instanceof ContainerInterface); From 6500f7efa6da0201d912630995edb5ca6c326729 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sat, 29 Oct 2022 20:14:39 +0200 Subject: [PATCH 2/3] Load environment variables from `$_ENV` before checking `$_SERVER` --- src/Container.php | 5 +++- tests/ContainerTest.php | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/Container.php b/src/Container.php index dc10643..e423a19 100644 --- a/src/Container.php +++ b/src/Container.php @@ -296,7 +296,7 @@ private function loadParameter(\ReflectionParameter $parameter, int $depth, bool private function hasVariable(string $name): bool { - return (\is_array($this->container) && \array_key_exists($name, $this->container)) || (\is_string($_SERVER[$name] ?? null) && \preg_match('/^[A-Z][A-Z0-9_]+$/', $name)); + return (\is_array($this->container) && \array_key_exists($name, $this->container)) || (isset($_ENV[$name]) || (\is_string($_SERVER[$name] ?? null)) && \preg_match('/^[A-Z][A-Z0-9_]+$/', $name)); } /** @@ -329,6 +329,9 @@ private function loadVariable(string $name, string $type, bool $nullable, int $d $this->container[$name] = $value; } elseif (\is_array($this->container) && \array_key_exists($name, $this->container)) { $value = $this->container[$name]; + } elseif (isset($_ENV[$name])) { + assert(\is_string($_ENV[$name])); + $value = $_ENV[$name]; } else { assert(\is_string($_SERVER[$name] ?? null)); $value = $_SERVER[$name]; diff --git a/tests/ContainerTest.php b/tests/ContainerTest.php index 42f7b89..6e3d78f 100644 --- a/tests/ContainerTest.php +++ b/tests/ContainerTest.php @@ -2039,6 +2039,17 @@ public function testGetEnvReturnsStringFromMapFactory(): void $this->assertEquals('bar', $container->getEnv('X_FOO')); } + public function testGetEnvReturnsStringFromGlobalEnvIfNotSetInMap(): void + { + $container = new Container([]); + + $_ENV['X_FOO'] = 'bar'; + $ret = $container->getEnv('X_FOO'); + unset($_ENV['X_FOO']); + + $this->assertEquals('bar', $ret); + } + public function testGetEnvReturnsStringFromGlobalServerIfNotSetInMap(): void { $container = new Container([]); @@ -2050,6 +2061,18 @@ public function testGetEnvReturnsStringFromGlobalServerIfNotSetInMap(): void $this->assertEquals('bar', $ret); } + public function testGetEnvReturnsStringFromGlobalEnvBeforeServerIfNotSetInMap(): void + { + $container = new Container([]); + + $_ENV['X_FOO'] = 'foo'; + $_SERVER['X_FOO'] = 'bar'; + $ret = $container->getEnv('X_FOO'); + unset($_ENV['X_FOO'], $_SERVER['X_FOO']); + + $this->assertEquals('foo', $ret); + } + public function testGetEnvReturnsStringFromPsrContainer(): void { $psr = $this->createMock(ContainerInterface::class); @@ -2074,6 +2097,22 @@ public function testGetEnvReturnsNullIfPsrContainerHasNoEntry(): void $this->assertNull($container->getEnv('X_FOO')); } + public function testGetEnvReturnsStringFromGlobalEnvIfPsrContainerHasNoEntry(): void + { + $psr = $this->createMock(ContainerInterface::class); + $psr->expects($this->atLeastOnce())->method('has')->with('X_FOO')->willReturn(false); + $psr->expects($this->never())->method('get'); + + assert($psr instanceof ContainerInterface); + $container = new Container($psr); + + $_ENV['X_FOO'] = 'bar'; + $ret = $container->getEnv('X_FOO'); + unset($_ENV['X_FOO']); + + $this->assertEquals('bar', $ret); + } + public function testGetEnvReturnsStringFromGlobalServerIfPsrContainerHasNoEntry(): void { $psr = $this->createMock(ContainerInterface::class); @@ -2090,6 +2129,23 @@ public function testGetEnvReturnsStringFromGlobalServerIfPsrContainerHasNoEntry( $this->assertEquals('bar', $ret); } + public function testGetEnvReturnsStringFromGlobalEnvBeforeServerIfPsrContainerHasNoEntry(): void + { + $psr = $this->createMock(ContainerInterface::class); + $psr->expects($this->atLeastOnce())->method('has')->with('X_FOO')->willReturn(false); + $psr->expects($this->never())->method('get'); + + assert($psr instanceof ContainerInterface); + $container = new Container($psr); + + $_ENV['X_FOO'] = 'foo'; + $_SERVER['X_FOO'] = 'bar'; + $ret = $container->getEnv('X_FOO'); + unset($_ENV['X_FOO'], $_SERVER['X_FOO']); + + $this->assertEquals('foo', $ret); + } + public function testGetEnvThrowsIfMapContainsInvalidType(): void { $container = new Container([ From 55fe66c9018c4646c4b9f001764f4dcdcd9fc2d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sat, 29 Oct 2022 20:31:02 +0200 Subject: [PATCH 3/3] Load environment variables from live process environment if thread-safe --- src/Container.php | 15 ++++++++--- tests/ContainerTest.php | 58 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/Container.php b/src/Container.php index e423a19..2058379 100644 --- a/src/Container.php +++ b/src/Container.php @@ -13,6 +13,9 @@ class Container /** @var array|ContainerInterface */ private $container; + /** @var bool */ + private $useProcessEnv; + /** @param array|ContainerInterface $loader */ public function __construct($loader = []) { @@ -32,6 +35,9 @@ public function __construct($loader = []) } } $this->container = $loader; + + // prefer reading environment from `$_ENV` and `$_SERVER`, only fall back to `getenv()` in thread-safe environments + $this->useProcessEnv = \ZEND_THREAD_SAFE === false || \in_array(\PHP_SAPI, ['cli', 'cli-server', 'cgi-fcgi', 'fpm-fcgi'], true); } /** @return mixed */ @@ -296,7 +302,7 @@ private function loadParameter(\ReflectionParameter $parameter, int $depth, bool private function hasVariable(string $name): bool { - return (\is_array($this->container) && \array_key_exists($name, $this->container)) || (isset($_ENV[$name]) || (\is_string($_SERVER[$name] ?? null)) && \preg_match('/^[A-Z][A-Z0-9_]+$/', $name)); + return (\is_array($this->container) && \array_key_exists($name, $this->container)) || (isset($_ENV[$name]) || (\is_string($_SERVER[$name] ?? null) || ($this->useProcessEnv && \getenv($name) !== false)) && \preg_match('/^[A-Z][A-Z0-9_]+$/', $name)); } /** @@ -332,9 +338,12 @@ private function loadVariable(string $name, string $type, bool $nullable, int $d } elseif (isset($_ENV[$name])) { assert(\is_string($_ENV[$name])); $value = $_ENV[$name]; - } else { - assert(\is_string($_SERVER[$name] ?? null)); + } elseif (isset($_SERVER[$name])) { + assert(\is_string($_SERVER[$name])); $value = $_SERVER[$name]; + } else { + $value = \getenv($name); + assert($this->useProcessEnv && $value !== false); } assert(\is_object($value) || \is_scalar($value) || $value === null); diff --git a/tests/ContainerTest.php b/tests/ContainerTest.php index 6e3d78f..389757c 100644 --- a/tests/ContainerTest.php +++ b/tests/ContainerTest.php @@ -2061,6 +2061,17 @@ public function testGetEnvReturnsStringFromGlobalServerIfNotSetInMap(): void $this->assertEquals('bar', $ret); } + public function testGetEnvReturnsStringFromProcessEnvIfNotSetInMap(): void + { + $container = new Container([]); + + putenv('X_FOO=bar'); + $ret = $container->getEnv('X_FOO'); + putenv('X_FOO'); + + $this->assertEquals('bar', $ret); + } + public function testGetEnvReturnsStringFromGlobalEnvBeforeServerIfNotSetInMap(): void { $container = new Container([]); @@ -2073,6 +2084,19 @@ public function testGetEnvReturnsStringFromGlobalEnvBeforeServerIfNotSetInMap(): $this->assertEquals('foo', $ret); } + public function testGetEnvReturnsStringFromGlobalEnvBeforeProcessEnvIfNotSetInMap(): void + { + $container = new Container([]); + + $_ENV['X_FOO'] = 'foo'; + putenv('X_FOO=bar'); + $ret = $container->getEnv('X_FOO'); + unset($_ENV['X_FOO']); + putenv('X_FOO'); + + $this->assertEquals('foo', $ret); + } + public function testGetEnvReturnsStringFromPsrContainer(): void { $psr = $this->createMock(ContainerInterface::class); @@ -2097,6 +2121,22 @@ public function testGetEnvReturnsNullIfPsrContainerHasNoEntry(): void $this->assertNull($container->getEnv('X_FOO')); } + public function testGetEnvReturnsStringFromProcessEnvIfPsrContainerHasNoEntry(): void + { + $psr = $this->createMock(ContainerInterface::class); + $psr->expects($this->atLeastOnce())->method('has')->with('X_FOO')->willReturn(false); + $psr->expects($this->never())->method('get'); + + assert($psr instanceof ContainerInterface); + $container = new Container($psr); + + putenv('X_FOO=bar'); + $ret = $container->getEnv('X_FOO'); + putenv('X_FOO'); + + $this->assertEquals('bar', $ret); + } + public function testGetEnvReturnsStringFromGlobalEnvIfPsrContainerHasNoEntry(): void { $psr = $this->createMock(ContainerInterface::class); @@ -2146,6 +2186,24 @@ public function testGetEnvReturnsStringFromGlobalEnvBeforeServerIfPsrContainerHa $this->assertEquals('foo', $ret); } + public function testGetEnvReturnsStringFromGlobalEnvBeforeProcessEnvIfPsrContainerHasNoEntry(): void + { + $psr = $this->createMock(ContainerInterface::class); + $psr->expects($this->atLeastOnce())->method('has')->with('X_FOO')->willReturn(false); + $psr->expects($this->never())->method('get'); + + assert($psr instanceof ContainerInterface); + $container = new Container($psr); + + $_ENV['X_FOO'] = 'foo'; + putenv('X_FOO=bar'); + $ret = $container->getEnv('X_FOO'); + unset($_ENV['X_FOO']); + putenv('X_FOO'); + + $this->assertEquals('foo', $ret); + } + public function testGetEnvThrowsIfMapContainsInvalidType(): void { $container = new Container([