Skip to content

fix: ConfigFactory and ProviderConfig config merging #298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
albertcht merged 7 commits into from
Dec 21, 2025
Merged

fix: ConfigFactory and ProviderConfig config merging #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
34b0eb4
fix: Use Arr::merge for config merging to handle both lists and scalars
binaryfire Dec 19, 2025
ce024ab
Switch to custom merge method
binaryfire Dec 20, 2025
de3bd4b
Add dedicated merge test
binaryfire Dec 20, 2025
5843766
Update docblocks
binaryfire Dec 21, 2025
7ad6bc7
Merge remote-tracking branch 'origin/main' into fix/provider-config-m…
binaryfire Dec 21, 2025
c71cc77
test: add PriorityDefinition merge coverage
binaryfire Dec 21, 2025
4b8c7e4
refactor: extract shared mergeTwo() to eliminate duplication
binaryfire Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/config/src/ConfigFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,14 @@ public function __invoke(ContainerInterface $container)

$rootConfig = $this->readConfig($configPath . '/hyperf.php');
$autoloadConfig = $this->readPaths($loadPaths, ['hyperf.php']);
$merged = array_replace_recursive(ProviderConfig::load(), $rootConfig, ...$autoloadConfig);

// Merge all config sources: provider configs + root config + autoload configs
$allConfigs = [ProviderConfig::load(), $rootConfig, ...$autoloadConfig];
$merged = array_reduce(
array_slice($allConfigs, 1),
ProviderConfig::mergeTwo(...),
$allConfigs[0]
);

return new Repository($merged);
}
Expand Down
81 changes: 81 additions & 0 deletions src/config/src/ProviderConfig.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use Hyperf\Collection\Arr;
use Hyperf\Config\ProviderConfig as HyperfProviderConfig;
use Hyperf\Di\Definition\PriorityDefinition;
use Hyperf\Support\Composer;
use Hypervel\Support\ServiceProvider;
use Throwable;
Expand Down Expand Up @@ -85,4 +86,84 @@ protected static function packagesToIgnore(): array

return array_merge($packages, $project);
}

/**
* Merge provider config arrays.
*
* Correctly handles:
* - Pure lists (numeric keys): appends values with deduplication
* - Associative arrays (string keys): recursively merges, later wins for scalars
* - Mixed arrays (e.g. listeners with priorities): appends numeric, merges string keys
*
* @return array<string, mixed>
*/
protected static function merge(...$arrays): array
{
if (empty($arrays)) {
return [];
}

$result = array_reduce(
array_slice($arrays, 1),
[static::class, 'mergeTwo'],
$arrays[0]
);

// Special handling for dependencies with PriorityDefinition
if (isset($result['dependencies'])) {
$result['dependencies'] = [];
foreach ($arrays as $item) {
foreach ($item['dependencies'] ?? [] as $key => $value) {
$depend = $result['dependencies'][$key] ?? null;
if (! $depend instanceof PriorityDefinition) {
$result['dependencies'][$key] = $value;
continue;
}

if ($value instanceof PriorityDefinition) {
$depend->merge($value);
}
}
}
}
Comment on lines +113 to +128
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The special handling for dependencies with PriorityDefinition has a logic issue. When iterating through all arrays and processing dependencies, the code resets $result['dependencies'] to an empty array at line 114, which discards any dependencies that were already merged by the mergeTwo function. Then it iterates through all arrays again, but it checks if $depend is an instance of PriorityDefinition when $depend comes from the now-empty $result['dependencies'], which means it will never be a PriorityDefinition on the first iteration.

The logic should either:

  1. Process dependencies after the initial merge is complete (not reset the array), or
  2. Build dependencies separately and then add them to the result

Consider refactoring this to preserve the merged dependencies and only apply special PriorityDefinition merging logic when needed.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@binaryfire binaryfire Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albertcht This is a false positive. The "reset to empty and re-iterate" pattern is intentional and is an exact copy from https://github.com/hyperf/hyperf/blob/master/src/config/src/ProviderConfig.php.

Why it works this way:

The initial merge (mergeTwo in this PR, array_merge_recursive in Hyperf's original) doesn't understand PriorityDefinition objects—it would just keep the last value for each key. The post-merge step rebuilds dependencies from scratch with special handling:

  1. First time a key is seen: $depend = null, so ! null instanceof PriorityDefinition → true → store the value
  2. Subsequent encounters: If the stored value is not a PriorityDefinition, override it (last one wins). If it is a PriorityDefinition and the new value is also one, call $depend->merge($value) to combine priorities.

This correctly handles the DI priority system where multiple providers can register competing implementations for the same interface, with the highest priority winning at resolution time.


return $result;
}

/**
* Merge two config arrays.
*
* Correctly handles:
* - Pure lists (numeric keys): appends values with deduplication
* - Associative arrays (string keys): recursively merges, later wins for scalars
* - Mixed arrays (e.g. listeners with priorities): appends numeric, merges string keys
*
* This method is public so ConfigFactory can use the same merge semantics.
*
* @return array<string, mixed>
*/
public static function mergeTwo(array $base, array $override): array
{
$result = $base;

foreach ($override as $key => $value) {
if (is_int($key)) {
// Numeric key - append if not already present (deduplicate)
if (! in_array($value, $result, true)) {
$result[] = $value;
}
} elseif (! array_key_exists($key, $result)) {
// New string key - just add it
$result[$key] = $value;
} elseif (is_array($value) && is_array($result[$key])) {
// Both are arrays - recursively merge
$result[$key] = self::mergeTwo($result[$key], $value);
} else {
// Scalar or mixed types - override wins
$result[$key] = $value;
}
}

return $result;
}
}
192 changes: 192 additions & 0 deletions tests/Config/ConfigFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
<?php

declare(strict_types=1);

namespace Hypervel\Tests\Config;

use Hypervel\Config\ProviderConfig;
use PHPUnit\Framework\TestCase;
use ReflectionMethod;

/**
* Tests for ConfigFactory merge behavior.
*
* ConfigFactory merges: ProviderConfig::load() + $rootConfig + ...$autoloadConfig
* This test verifies the merge logic handles both scalar replacement and list
* combining correctly.
*
* @internal
* @coversNothing
*/
class ConfigFactoryTest extends TestCase
{
/**
* Test that the merge strategy used by ConfigFactory correctly handles
* numeric arrays (lists) by combining them, not replacing at indices.
*
* This is a regression test for the issue where array_replace_recursive
* was replacing values at matching indices instead of combining lists.
*
* Scenario: Provider configs define commands, app config adds more commands.
* Expected: All commands should be present in the merged result.
*/
public function testMergePreservesListsFromProviderConfigs(): void
{
// Simulates ProviderConfig::load() result
$providerConfig = [
'commands' => [
'App\Commands\CommandA',
'App\Commands\CommandB',
'App\Commands\CommandC',
],
'listeners' => [
'App\Listeners\ListenerA',
'App\Listeners\ListenerB',
],
];

// Simulates app config (e.g., config/commands.php adding custom commands)
$appConfig = [
'commands' => [
'App\Commands\CustomCommand',
],
];

// This simulates what ConfigFactory currently does
$result = $this->mergeConfigs($providerConfig, $appConfig);

// All provider commands should still be present
$this->assertContains(
'App\Commands\CommandA',
$result['commands'],
'CommandA from provider config should be preserved'
);
$this->assertContains(
'App\Commands\CommandB',
$result['commands'],
'CommandB from provider config should be preserved'
);
$this->assertContains(
'App\Commands\CommandC',
$result['commands'],
'CommandC from provider config should be preserved'
);

// App's custom command should be added
$this->assertContains(
'App\Commands\CustomCommand',
$result['commands'],
'CustomCommand from app config should be added'
);

// Should have 4 commands total (3 from provider + 1 from app)
$this->assertCount(4, $result['commands'], 'Should have all 4 commands');
}

/**
* Test that the merge strategy correctly replaces scalar values in
* associative arrays (app config overrides provider config).
*/
public function testMergeReplacesScalarsInAssociativeArrays(): void
{
$providerConfig = [
'database' => [
'default' => 'sqlite',
'connections' => [
'pgsql' => [
'driver' => 'pgsql',
'host' => 'localhost',
'port' => 5432,
],
],
],
];

$appConfig = [
'database' => [
'default' => 'pgsql',
'connections' => [
'pgsql' => [
'host' => 'production-db.example.com',
],
],
],
];

$result = $this->mergeConfigs($providerConfig, $appConfig);

// App's default should override provider's default
$this->assertSame('pgsql', $result['database']['default']);

// App's host should override provider's host
$this->assertSame(
'production-db.example.com',
$result['database']['connections']['pgsql']['host']
);

// Driver should remain a string (not become an array)
$this->assertIsString(
$result['database']['connections']['pgsql']['driver'],
'Driver should remain a string, not become an array'
);
$this->assertSame('pgsql', $result['database']['connections']['pgsql']['driver']);

// Provider's port should be preserved
$this->assertSame(5432, $result['database']['connections']['pgsql']['port']);
}

/**
* Test merging multiple config arrays (simulating provider + root + autoload configs).
*/
public function testMergeMultipleConfigArrays(): void
{
$providerConfig = [
'commands' => ['CommandA', 'CommandB'],
'app' => ['name' => 'Provider Default'],
];

$rootConfig = [
'app' => ['debug' => true],
];

$autoloadConfig1 = [
'commands' => ['CommandC'],
'app' => ['name' => 'My App'],
];

$autoloadConfig2 = [
'database' => ['default' => 'mysql'],
];

// Merge all configs (simulating ConfigFactory behavior)
$result = $this->mergeConfigs($providerConfig, $rootConfig, $autoloadConfig1, $autoloadConfig2);

// All commands should be combined
$this->assertContains('CommandA', $result['commands']);
$this->assertContains('CommandB', $result['commands']);
$this->assertContains('CommandC', $result['commands']);

// Later app.name should win
$this->assertSame('My App', $result['app']['name']);

// app.debug should be merged in
$this->assertTrue($result['app']['debug']);

// database from autoloadConfig2 should be present
$this->assertSame('mysql', $result['database']['default']);
}

/**
* Simulate ConfigFactory's merge behavior using ProviderConfig::merge().
*
* ConfigFactory uses ProviderConfig::mergeTwo() directly for merging.
* We test via ProviderConfig::merge() which uses the same mergeTwo() method
* internally, ensuring identical merge semantics.
*/
private function mergeConfigs(array ...$configs): array
{
$method = new ReflectionMethod(ProviderConfig::class, 'merge');

return $method->invoke(null, ...$configs);
}
}
Loading