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

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

Merged
merged 1 commit into from Jan 29, 2019

Conversation

@thePanz
Copy link
Contributor

thePanz commented Dec 8, 2018

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29044
License MIT

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

\cc @thomaskonrad

@thePanz thePanz changed the base branch from 4.2 to 4.1 Dec 8, 2018

@thePanz thePanz changed the title [Messenger] fixed RabbitMQ arguments not passed as integer values [Messenger] fixed RabbitMQ arguments not passed as integer values #SymfonyConHackday2018 Dec 8, 2018

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 8, 2018

return $queueOptions;
}
foreach ($queueOptions['arguments'] as $key => $value) {

This comment has been minimized.

@ro0NL

ro0NL Dec 8, 2018

Contributor

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 add private function getArguments() and normalize when needed (i.e. setArguments(getArguments())

This comment has been minimized.

@thePanz

thePanz Dec 15, 2018

Author Contributor

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

@alquerci
Copy link
Contributor

alquerci left a comment

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

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

Status: Needs Work

@thePanz thePanz force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from 7eff179 to 31dfeee Dec 15, 2018

@alquerci
Copy link
Contributor

alquerci left a comment

@thePanz Changes done from last review pass is good except for one 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 thePanz force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from fd0f0e2 to dab71ef Dec 16, 2018

@thePanz

This comment has been minimized.

Copy link
Contributor Author

thePanz commented Dec 16, 2018

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

@thePanz thePanz closed this Dec 16, 2018

@thePanz thePanz reopened this Dec 16, 2018

@thePanz thePanz force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from 86595d0 to a595a99 Dec 16, 2018

@alquerci
Copy link
Contributor

alquerci left a comment

@thePanz For tests I think it could be enough.

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.

@ro0NL, what do you think?

@thePanz thePanz force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from 66169e4 to 108d1d2 Dec 17, 2018

@alquerci
Copy link
Contributor

alquerci left a comment

@thePanz Look at fabbot.io review.

@thePanz thePanz force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from 9e20d1d to 382ead4 Dec 17, 2018

@thePanz

This comment has been minimized.

Copy link
Contributor Author

thePanz commented Dec 17, 2018

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

@alquerci

This comment has been minimized.

Copy link
Contributor

alquerci commented Dec 17, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

thePanz commented Jan 16, 2019

Any updates @alquerci @ro0NL @Toflar ?

@Toflar

This comment has been minimized.

Copy link
Contributor

Toflar commented Jan 16, 2019

Looks good to me.

@alquerci

This comment has been minimized.

Copy link
Contributor

alquerci commented Jan 16, 2019

👌

@nicolas-grekas nicolas-grekas force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch 2 times, most recently from 6e11eed to 7c428ef Jan 25, 2019

@nicolas-grekas nicolas-grekas force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from 7c428ef to 55985df Jan 25, 2019

@xabbuh xabbuh modified the milestones: 4.1, 4.2 Jan 29, 2019

@xabbuh

xabbuh approved these changes Jan 29, 2019

@nicolas-grekas nicolas-grekas changed the title [Messenger] fixed RabbitMQ arguments not passed as integer values #SymfonyConHackday2018 [Messenger] fixed RabbitMQ arguments not passed as integer values Jan 29, 2019

@nicolas-grekas nicolas-grekas changed the base branch from 4.1 to 4.2 Jan 29, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 29, 2019

Thank you @thePanz.

@nicolas-grekas nicolas-grekas force-pushed the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch from 55985df to f19c035 Jan 29, 2019

@nicolas-grekas nicolas-grekas merged commit f19c035 into symfony:4.2 Jan 29, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jan 29, 2019

bug #29532 [Messenger] fixed RabbitMQ arguments not passed as integer…
… 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       | MIT

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

\cc @thomaskonrad

Commits
-------

f19c035 [Messenger] fixed RabbitMQ arguments not passed as integer values

@thePanz thePanz deleted the thePanz:issue-29044-messenger-fix-rabbitmq-int-arguments branch Jan 29, 2019

@fabpot fabpot referenced this pull request Feb 3, 2019

Merged

Release v4.2.3 #30068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment