Skip to content

[Messenger] fixed queue_name option on amazon sqs connection #37364

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

Merged
merged 1 commit into from
Jun 20, 2020

Conversation

ck-developer
Copy link
Contributor

@ck-developer ck-developer commented Jun 19, 2020

Q A
Branch? 5.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #37293
License MIT

@ck-developer ck-developer requested a review from sroze as a code owner June 19, 2020 21:31
@ck-developer ck-developer force-pushed the fix-empty-queue-name-aws-sqs branch from d2e5518 to 0a6ed8c Compare June 19, 2020 21:32
@ck-developer ck-developer changed the title [Messenger] fix queue_name option on amazon sqs connection [Messenger] fixed queue_name option on amazon sqs connection Jun 19, 2020
@ck-developer ck-developer force-pushed the fix-empty-queue-name-aws-sqs branch from 0a6ed8c to df7e562 Compare June 19, 2020 21:40
@ck-developer ck-developer changed the base branch from master to 5.1 June 19, 2020 21:40
@chalasr chalasr added this to the 5.1 milestone Jun 19, 2020
@ck-developer ck-developer force-pushed the fix-empty-queue-name-aws-sqs branch 3 times, most recently from 54d8124 to f60068d Compare June 20, 2020 01:00
@ck-developer ck-developer requested a review from jderusse June 20, 2020 01:01
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

What about adding a test for when you provide both a parameter and add a DSN with a queue name in it?

What should be the preferred result?

new Connection(['queue_name' => 'queue'], new SqsClient(['region' => 'eu-west-1', 'accessKeyId' => null, 'accessKeySecret' => null], null, $httpClient)),
Connection::fromDsn('sqs://default', ['queue_name' => 'queue'], $httpClient)
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
$this->assertEquals(
new Connection(['queue_name' => 'my_queue'], new SqsClient(['region' => 'eu-west-1', 'accessKeyId' => null, 'accessKeySecret' => null], null, $httpClient)),
Connection::fromDsn('sqs://default/my_queue', ['queue_name' => 'queue_ignored'], $httpClient)
);
}

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@chalasr
Copy link
Member

chalasr commented Jun 20, 2020

Good catch, thanks @ck-developer.

@fabpot fabpot mentioned this pull request Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Messenger] queue_name option not used in amazon sqs connection
5 participants