Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Fix merging PrototypedArrayNode associative values#39847
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJan 14, 2021
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
jderusse left a comment
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.
could you please add a test?
seehttps://github.com/symfony/symfony/pull/39686/files for similar PR
svitiashchuk commentedJan 15, 2021
Test added |
jderusse commentedJan 15, 2021
looks like the failed tests is related to your change |
svitiashchuk commentedJan 15, 2021
Yes, it was. Now, I've changed the argument for which P.S. I'm not sure about that, but is that a normal behaviour that original key replaced when |
jderusse commentedJan 15, 2021
Indeed, the attributeAsKey should not be an expected subkey of the array. |
src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
OskarStark commentedJan 15, 2021
svitiashchuk commentedJan 15, 2021
@OskarStark yes, thanks for pointing. Did it in different environment, but now fixed it. |
xabbuh left a comment
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.
I am not convinced that this will always work. What if there is a queue where one option is namedoption?
svitiashchuk left a comment
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.
@xabbuh I've added anoption key inside test config options array to prove that this will work.
xabbuh commentedJan 18, 2021
@svityashchuk If you change your example to something like this, you will experience the same behavior: 'options' => ['auto_setup' =>true,'queue' => ['name' =>'Queue','option' =>'foo', ],], |
svitiashchuk commentedJan 19, 2021
@xabbuh thank you for providing an example. I've got that behaviour. So, in the circumstances I see the only possibility to use another keyword as keyAttribute. Otherwise, I won't be able to pass this check at foreach ($rightSideas$k =>$v) {// prototype, and key is irrelevant, append the elementif (null ===$this->keyAttribute) {$leftSide[] =$v;continue; } Moreover, now it seems for me like this condition protectedfunction normalizeValue($value){//...$isAssoc =array_keys($value) !==range(0,\count($value) -1);//...foreach ($valueas$k =>$v) {//...if (null !==$this->keyAttribute ||$isAssoc) {$normalized[$k] =$prototype->normalize($v); }else {$normalized[] =$prototype->normalize($v); } Pay attention that |
xabbuh commentedJan 20, 2021
This sounds like a solution that could work. Let's give that a try. For |
svitiashchuk commentedJan 27, 2021
Now it seems like everything is ok :) I would be very happy if this is merged. |
Uh oh!
There was an error while loading.Please reload this page.
a896d1b to004abd4CompareUh oh!
There was an error while loading.Please reload this page.
nicolas-grekas left a comment
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.
Please update the PR title - it's not about Messenger :)
Sscorpion9991 commentedApr 20, 2021
Hi! Is this PR going to be merged? I have the same issue. |
derrabus commentedMay 22, 2021
Thank you @svityashchuk. |

Uh oh!
There was an error while loading.Please reload this page.
Now keys will not be considered as irrelevant.
The problem was because in ConfigTree
optionsare defined asarrayNodeand has aprototype('variable'). So for combining config togetherPrototypedArrayNode::mergeValues()was called. And there was a trouble - for thatPrototypedArrayNode$rightSidevalues were added without keys:\Symfony\Component\Config\Definition\PrototypedArrayNode::mergeValues