-
-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
fix: ConfigFactory and ProviderConfig config merging
#298
Conversation
Replace array_replace_recursive with Arr::merge in ConfigFactory, and override ProviderConfig::merge() to use Arr::merge instead of the parent's array_merge_recursive. Arr::merge correctly: - Combines list arrays (commands, listeners) by appending with deduplication - Replaces scalar values in associative arrays (later value wins) - Recursively merges nested associative arrays This fixes two bugs: 1. array_merge_recursive (Hyperf's default) converts duplicate scalar keys into arrays: ['driver' => 'pgsql'] + ['driver' => 'pgsql'] became ['driver' => ['pgsql', 'pgsql']] 2. array_replace_recursive (previous fix) replaced list items at matching indices instead of combining them: commands from providers were being overwritten instead of accumulated Includes comprehensive test coverage for both ProviderConfig and ConfigFactory merge behavior.
Arr::merge in ConfigFactory and ProviderConfig for correct config mergingArr::merge in ConfigFactory and ProviderConfig for correct config merging
Arr::merge in ConfigFactory and ProviderConfig for correct config mergingConfigFactory and ProviderConfig config merging
|
@albertcht I've also added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes config merging bugs in ConfigFactory and ProviderConfig by implementing custom merge logic that correctly handles all Hyperf config patterns. The previous implementation had issues with scalar value duplication, list array replacement, and loss of string keys in mixed arrays.
Key Changes:
- Implemented custom
mergeTwo()method in bothProviderConfigandConfigFactorythat handles numeric keys (append with deduplication) and string keys (recursive merge with override) correctly - Fixed the critical bug where priority listeners with string keys were being lost during merge
- Added comprehensive test coverage with 16 unit tests in
ProviderConfigTest, 3 tests inConfigFactoryTest, and 5 integration tests inMergeIntegrationTest
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config/src/ProviderConfig.php | Added custom merge() and mergeTwo() methods to replace Arr::merge, includes special handling for PriorityDefinition |
| src/config/src/ConfigFactory.php | Replaced array_replace_recursive with custom mergeTwo() method for correct list and scalar merging |
| tests/Config/ProviderConfigTest.php | Comprehensive unit tests (16 tests) covering all merge edge cases including priority listeners, scalar preservation, and deduplication |
| tests/Config/ConfigFactoryTest.php | Tests verifying ConfigFactory uses the same merge semantics as ProviderConfig (3 tests) |
| tests/Config/MergeIntegrationTest.php | Integration tests using real config fixtures to verify end-to-end merging behavior (5 tests) |
| tests/Config/fixtures/base/*.php | Base configuration fixtures representing initial config state |
| tests/Config/fixtures/override/*.php | Override configuration fixtures representing configs that merge with base |
| tests/Config/fixtures/expected/*.php | Expected results after merging base and override configs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
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:
- Process dependencies after the initial merge is complete (not reset the array), or
- 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.
There was a problem hiding this comment.
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:
- First time a key is seen:
$depend = null, so! null instanceof PriorityDefinition→ true → store the value - Subsequent encounters: If the stored value is not a
PriorityDefinition, override it (last one wins). If it is aPriorityDefinitionand 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.
…erge-scalar-values
Add 5 tests covering PriorityDefinition handling in ProviderConfig::merge(): - testMergeDependenciesWithPriorityDefinition - testMergeThreeConfigsWithPriorityDefinition - testMergePlainDependencyThenPriorityDefinition - testMergePriorityDefinitionThenPlainDependency - testMergeMixedDependencies These tests verify the special dependency merge logic that handles Hyperf's PriorityDefinition for DI priority resolution.
- Make ProviderConfig::mergeTwo() public static so ConfigFactory can use it - Remove duplicate mergeTwo() implementation from ConfigFactory (32 lines) - Add test verifying mergeTwo() works as public API - Update docblocks to reflect the shared architecture Both classes now use identical merge semantics from a single source of truth.
This PR fixes config merging in
ConfigFactoryandProviderConfigby implementing custom merge logic that correctly handles all Hyperf config patterns.Background
Originally, both classes used
array_merge_recursive. PR #290 changedConfigFactoryto usearray_replace_recursive, which fixed scalar values but broke list arrays. And usingArr::mergedoesn't work for mixed arrays.The problem with all standard merge options:
Bug 1: array_merge_recursive breaks scalars
Bug 2: array_replace_recursive breaks lists
Bug 3: Arr::merge loses string keys in mixed arrays
Hyperf uses this pattern for listeners with priorities:
Arr::mergetreats this as a list and loses the string key:This caused
ModelHookEventListenerto not be registered, breaking model event hooks likeHasUuids::creating().The fix
Custom merge logic that handles each key type correctly:
Changes
Tests