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
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 29, 2019
Merged

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

merged 1 commit into from
Jan 29, 2019

Conversation

thePanz
Copy link
Contributor

@thePanz thePanz commented Dec 8, 2018 •
edited
Loading

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 December 8, 2018 15:16
@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
alquerci
alquerci reviewed Dec 8, 2018
View reviewed changes
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
ro0NL
ro0NL reviewed Dec 8, 2018
View reviewed changes
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated
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 add private function getArguments() and normalize when needed (i.e. setArguments(getArguments())

Copy link
Contributor Author

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?

greg0ire
greg0ire reviewed Dec 9, 2018
View reviewed changes
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
alquerci
alquerci suggested changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@alquerci alquerci 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 for arguments schema.

Status: Needs Work

src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
alquerci
alquerci reviewed Dec 15, 2018
View reviewed changes
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
alquerci
alquerci reviewed Dec 15, 2018
View reviewed changes
Copy link
Contributor

@alquerci alquerci 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 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
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
alquerci
alquerci reviewed Dec 16, 2018
View reviewed changes
Copy link
Contributor

@alquerci alquerci 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 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?

src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/ConnectionTest.php Outdated Show resolved Hide resolved
alquerci
alquerci suggested changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@alquerci alquerci 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 at fabbot.io review.

src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/ConnectionTest.php Outdated Show resolved Hide resolved
@thePanz
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
Copy link
Contributor

alquerci commented Dec 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
Copy link
Contributor Author

thePanz commented Jan 16, 2019

Any updates @alquerci @ro0NL @Toflar ?

@Toflar
Copy link
Contributor

Toflar commented Jan 16, 2019

Looks good to me.

@alquerci
Copy link
Contributor

👌

@xabbuh xabbuh modified the milestones: 4.1, 4.2 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 January 29, 2019 12:42
@nicolas-grekas
Copy link
Member

Thank you @thePanz.

@nicolas-grekas nicolas-grekas merged commit f19c035 into symfony:4.2 Jan 29, 2019
nicolas-grekas added a commit that referenced this pull request Jan 29, 2019
@nicolas-grekas
bug #29532 [Messenger] fixed RabbitMQ arguments not passed as integer…
50a0bda 
… 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 issue-29044-messenger-fix-rabbitmq-int-arguments branch January 29, 2019 14:04
@fabpot fabpot mentioned this pull request Feb 3, 2019
Merged
@weaverryan weaverryan mentioned this pull request Mar 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Toflar Toflar Toflar left review comments

@greg0ire greg0ire greg0ire left review comments

@ro0NL ro0NL ro0NL left review comments

@thomaskonrad thomaskonrad thomaskonrad left review comments

@diogorodriguesc diogorodriguesc diogorodriguesc left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@xabbuh xabbuh xabbuh approved these changes

@alquerci alquerci alquerci approved these changes

Assignees
No one assigned
Labels
Bug Messenger Status: Reviewed
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.