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] fixed RabbitMQ arguments not passed as integer values#29532

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
nicolas-grekas merged 1 commit intosymfony:4.2fromthePanz:issue-29044-messenger-fix-rabbitmq-int-arguments
Jan 29, 2019
Merged

[Messenger] fixed RabbitMQ arguments not passed as integer values#29532

nicolas-grekas merged 1 commit intosymfony:4.2fromthePanz:issue-29044-messenger-fix-rabbitmq-int-arguments
Jan 29, 2019

Conversation

@thePanz
Copy link
Contributor

@thePanzthePanz commentedDec 8, 2018•
edited
Loading

QA
Branch?4.2
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#29044
LicenseMIT

RabbitMQ expects some arguments to be passed as integer values.
Make sure to cast those after parsing the DSN

\cc@thomaskonrad

nicholasruunu and ro0NL reacted with thumbs up emoji
@thePanzthePanz changed the base branch from4.2 to4.1December 8, 2018 15:16
@thePanzthePanz changed the title[Messenger] fixed RabbitMQ arguments not passed as integer values[Messenger] fixed RabbitMQ arguments not passed as integer values #SymfonyConHackday2018Dec 8, 2018
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneDec 8, 2018
return$queueOptions;
}

foreach ($queueOptions['arguments']as$key =>$value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you could do

foreach (array('x-max-..', ...) as $key) {   if (isset($queueOptions['arguments'][$key]) { ... }}

and save a few lines. Also im wondering if it's better to addprivate function getArguments() and normalize when needed (i.e.setArguments(getArguments())

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I refactored that part. I added a constant keeping all the arguments that should be an integer, and later iterating over that. wdyt?

Copy link
Contributor

@alquercialquerci left a comment

Choose a reason for hiding this comment

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

This patch is a good entry point to find solution of the bug.

But it left to provide a scale-able solution forarguments schema.

Status: Needs Work

Copy link
Contributor

@alquercialquerci left a comment•
edited
Loading

Choose a reason for hiding this comment

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

@thePanz Changes done from last review pass is good except forone where I have a question.

Also, is it possible to have tests for all arguments keys and for edge cases like when an queue argument value is null?

Status: Needs Review

@thePanz
Copy link
ContributorAuthor

@alquerci added handling of null and not-integer values, maybe some more tests should be added?

@thePanzthePanz reopened thisDec 16, 2018
Copy link
Contributor

@alquercialquerci left a comment

Choose a reason for hiding this comment

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

@thePanz For tests I think it could be enough.

I have a doubt about how to handlenull, because it could mean that the value is not set. Then, in that case removing the key instead of throwing an exception will do the stuff.

@ro0NL, what do you think?

Copy link
Contributor

@alquercialquerci left a comment

Choose a reason for hiding this comment

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

@thePanz Look atfabbot.io review.

@thePanz
Copy link
ContributorAuthor

@alquerci is there any other change to be done? :)
Still, I don't know why the@carsonbot added theneeds work label 🤔

@alquerci
Copy link
Contributor

alquerci commentedDec 17, 2018•
edited
Loading

@thePanz

Still, I don't know why the@carsonbot added the needs work label 🤔

It was triggered by my last review pass#29532 (review)

Now the patch looks good except for the doubt I have:

I have a doubt about how to handle null, because it could mean that the value is not set. Then, in that case removing the key instead of throwing an exception will do the stuff.

thePanz reacted with thumbs up emoji

@thePanz
Copy link
ContributorAuthor

Any updates@alquerci@ro0NL@Toflar ?

@Toflar
Copy link
Contributor

Looks good to me.

@alquerci
Copy link
Contributor

👌

@xabbuhxabbuh modified the milestones:4.1,4.2Jan 29, 2019
@nicolas-grekasnicolas-grekas changed the title[Messenger] fixed RabbitMQ arguments not passed as integer values #SymfonyConHackday2018[Messenger] fixed RabbitMQ arguments not passed as integer valuesJan 29, 2019
@nicolas-grekasnicolas-grekas changed the base branch from4.1 to4.2January 29, 2019 12:42
@nicolas-grekas
Copy link
Member

Thank you@thePanz.

@nicolas-grekasnicolas-grekas merged commitf19c035 intosymfony:4.2Jan 29, 2019
nicolas-grekas added a commit that referenced this pull requestJan 29, 2019
… values (thePanz)This PR was submitted for the 4.1 branch but it was merged into the 4.2 branch instead (closes#29532).Discussion----------[Messenger] fixed RabbitMQ arguments not passed as integer values| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#29044| License       | MITRabbitMQ expects some arguments to be passed as integer values.Make sure to cast those after parsing the DSN\cc@thomaskonradCommits-------f19c035 [Messenger] fixed RabbitMQ arguments not passed as integer values
@thePanzthePanz deleted the issue-29044-messenger-fix-rabbitmq-int-arguments branchJanuary 29, 2019 14:04
@fabpotfabpot mentioned this pull requestFeb 3, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

+6 more reviewers

@ToflarToflarToflar left review comments

@greg0iregreg0iregreg0ire left review comments

@ro0NLro0NLro0NL left review comments

@thomaskonradthomaskonradthomaskonrad left review comments

@diogorodriguescdiogorodriguescdiogorodriguesc left review comments

@alquercialquercialquerci approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

10 participants

@thePanz@alquerci@Toflar@nicolas-grekas@greg0ire@ro0NL@xabbuh@thomaskonrad@diogorodriguesc@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp