Merge pull request #52 from silktide/recursion-check

[Bugfix] Better Parameter Resolution
silktide · Feb 16, 2021 · 58ce567 · 58ce567
2 parents 09a0142 + b163626
commit 58ce567
Show file tree

Hide file tree

Showing 25 changed files with 158 additions and 78 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,5 +1,4 @@
 /.idea
 /vendor
 /composer.lock
-/old
 /.phpunit.result.cache
diff --git a/composer.json b/composer.json
@@ -6,14 +6,15 @@
   "require": {
     "php": ">=7.4",
     "ext-json": "*",
+    "ext-mbstring": "*",
     "pimple/pimple": "~3.0",
     "symfony/yaml": "^2.4|~3|~4|~5",
     "psr/simple-cache": "^1.0",
     "ocramius/proxy-manager": "^2.8"
   },
   "require-dev": {
-    "phpunit/phpunit": "~8",
-    "phpstan/phpstan": "^0.10.2",
+    "phpunit/phpunit": "~9",
+    "phpstan/phpstan": "~0",
     "mikey179/vfsstream": "^1.6",
     "cache/array-adapter": "^1.0",
     "cache/filesystem-adapter": "^1.0"

diff --git a/phpunit.xml b/phpunit.xml
@@ -1,9 +1,8 @@
 <?xml version="1.0" encoding="utf-8"?>
-<phpunit bootstrap="tests/bootstrap.php" >
+<phpunit bootstrap="vendor/autoload.php" >
     <testsuites>
         <testsuite name="tests">
             <directory>tests</directory>
-            <exclude>tests/bootstrap.php</exclude>
         </testsuite>
     </testsuites>
 </phpunit>
diff --git a/src/CompiledConfigBuilder.php b/src/CompiledConfigBuilder.php
@@ -98,7 +98,9 @@ public function build(MasterConfig $masterConfig, array $parameters = [])
         $tagMap = [];
         foreach ($parameters as $key => $value) {
             // Resolve all the parameters up front
-            $parameters[$key] = $parameterResolver->resolve($parameters, $value, $tagMap);
+            $parameters[$key] = is_array($value) ?
+                $parameterResolver->resolveArray($parameters, $value, $tagMap) :
+                $parameterResolver->resolve($parameters, $value, $tagMap);
         }
 
         foreach ($services as $serviceName => &$definition) {

diff --git a/src/ContainerBuilder.php b/src/ContainerBuilder.php
@@ -81,7 +81,7 @@ private function buildService(Container $container, array $definition)
         if ($isFactoryCreated) {
             $service = call_user_func_array(
                 [
-                    $definition["factoryClass"] ?? $container->offsetGet(mb_substr($definition["factoryService"], 1)),
+                    $definition["factoryClass"] ?? $container->offsetGet(\mb_substr($definition["factoryService"], 1)),
                     $definition["factoryMethod"]
                 ],
                 $arguments
@@ -103,9 +103,9 @@ protected function resolveArray(Container $container, array $array)
     {
         $arguments = [];
         foreach ($array as $k => $value) {
-            if (is_string($value) && strlen($value) > 0 && $value[0] === "\0") {
-                $arguments[$k] = $container->offsetGet(mb_substr($value, 1));
-            } elseif (is_array($value)) {
+            if (\is_string($value) && \strlen($value) > 0 && $value[0] === "\0") {
+                $arguments[$k] = $container->offsetGet(\mb_substr($value, 1));
+            } elseif (\is_array($value)) {
                 $arguments[$k] = $this->resolveArray($container, $value);
             } else {
                 $arguments[$k] = $value;

diff --git a/src/FileConfig.php b/src/FileConfig.php
@@ -237,26 +237,26 @@ protected function namespace(string $string)
 
     protected function isNamespaced(string $key)
     {
-        return mb_strpos($key, Token::NAMESPACE_SEPARATOR) !== false;
+        return \mb_strpos($key, Token::NAMESPACE_SEPARATOR) !== false;
     }
 
     protected function recursivelyNamespace($value)
     {
-        if (is_array($value)) {
+        if (\is_array($value)) {
             $return = [];
             foreach ($value as $k => $v) {
                 $return[$k] = $this->recursivelyNamespace($v);
             }
             return $return;
-        } elseif (is_string($value) && strlen($value) > 0) {
+        } elseif (\is_string($value) && \strlen($value) > 0) {
             if ($value[0] === Token::SERVICE_CHAR) {
                 return Token::SERVICE_CHAR . $this->namespace(mb_substr($value, 1));
-            } elseif (mb_strpos($value, Token::PARAMETER_CHAR) !== false) {
+            } elseif (\mb_strpos($value, Token::PARAMETER_CHAR) !== false) {
                 $replacements = [];
                 $n = 0;
-                while (mb_substr_count($value, Token::PARAMETER_CHAR) > 1) {
-                    $pos1 = mb_strpos($value, Token::PARAMETER_CHAR);
-                    $pos2 = mb_strpos($value, Token::PARAMETER_CHAR, $pos1 + 1);
+                while (\mb_substr_count($value, Token::PARAMETER_CHAR) > 1) {
+                    $pos1 = \mb_strpos($value, Token::PARAMETER_CHAR);
+                    $pos2 = \mb_strpos($value, Token::PARAMETER_CHAR, $pos1 + 1);
 
                     $placeholder = '|||'.$n.'|||';
 
@@ -265,12 +265,12 @@ protected function recursivelyNamespace($value)
                         $this->namespace(mb_substr($value, $pos1 + 1, $pos2 - ($pos1 + 1))) .
                         Token::PARAMETER_CHAR;
 
-                    $value = mb_substr($value, 0, $pos1) . $placeholder . mb_substr($value, $pos2 + 1);
+                    $value = \mb_substr($value, 0, $pos1) . $placeholder . \mb_substr($value, $pos2 + 1);
                     $n++;
                 }
 
                 foreach ($replacements as $search => $replace) {
-                    $value = str_replace($search, $replace, $value);
+                    $value = \str_replace($search, $replace, $value);
                 }
             }
         }

diff --git a/src/FileStateCollection.php b/src/FileStateCollection.php
@@ -4,7 +4,7 @@
 
 class FileStateCollection
 {
-    protected $state = [];
+    protected array $state = [];
 
     public function __construct(array $filenames)
     {

diff --git a/src/Loader/YamlLoader.php b/src/Loader/YamlLoader.php
@@ -9,9 +9,9 @@
 
 class YamlLoader implements LoaderInterface
 {
-    protected $hasSymfony;
-    protected $hasExtension;
-    protected $forceSymfony;
+    protected bool $hasSymfony;
+    protected bool $hasExtension;
+    protected bool $forceSymfony;
 
     public function __construct(bool $useSymfony = false)
     {

diff --git a/src/MasterConfig.php b/src/MasterConfig.php
@@ -8,10 +8,10 @@
 
 class MasterConfig
 {
-    protected $filenames = [];
-    protected $services = [];
-    protected $parameters = [];
-    protected $extensions = [];
+    protected array $filenames = [];
+    protected array $services = [];
+    protected array $parameters = [];
+    protected array $extensions = [];
 
     public function addFileConfig(FileConfig $fileConfig)
     {
@@ -71,9 +71,8 @@ public function getExtensions()
      * Unfortunately, all of PHPs underlying sorting is done by QuickSort, so we have to do the sorting ourselves
      *
      * @param array $array
-     * @return array
      */
-    protected function stableWeightSort(array &$array)
+    protected function stableWeightSort(array &$array) : void
     {
         foreach ($array as $i => &$value) {
             $value["key"] = $i;

diff --git a/src/ParameterResolver.php b/src/ParameterResolver.php
@@ -17,14 +17,14 @@ class ParameterResolver
     public function resolveArray(array $parameters, array $array, array &$tagMap)
     {
         foreach ($array as $k => $v) {
-            $array[$k] = is_array($v) ? $this->resolveArray($parameters, $v, $tagMap) : $this->resolve($parameters, $v, $tagMap);
+            $array[$k] = \is_array($v) ? $this->resolveArray($parameters, $v, $tagMap) : $this->resolve($parameters, $v, $tagMap);
         }
         return $array;
     }
 
     public function resolve(array $parameters, $parameter, array &$tagMap)
     {
-        if (!is_string($parameter) || strlen($parameter) === 0) {
+        if (!\is_string($parameter) || \strlen($parameter) === 0) {
             return $parameter;
         }
 
@@ -35,24 +35,24 @@ public function resolve(array $parameters, $parameter, array &$tagMap)
                 return $parameter;
 
             case Token::SERVICE_CHAR:
-                return "\0" . mb_substr($parameter, 1);
+                return "\0" . \mb_substr($parameter, 1);
 
             case Token::TAG_CHAR:
-                $tagMap[mb_substr($parameter, 1)] = true;
+                $tagMap[\mb_substr($parameter, 1)] = true;
                 return "\0" . $parameter;
         }
 
         $parameter = $this->replaceParameters($parameters, $parameter);
-        if (!is_string($parameter)) {
-            if (is_array($parameter)) {
+        if (!\is_string($parameter)) {
+            if (\is_array($parameter)) {
                 return $this->resolveArray($parameters, $parameter, $tagMap);
             }
             return $parameter;
         }
 
         $parameter = $this->replaceConstants($parameter);
-        if (!is_string($parameter)) {
-            if (is_array($parameter)) {
+        if (!\is_string($parameter)) {
+            if (\is_array($parameter)) {
                 return $this->resolveArray($parameters, $parameter, $tagMap);
             }
             return $parameter;
@@ -63,13 +63,13 @@ public function resolve(array $parameters, $parameter, array &$tagMap)
 
     protected function replaceParameters(array $parameters, string $parameter)
     {
-        if (mb_strpos($parameter, Token::PARAMETER_CHAR) === false) {
+        if (\mb_strpos($parameter, Token::PARAMETER_CHAR) === false) {
             return $parameter;
         }
 
         return $this->replaceSurroundingToken($parameter, Token::PARAMETER_CHAR, function($value) use ($parameters) {
             // We need array_key_exists for if a value has been set as null, but isset is far far faster than array_key_exists
-            if (isset($parameters[$value]) || array_key_exists($value, $parameters)) {
+            if (isset($parameters[$value]) || \array_key_exists($value, $parameters)) {
                 return $parameters[$value];
             }
 
@@ -79,7 +79,7 @@ protected function replaceParameters(array $parameters, string $parameter)
 
     protected function replaceConstants(string $parameter, array &$resolvedConstants = [])
     {
-        if (mb_strpos($parameter, Token::CONSTANT_CHAR) === false) {
+        if (\mb_strpos($parameter, Token::CONSTANT_CHAR) === false) {
             return $parameter;
         }
 
@@ -96,12 +96,11 @@ protected function replaceConstants(string $parameter, array &$resolvedConstants
 
     protected function replaceEnvironment(string $parameter)
     {
-        if (mb_strpos($parameter, Token::ENV_CHAR) === false) {
+        if (\mb_strpos($parameter, Token::ENV_CHAR) === false) {
             return $parameter;
         }
 
         return $this->replaceSurroundingToken($parameter, Token::ENV_CHAR, function($value) {
-
             if (($env = getenv($value)) !== false) {
                 $this->resolvedEnvVars[$value] = $env;
                 return $env;
@@ -111,41 +110,60 @@ protected function replaceEnvironment(string $parameter)
         });
     }
 
-    protected function replaceSurroundingToken(string $parameter, string $token, callable $callable)
+    protected function replaceSurroundingToken(string $parameter, string $token, callable $callable, array $stack = [])
     {
         $oldParameter = $parameter;
 
-        while (is_string($parameter) && mb_substr_count($parameter, $token) > 0) {
-            $pos1 = mb_strpos($parameter, $token);
-            $pos2 = mb_strpos($parameter, $token, $pos1 + 1);
+        while (\is_string($parameter) && \mb_substr_count($parameter, $token) > 0) {
+            // The passStack keeps track of what parameters we've had to resolve on this parameter and passes this
+            // down to ensure that we don't end up trying to do any recursive resolution
+            $passStack = $stack;
+
+            // Find the first entry in the parameter
+            $pos1 = \mb_strpos($parameter, $token);
+            $pos2 = \mb_strpos($parameter, $token, $pos1 + 1);
 
+            // If they are next to each other, then they're escaped tokens and should be ignored
             if ($pos2 === $pos1 + 1) {
-                $parameter = mb_substr($parameter, 0, $pos1) . self::ESCAPED_TOKEN . mb_substr($parameter, $pos2 + 1);
+                $parameter = \mb_substr($parameter, 0, $pos1) . self::ESCAPED_TOKEN . \mb_substr($parameter, $pos2 + 1);
                 continue;
             }
 
+            // If there's only one token, then someone has screwed up the config
             if ($pos2 === false) {
                 throw new ConfigException("An uneven number of '{$token}' token bindings exists for '{$oldParameter}'");
             }
 
-            $value = mb_substr($parameter, $pos1 + 1, $pos2 - ($pos1 + 1));
-            $newValue = $callable($value);
+            // Get the value of the parameter, so with '%foo_bar% salad' this should return 'foo_bar'
+            $value = \mb_substr($parameter, $pos1 + 1, $pos2 - ($pos1 + 1));
 
+            if (in_array($value, $stack, true)) {
+                throw new ConfigException("Infinite parameter loop detected: " . implode(" -> ", $stack));
+            }
+
+            // Add that we've resolved this key to the passStack
+            $passStack[] = $value;
+            $newValue = $callable($value);
             // If it took up the entire parameter
-            if (($pos1 === 0 && ($pos2 + 1) === mb_strlen($parameter))) {
-                $parameter = $newValue;
+            if (($pos1 === 0 && ($pos2 + 1) === \mb_strlen($parameter))) {
+                if (\is_string($newValue)) {
+                    $parameter = $this->replaceSurroundingToken($newValue, $token, $callable, $passStack);
+                } else {
+                    $parameter = $newValue;
+                }
                 continue;
             }
 
-            if (!is_string($newValue) && !is_numeric($newValue)) {
+            if (!\is_string($newValue) && !\is_numeric($newValue)) {
                 throw new ConfigException(
                     "Parameter '{$value}' as part of '{$oldParameter}' resolved to a non-string. This is only permissible if the parameter attempts no interpolation"
                 );
             }
-            $parameter = mb_substr($parameter, 0, $pos1) . $newValue . mb_substr($parameter, $pos2 + 1);
+
+            $parameter = \mb_substr($parameter, 0, $pos1) . $this->replaceSurroundingToken($newValue, $token, $callable, $passStack) . \mb_substr($parameter, $pos2 + 1);
         }
 
-        return is_string($parameter) ? str_replace(self::ESCAPED_TOKEN, $token, $parameter) : $parameter;
+        return \is_string($parameter) ? \str_replace(self::ESCAPED_TOKEN, $token, $parameter) : $parameter;
     }
 
     public function getResolvedConstants() : array

diff --git a/src/TagCollection.php b/src/TagCollection.php
@@ -7,10 +7,10 @@
 
 class TagCollection implements \Iterator
 {
-    private $position = 0;
-    protected $services = [];
-    protected $aliases = [];
-    protected $container;
+    private int $position = 0;
+    protected array $services = [];
+    protected array $aliases = [];
+    protected Container $container;
 
     public function __construct(Container $container, array $services)
     {

diff --git a/tests/bootstrap.php b/tests/bootstrap.php
diff --git a/tests/integration/Basic/BasicTest.php b/tests/integration/Basic/BasicTest.php
@@ -29,6 +29,18 @@ public function testBasic()
         self::assertSame("potatosalad", $arguments[3]);
     }
 
+    public function testArray()
+    {
+        $container = Syringe::build([
+            "paths" => [__DIR__],
+            "files" => ["file1.yml"]
+        ]);
+
+        self::assertSame(["fridge", "magnet"], $container["my_parameter_9"]);
+
+        self::assertSame(["potato", ["salad"]], $container["my_parameter_10"]);
+    }
+
     public function testBoolAndInt()
     {
         $container = Syringe::build([

diff --git a/tests/integration/Basic/file1.yml b/tests/integration/Basic/file1.yml
@@ -22,4 +22,12 @@ parameters:
   my_parameter_6: 5003
 
   my_parameter_7: "potato"
-  my_parameter_8: "salad"
+  my_parameter_8: "salad"
+
+  my_parameter_9:
+    - "fridge"
+    - "magnet"
+
+  my_parameter_10:
+    - "%my_parameter_7%"
+    - ["%my_parameter_8%"]