[Yaml] Stop replacing NULLs when merging #21756
Conversation
|
What about using array_replace instead ? It would be clearer imo. |
|
There is a difference between the removed and the new code : any pre-existing |
|
@GuilhemN Yeah it would be clearer since this operator isn't widely used, but array_replace is also slower. I try to make my changes without performance impact. I think we can instead solve this by adding // comment to indicate what we are doing if you wish. What do you guys think? @nicolas-grekas Yep I'm aware of that. I'll let you guys decide what is correct behaviour, then I will modify my patch appropriately, even if it's only adding the test case. IMO duplicate keys are not supposed to be overwriting any previously set values. In 3.2 there was even deprecated ability to specify duplicate keys. I can even modify this to make this trigger deprecation instead, however I don't know what message is appropriate, since I'm not familiar what does this block of parser do. |
|
@nicolas-grekas I didn't check but I don't see any good reason justifiing a different behavior for |
|
Nothing is said about |
|
@gadelat can you add a test case in this direction please? |
|
👍 This will solve the initial overlook that didn't take into account that Status: Reviewed |
|
Thank you @gadelat. |
This PR was merged into the 2.7 branch. Discussion ---------- [Yaml] Stop replacing NULLs when merging | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This introduces slight change of behaviour. Whereas previous code is overwriting already processed NULL values, this code is not. I think this is more expected behaviour, though? Commits ------- d967440 [Yaml] Stop replacing NULLs when merging
This introduces slight change of behaviour. Whereas previous code is overwriting already processed NULL values, this code is not. I think this is more expected behaviour, though?