Skip to content

Commit 2dabd18

Browse files
albertchtalbertcht
authored
Merge pull request #298 from binaryfire/fix/provider-config-merge-scalar-values
fix: `ConfigFactory` and `ProviderConfig` config merging
2 parents 5e37d04 + 4b8c7e4 commit 2dabd18

20 files changed

+1809
-1
lines changed

‎src/config/src/ConfigFactory.php‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,14 @@ public function __invoke(ContainerInterface $container)
2121

2222
$rootConfig = $this->readConfig($configPath . '/hyperf.php');
2323
$autoloadConfig = $this->readPaths($loadPaths, ['hyperf.php']);
24-
$merged = array_replace_recursive(ProviderConfig::load(), $rootConfig, ...$autoloadConfig);
24+
25+
// Merge all config sources: provider configs + root config + autoload configs
26+
$allConfigs = [ProviderConfig::load(), $rootConfig, ...$autoloadConfig];
27+
$merged = array_reduce(
28+
array_slice($allConfigs, 1),
29+
ProviderConfig::mergeTwo(...),
30+
$allConfigs[0]
31+
);
2532

2633
return new Repository($merged);
2734
}

‎src/config/src/ProviderConfig.php‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
use Hyperf\Collection\Arr;
88
use Hyperf\Config\ProviderConfig as HyperfProviderConfig;
9+
use Hyperf\Di\Definition\PriorityDefinition;
910
use Hyperf\Support\Composer;
1011
use Hypervel\Support\ServiceProvider;
1112
use Throwable;
@@ -85,4 +86,84 @@ protected static function packagesToIgnore(): array
8586

8687
return array_merge($packages, $project);
8788
}
89+
90+
/**
91+
* Merge provider config arrays.
92+
*
93+
* Correctly handles:
94+
* - Pure lists (numeric keys): appends values with deduplication
95+
* - Associative arrays (string keys): recursively merges, later wins for scalars
96+
* - Mixed arrays (e.g. listeners with priorities): appends numeric, merges string keys
97+
*
98+
* @return array<string, mixed>
99+
*/
100+
protected static function merge(...$arrays): array
101+
{
102+
if (empty($arrays)) {
103+
return [];
104+
}
105+
106+
$result = array_reduce(
107+
array_slice($arrays, 1),
108+
[static::class, 'mergeTwo'],
109+
$arrays[0]
110+
);
111+
112+
// Special handling for dependencies with PriorityDefinition
113+
if (isset($result['dependencies'])) {
114+
$result['dependencies'] = [];
115+
foreach ($arrays as $item) {
116+
foreach ($item['dependencies'] ?? [] as $key => $value) {
117+
$depend = $result['dependencies'][$key] ?? null;
118+
if (! $depend instanceof PriorityDefinition) {
119+
$result['dependencies'][$key] = $value;
120+
continue;
121+
}
122+
123+
if ($value instanceof PriorityDefinition) {
124+
$depend->merge($value);
125+
}
126+
}
127+
}
128+
}
129+
130+
return $result;
131+
}
132+
133+
/**
134+
* Merge two config arrays.
135+
*
136+
* Correctly handles:
137+
* - Pure lists (numeric keys): appends values with deduplication
138+
* - Associative arrays (string keys): recursively merges, later wins for scalars
139+
* - Mixed arrays (e.g. listeners with priorities): appends numeric, merges string keys
140+
*
141+
* This method is public so ConfigFactory can use the same merge semantics.
142+
*
143+
* @return array<string, mixed>
144+
*/
145+
public static function mergeTwo(array $base, array $override): array
146+
{
147+
$result = $base;
148+
149+
foreach ($override as $key => $value) {
150+
if (is_int($key)) {
151+
// Numeric key - append if not already present (deduplicate)
152+
if (! in_array($value, $result, true)) {
153+
$result[] = $value;
154+
}
155+
} elseif (! array_key_exists($key, $result)) {
156+
// New string key - just add it
157+
$result[$key] = $value;
158+
} elseif (is_array($value) && is_array($result[$key])) {
159+
// Both are arrays - recursively merge
160+
$result[$key] = self::mergeTwo($result[$key], $value);
161+
} else {
162+
// Scalar or mixed types - override wins
163+
$result[$key] = $value;
164+
}
165+
}
166+
167+
return $result;
168+
}
88169
}

‎tests/Config/ConfigFactoryTest.php‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hypervel\Tests\Config;
6+
7+
use Hypervel\Config\ProviderConfig;
8+
use PHPUnit\Framework\TestCase;
9+
use ReflectionMethod;
10+
11+
/**
12+
* Tests for ConfigFactory merge behavior.
13+
*
14+
* ConfigFactory merges: ProviderConfig::load() + $rootConfig + ...$autoloadConfig
15+
* This test verifies the merge logic handles both scalar replacement and list
16+
* combining correctly.
17+
*
18+
* @internal
19+
* @coversNothing
20+
*/
21+
class ConfigFactoryTest extends TestCase
22+
{
23+
/**
24+
* Test that the merge strategy used by ConfigFactory correctly handles
25+
* numeric arrays (lists) by combining them, not replacing at indices.
26+
*
27+
* This is a regression test for the issue where array_replace_recursive
28+
* was replacing values at matching indices instead of combining lists.
29+
*
30+
* Scenario: Provider configs define commands, app config adds more commands.
31+
* Expected: All commands should be present in the merged result.
32+
*/
33+
public function testMergePreservesListsFromProviderConfigs(): void
34+
{
35+
// Simulates ProviderConfig::load() result
36+
$providerConfig = [
37+
'commands' => [
38+
'App\Commands\CommandA',
39+
'App\Commands\CommandB',
40+
'App\Commands\CommandC',
41+
],
42+
'listeners' => [
43+
'App\Listeners\ListenerA',
44+
'App\Listeners\ListenerB',
45+
],
46+
];
47+
48+
// Simulates app config (e.g., config/commands.php adding custom commands)
49+
$appConfig = [
50+
'commands' => [
51+
'App\Commands\CustomCommand',
52+
],
53+
];
54+
55+
// This simulates what ConfigFactory currently does
56+
$result = $this->mergeConfigs($providerConfig, $appConfig);
57+
58+
// All provider commands should still be present
59+
$this->assertContains(
60+
'App\Commands\CommandA',
61+
$result['commands'],
62+
'CommandA from provider config should be preserved'
63+
);
64+
$this->assertContains(
65+
'App\Commands\CommandB',
66+
$result['commands'],
67+
'CommandB from provider config should be preserved'
68+
);
69+
$this->assertContains(
70+
'App\Commands\CommandC',
71+
$result['commands'],
72+
'CommandC from provider config should be preserved'
73+
);
74+
75+
// App's custom command should be added
76+
$this->assertContains(
77+
'App\Commands\CustomCommand',
78+
$result['commands'],
79+
'CustomCommand from app config should be added'
80+
);
81+
82+
// Should have 4 commands total (3 from provider + 1 from app)
83+
$this->assertCount(4, $result['commands'], 'Should have all 4 commands');
84+
}
85+
86+
/**
87+
* Test that the merge strategy correctly replaces scalar values in
88+
* associative arrays (app config overrides provider config).
89+
*/
90+
public function testMergeReplacesScalarsInAssociativeArrays(): void
91+
{
92+
$providerConfig = [
93+
'database' => [
94+
'default' => 'sqlite',
95+
'connections' => [
96+
'pgsql' => [
97+
'driver' => 'pgsql',
98+
'host' => 'localhost',
99+
'port' => 5432,
100+
],
101+
],
102+
],
103+
];
104+
105+
$appConfig = [
106+
'database' => [
107+
'default' => 'pgsql',
108+
'connections' => [
109+
'pgsql' => [
110+
'host' => 'production-db.example.com',
111+
],
112+
],
113+
],
114+
];
115+
116+
$result = $this->mergeConfigs($providerConfig, $appConfig);
117+
118+
// App's default should override provider's default
119+
$this->assertSame('pgsql', $result['database']['default']);
120+
121+
// App's host should override provider's host
122+
$this->assertSame(
123+
'production-db.example.com',
124+
$result['database']['connections']['pgsql']['host']
125+
);
126+
127+
// Driver should remain a string (not become an array)
128+
$this->assertIsString(
129+
$result['database']['connections']['pgsql']['driver'],
130+
'Driver should remain a string, not become an array'
131+
);
132+
$this->assertSame('pgsql', $result['database']['connections']['pgsql']['driver']);
133+
134+
// Provider's port should be preserved
135+
$this->assertSame(5432, $result['database']['connections']['pgsql']['port']);
136+
}
137+
138+
/**
139+
* Test merging multiple config arrays (simulating provider + root + autoload configs).
140+
*/
141+
public function testMergeMultipleConfigArrays(): void
142+
{
143+
$providerConfig = [
144+
'commands' => ['CommandA', 'CommandB'],
145+
'app' => ['name' => 'Provider Default'],
146+
];
147+
148+
$rootConfig = [
149+
'app' => ['debug' => true],
150+
];
151+
152+
$autoloadConfig1 = [
153+
'commands' => ['CommandC'],
154+
'app' => ['name' => 'My App'],
155+
];
156+
157+
$autoloadConfig2 = [
158+
'database' => ['default' => 'mysql'],
159+
];
160+
161+
// Merge all configs (simulating ConfigFactory behavior)
162+
$result = $this->mergeConfigs($providerConfig, $rootConfig, $autoloadConfig1, $autoloadConfig2);
163+
164+
// All commands should be combined
165+
$this->assertContains('CommandA', $result['commands']);
166+
$this->assertContains('CommandB', $result['commands']);
167+
$this->assertContains('CommandC', $result['commands']);
168+
169+
// Later app.name should win
170+
$this->assertSame('My App', $result['app']['name']);
171+
172+
// app.debug should be merged in
173+
$this->assertTrue($result['app']['debug']);
174+
175+
// database from autoloadConfig2 should be present
176+
$this->assertSame('mysql', $result['database']['default']);
177+
}
178+
179+
/**
180+
* Simulate ConfigFactory's merge behavior using ProviderConfig::merge().
181+
*
182+
* ConfigFactory uses ProviderConfig::mergeTwo() directly for merging.
183+
* We test via ProviderConfig::merge() which uses the same mergeTwo() method
184+
* internally, ensuring identical merge semantics.
185+
*/
186+
private function mergeConfigs(array ...$configs): array
187+
{
188+
$method = new ReflectionMethod(ProviderConfig::class, 'merge');
189+
190+
return $method->invoke(null, ...$configs);
191+
}
192+
}

0 commit comments

Comments
 (0)