Fix config merger (#163)

Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
yiisoft · Dec 25, 2023 · f6c3d85 · f6c3d85
1 parent 8ef9884
commit f6c3d85
Show file tree

Hide file tree

Showing 25 changed files with 342 additions and 43 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,8 @@
 
 - New #155: Add ability to specify recursion depth for recursive modifier (@vjik)
 - Enh #157: Remove unnecessary code in `PackagesListBuilder` (@vjik)
+- Bug #153: Do not throw "Duplicate key…" exception when using nested groups (@vjik)
+- Bug #163: References to another configs use reverse and recursive modifiers of root group now (@vjik)
 
 ## 1.4.0 November 17, 2023
 

diff --git a/README.md b/README.md
@@ -471,6 +471,8 @@ If you want to recursively merge arrays to a certain depth, use the `RecursiveMe
 RecursiveMerge::groups(['widgets-themes', 'my-custom-group'], 1)
 ```
 
+> Note: References to another configs use recursive modifier of root group.
+
 ### Reverse merge of arrays
 
 Result of reverse merge is being ordered descending by data source. It is useful for merging module config with
@@ -494,6 +496,8 @@ $config = new Config(
 $events = $config->get('events-console'); // merged reversed
 ```
 
+> Note: References to another configs use reverse modifier of root group.
+
 ### Remove elements from vendor package configuration
 
 Sometimes it is necessary to remove some elements of vendor packages configuration. To do this,

diff --git a/src/Config.php b/src/Config.php
@@ -70,7 +70,9 @@ public function get(string $group): array
         }
 
         $this->runBuildParams();
-        $this->runBuildGroup($group);
+
+        $this->merger->reset();
+        $this->build[$group] = $this->buildGroup($group);
 
         return $this->build[$group];
     }
@@ -85,51 +87,36 @@ public function has(string $group): bool
      */
     private function runBuildParams(): void
     {
-        if ($this->paramsGroup !== null) {
+        if ($this->paramsGroup !== null && !isset($this->build[$this->paramsGroup])) {
             $this->isBuildingParams = true;
-            $this->runBuildGroup($this->paramsGroup);
+            $this->build[$this->paramsGroup] = $this->buildGroup($this->paramsGroup);
             $this->isBuildingParams = false;
         }
     }
 
-    /**
-     * @throws ErrorException If an error occurred during the build.
-     */
-    private function runBuildGroup(string $group): void
-    {
-        $this->merger->reset();
-        $this->buildGroup($group);
-    }
-
     /**
      * Builds the configuration of the group.
      *
      * @param string $group The group name.
      *
      * @throws ErrorException If an error occurred during the build.
      */
-    private function buildGroup(string $group): void
+    private function buildGroup(string $group, array $result = [], ?string $originalGroup = null): array
     {
-        if (isset($this->build[$group])) {
-            return;
-        }
-
-        $this->build[$group] = [];
-
         foreach ($this->filesExtractor->extract($group) as $file => $context) {
-            if (Options::isVariable($file)) {
+            if ($context->isVariable()) {
                 $variable = $this->prepareVariable($file, $group);
-                $array = $this->get($variable);
+                $result = $this->buildGroup($variable, $result, $originalGroup ?? $group);
             } else {
-                $array = $this->buildFile($file);
+                $result = $this->merger->merge(
+                    $context->setOriginalGroup($originalGroup ?? $group),
+                    $result,
+                    $this->buildFile($file),
+                );
             }
-
-            $this->build[$group] = $this->merger->merge(
-                $context,
-                $this->build[$group],
-                $array,
-            );
         }
+
+        return $result;
     }
 
     /**

diff --git a/src/Context.php b/src/Context.php
@@ -14,6 +14,8 @@ final class Context
     public const APPLICATION = 3;
     public const ENVIRONMENT = 4;
 
+    private string $originalGroup = '';
+
     public function __construct(
         private string $group,
         private string $package,
@@ -23,6 +25,17 @@ public function __construct(
     ) {
     }
 
+    public function setOriginalGroup(string $group): self
+    {
+        $this->originalGroup = $group;
+        return $this;
+    }
+
+    public function originalGroup(): string
+    {
+        return $this->originalGroup;
+    }
+
     public function group(): string
     {
         return $this->group;

diff --git a/src/Merger.php b/src/Merger.php
@@ -55,8 +55,8 @@ public function reset(): void
      */
     public function merge(Context $context, array $arrayA, array $arrayB): array
     {
-        $recursionDepth = $this->dataModifiers->getRecursionDepth($context->group());
-        $isReverseMerge = $this->dataModifiers->isReverseMergeGroup($context->group());
+        $recursionDepth = $this->dataModifiers->getRecursionDepth($context->originalGroup());
+        $isReverseMerge = $this->dataModifiers->isReverseMergeGroup($context->originalGroup());
 
         if ($isReverseMerge) {
             $arrayB = $this->prepareArrayForReverse($context, [], $arrayB, $recursionDepth !== false);
@@ -143,7 +143,7 @@ private function performMerge(
                 );
 
                 if ($file !== null) {
-                    $this->throwDuplicateKeyErrorException($context->group(), $fullKeyPath, [$file, $context->file()]);
+                    $this->throwDuplicateKeyErrorException($context->originalGroup(), $fullKeyPath, [$file, $context->file()]);
                 }
             }
 
@@ -197,11 +197,6 @@ private function prepareArrayForReverse(
                 continue;
             }
 
-            if ($context->isVariable()) {
-                $result[$key] = $value;
-                continue;
-            }
-
             $recursiveKeyPath[] = $key;
 
             /** @var string|null $file */
@@ -211,7 +206,7 @@ private function prepareArrayForReverse(
             );
 
             if ($file !== null) {
-                $this->throwDuplicateKeyErrorException($context->group(), $recursiveKeyPath, [$file, $context->file()]);
+                $this->throwDuplicateKeyErrorException($context->originalGroup(), $recursiveKeyPath, [$file, $context->file()]);
             }
 
             $result[$key] = $value;

diff --git a/tests/Integration/Dummy/DummyTest.php b/tests/Integration/Dummy/DummyTest.php
@@ -369,16 +369,16 @@ public function testConfigWithReverseMerge(): void
                 'b-web-environment-override-key' => 'c-web-override-value',
                 'c-web-key' => 'c-web-value',
                 'c-web-environment-override-key' => 'c-web-override-value',
-                'root-common-nested-key-2' => 'root-common-nested-value-2',
-                'root-common-nested-key-1' => 'root-common-nested-value-1',
+                'a-common-key' => 'a-common-value',
                 'a-common-root-override-key' => 'common-root-override-value',
+                'b-common-key' => 'b-common-value',
                 'b-common-root-override-key' => 'common-root-override-value',
+                'c-common-key' => 'c-common-value',
                 'c-common-root-override-key' => 'common-root-override-value',
-                'root-common-key-2' => 'root-common-value-2',
                 'root-common-key-1' => 'root-common-value-1',
-                'c-common-key' => 'c-common-value',
-                'b-common-key' => 'b-common-value',
-                'a-common-key' => 'a-common-value',
+                'root-common-key-2' => 'root-common-value-2',
+                'root-common-nested-key-1' => 'root-common-nested-value-1',
+                'root-common-nested-key-2' => 'root-common-nested-value-2',
                 'root-web-key' => 'root-web-value',
             ],
             $config->get('web')

diff --git a/tests/Integration/NestedGroupBug153/NestedGroupBug153Test.php b/tests/Integration/NestedGroupBug153/NestedGroupBug153Test.php
@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Yiisoft\Config\Tests\Integration\NestedGroupBug153;
+
+use Yiisoft\Config\Tests\Integration\IntegrationTestCase;
+
+final class NestedGroupBug153Test extends IntegrationTestCase
+{
+    public function testBase(): void
+    {
+        $config = $this->runComposerUpdateAndCreateConfig(
+            rootPath: __DIR__,
+            packages: [
+                'test/a' => __DIR__ . '/packages/a',
+            ],
+            extra: [
+                'config-plugin' => [
+                    'params' => [],
+                    'di' => 'di.php',
+                    'di-web' => '$di',
+                ],
+            ],
+        );
+
+        $this->assertSame(['key' => 'app-di'], $config->get('di-web'));
+    }
+}
diff --git a/tests/Integration/NestedGroupBug153/di.php b/tests/Integration/NestedGroupBug153/di.php
@@ -0,0 +1,7 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'key' => 'app-di',
+];
diff --git a/tests/Integration/NestedGroupBug153/packages/a/composer.json b/tests/Integration/NestedGroupBug153/packages/a/composer.json
@@ -0,0 +1,9 @@
+{
+    "name": "test/a",
+    "version": "1.0.0",
+    "extra": {
+        "config-plugin": {
+            "di-web": "di-web.php"
+        }
+    }
+}
diff --git a/tests/Integration/NestedGroupBug153/packages/a/di-web.php b/tests/Integration/NestedGroupBug153/packages/a/di-web.php
@@ -0,0 +1,7 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'key' => 'package-di-web',
+];
diff --git a/tests/Integration/NestedGroupDuplicateKey/NestedGroupDuplicateKeyTest.php b/tests/Integration/NestedGroupDuplicateKey/NestedGroupDuplicateKeyTest.php
@@ -0,0 +1,39 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Yiisoft\Config\Tests\Integration\NestedGroupDuplicateKey;
+
+use ErrorException;
+use Yiisoft\Config\Tests\Integration\IntegrationTestCase;
+
+final class NestedGroupDuplicateKeyTest extends IntegrationTestCase
+{
+    public function testBase(): void
+    {
+        $config = $this->runComposerUpdateAndCreateConfig(
+            rootPath: __DIR__,
+            packages: [
+                'test/a' => __DIR__ . '/packages/a',
+            ],
+            extra: [
+                'config-plugin' => [
+                    'params' => [],
+                    'di' => 'di.php',
+                    'di-web' => [
+                        '$di',
+                        'di-web.php',
+                    ],
+                ],
+            ],
+        );
+
+        $this->expectException(ErrorException::class);
+        $this->expectExceptionMessage(
+            'Duplicate key "key" in the following configs while building "di-web" group:' . "\n" .
+            ' - di-web.php' . "\n" .
+            ' - di.php'
+        );
+        $config->get('di-web');
+    }
+}
diff --git a/tests/Integration/NestedGroupDuplicateKey/di-web.php b/tests/Integration/NestedGroupDuplicateKey/di-web.php
@@ -0,0 +1,7 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'key' => 'app-di-web',
+];
diff --git a/tests/Integration/NestedGroupDuplicateKey/di.php b/tests/Integration/NestedGroupDuplicateKey/di.php
@@ -0,0 +1,7 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'key' => 'app-di',
+];
diff --git a/tests/Integration/NestedGroupDuplicateKey/packages/a/composer.json b/tests/Integration/NestedGroupDuplicateKey/packages/a/composer.json
@@ -0,0 +1,9 @@
+{
+    "name": "test/a",
+    "version": "1.0.0",
+    "extra": {
+        "config-plugin": {
+            "di": "di.php"
+        }
+    }
+}
diff --git a/tests/Integration/NestedGroupDuplicateKey/packages/a/di.php b/tests/Integration/NestedGroupDuplicateKey/packages/a/di.php
@@ -0,0 +1,7 @@
+<?php
+
+declare(strict_types=1);
+
+return [
+    'key' => 'package-di',
+];
diff --git a/tests/Integration/RemoveFromVendorExtended/RemoveFromVendorExtendedTest.php b/tests/Integration/RemoveFromVendorExtended/RemoveFromVendorExtendedTest.php
@@ -0,0 +1,56 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Integration\RemoveFromVendorExtended;
+
+use Yiisoft\Config\Modifier\RecursiveMerge;
+use Yiisoft\Config\Modifier\RemoveFromVendor;
+use Yiisoft\Config\Tests\Integration\IntegrationTestCase;
+
+final class RemoveFromVendorExtendedTest extends IntegrationTestCase
+{
+    public function testBase(): void
+    {
+        $config = $this->runComposerUpdateAndCreateConfig(
+            rootPath: __DIR__,
+            packages: [
+                'yiisoft/auth-jwt' => __DIR__ . '/packages/yiisoft-auth-jwt',
+            ],
+            extra: [
+                'config-plugin' => [
+                    'params' => 'params.php',
+                    'params-web' => [
+                        '$params',
+                        'params-web.php',
+                    ],
+                ],
+                'config-plugin-environments' => [
+                    'dev' => [
+                        'params' => 'params-dev.php',
+                    ],
+                ],
+            ],
+            environment: 'dev',
+            paramsGroup: 'params-web',
+            modifiers: [
+                RecursiveMerge::groups('params', 'params-web'),
+                RemoveFromVendor::keys(['yiisoft/auth-jwt', 'algorithms'])->package('yiisoft/auth-jwt', 'params'),
+            ],
+        );
+
+        $this->assertSame(
+            [
+                'yiisoft/auth-jwt' => [
+                    'algorithms' => [
+                        'RS384',
+                        'HS512',
+                    ],
+                ],
+                'debug' => true,
+                'test' => 7,
+            ],
+            $config->get('params-web'),
+        );
+    }
+}
diff --git a/tests/Integration/RemoveFromVendorExtended/packages/yiisoft-auth-jwt/composer.json b/tests/Integration/RemoveFromVendorExtended/packages/yiisoft-auth-jwt/composer.json
@@ -0,0 +1,9 @@
+{
+    "name": "yiisoft/auth-jwt",
+    "version": "1.0.0",
+    "extra": {
+        "config-plugin": {
+            "params": "params.php"
+        }
+    }
+}