NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

chalasr merged 1 commit intosymfony:5.1fromck-developer:fix-empty-queue-name-aws-sqs

Jun 20, 2020

Merged

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

chalasr merged 1 commit intosymfony:5.1fromck-developer:fix-empty-queue-name-aws-sqs

Jun 20, 2020

Conversation

Copy link

Contributor

ck-developer commentedJun 19, 2020•
edited
Loading

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

ck-developer requested a review fromsroze as acode owner

June 19, 2020 21:31

carsonbot added Status: Needs Review Messenger Bug labels

Jun 19, 2020

ck-developer force-pushed thefix-empty-queue-name-aws-sqs branch fromd2e5518 to0a6ed8cCompare

June 19, 2020 21:32

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 force-pushed thefix-empty-queue-name-aws-sqs branch from0a6ed8c todf7e562Compare

June 19, 2020 21:40

ck-developer changed the base branch frommaster to5.1

June 19, 2020 21:40

chalasr added this to the5.1 milestone

Jun 19, 2020

jderusse reviewed

Jun 20, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/ConnectionTest.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Messenger/Bridge/AmazonSqs/Transport/Connection.phpShow resolvedHide resolved

ck-developer force-pushed thefix-empty-queue-name-aws-sqs branch 3 times, most recently from54d8124 tof60068dCompare

June 20, 2020 01:00

ck-developer requested a review fromjderusse

June 20, 2020 01:01

jderusse approved these changes

Jun 20, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jun 20, 2020

chalasr approved these changes

Jun 20, 2020

View reviewed changes

Nyholm reviewed

Jun 20, 2020

View reviewed changes

Copy link

Member

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?

src/Symfony/Component/Messenger/Bridge/AmazonSqs/Tests/Transport/ConnectionTest.php

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

Copy link

Member

NyholmJun 20, 2020

Choose a reason for hiding this comment

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

Suggested change

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