Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
derrabus merged 1 commit intosymfony:4.4fromsvitiashchuk:ticket_39846
May 22, 2021

Conversation

@svitiashchuk
Copy link
Contributor

@svitiashchuksvitiashchuk commentedJan 14, 2021
edited by nicolas-grekas
Loading

Now keys will not be considered as irrelevant.

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#39846,fix#36874
LicenseMIT
Doc PR

The problem was because in ConfigTreeoptions are defined asarrayNode and has aprototype('variable'). So for combining config togetherPrototypedArrayNode::mergeValues() was called. And there was a trouble - for thatPrototypedArrayNode$rightSide values were added without keys:

\Symfony\Component\Config\Definition\PrototypedArrayNode::mergeValues

foreach ($rightSideas$k =>$v) {// prototype, and key is irrelevant, append the elementif (null ===$this->keyAttribute) {$leftSide[] =$v;continue;    }

@carsonbot
Copy link

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:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Copy link
Member

@jderussejderusse left a 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
Copy link
ContributorAuthor

Test added

@jderusse
Copy link
Member

looks like the failed tests is related to your change

@svitiashchuk
Copy link
ContributorAuthor

Yes, it was. Now, I've changed the argument$name value of methodArrayNodeDefinition::useAttributeAsKey to'option'. Because in fixture configmessenger_transports.php there was a piece:

'options' => ['queue' => ['name' => 'Queue']],

for which'name' => 'Queue' was considered as key for option, and therefore, rewritten the original key'queue' with'Queue'. Now that problem solved.

P.S. I'm not sure about that, but is that a normal behaviour that original key replaced whenattributeAsKey is set and original key is present in config simultaneously. Should I create another one issue?

@jderusse
Copy link
Member

Indeed, the attributeAsKey should not be an expected subkey of the array.

@OskarStark
Copy link
Contributor

CleanShot 2021-01-15 at 15 49 24

It looks like your committer email is not associated with your Github account 🤔

@svitiashchuk
Copy link
ContributorAuthor

@OskarStark yes, thanks for pointing. Did it in different environment, but now fixed it.

OskarStark reacted with thumbs up emoji

Copy link
Member

@xabbuhxabbuh left a 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?

Copy link
ContributorAuthor

@svitiashchuksvitiashchuk left a 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
Copy link
Member

@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
Copy link
ContributorAuthor

@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\Symfony\Component\Config\Definition\PrototypedArrayNode::mergeValues:

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 conditionnull === $this->keyAttribute is not enough to decide to append value as plain array item (I mean, not an associative array element). Therefore, I think we need to add an additional condition, like it is done in\Symfony\Component\Config\Definition\PrototypedArrayNode::normalizeValue. There we can see the next lines:

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$isAssoc variable affects the way normalized value is added to resulting$normalized array.
If you wouldn't mind, I will provide corresponding changes.

@xabbuh
Copy link
Member

This sounds like a solution that could work. Let's give that a try. For5.x I have pending changes locally that allow to configure list and map nodes explicitly which would allow an easier solution. But that's of course not something we can use here for the bugfix.

@svitiashchuk
Copy link
ContributorAuthor

Now it seems like everything is ok :)
Thanks to my colleague I realized that I had written test in different bundle. Therefore, on Travis CI check it was impossible to test changes that I had provided. So I've removed that test and wrote another one in relevant directory.

I would be very happy if this is merged.
@xabbuh could you please take a look at it?

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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 :)

@svitiashchuksvitiashchuk changed the title[Messenger] Messenger options were merged with numbers as keys.[Config Component] Merging PrototypedArrayNode associative valuesFeb 16, 2021
@Sscorpion9991
Copy link

Hi! Is this PR going to be merged? I have the same issue.

@nicolas-grekasnicolas-grekas changed the title[Config Component] Merging PrototypedArrayNode associative values[Config] Merging PrototypedArrayNode associative valuesApr 20, 2021
@nicolas-grekasnicolas-grekas changed the title[Config] Merging PrototypedArrayNode associative values[Config] Fix merging PrototypedArrayNode associative valuesApr 20, 2021
@carsonbotcarsonbot changed the title[Config] Fix merging PrototypedArrayNode associative values[Messenger] Fix merging PrototypedArrayNode associative valuesApr 20, 2021
@derrabus
Copy link
Member

Thank you @svityashchuk.

@derrabusderrabus merged commit53d9b10 intosymfony:4.4May 22, 2021
This was referencedMay 31, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@jderussejderussejderusse approved these changes

@OskarStarkOskarStarkOskarStark left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@derrabusderrabusderrabus approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@maxheliasmaxheliasmaxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

9 participants

@svitiashchuk@carsonbot@jderusse@OskarStark@xabbuh@Sscorpion9991@derrabus@nicolas-grekas@maxhelias

[8]ページ先頭

©2009-2025 Movatter.jp