This is a pure refactoring PR that enables more flexibility with service injection without actually changing any behaviour or breaking backwards compatibility. It satisfies only 1 of 2 acceptance criteria of#38640 but since they're independent, I'm not marking the PR as WIP.

Receiver & sender injection into AmazonSqsTransport

It is now possible to inject your own receiver and sender intoSymfony\Component\Messenger\Bridge\AmazonSqs\Transport\AmazonSqsTransport.

Recommended way - AmazonSqsTransport::create

For clean dependency injection, I recommed using thecreate static method, which obliges you to pass all dependencies:

$transport = AmazonSqsTransport::create($connection,$receiver,$sender);

For example, this code fromSymfony\Component\Messenger\Bridge\AmazonSqs\Transport\AmazonSqsTransportFactory:

returnnewAmazonSqsTransport(Connection::fromDsn($dsn,$options),$serializer);

could be replaced with this:

$connection = Connection::fromDsn($dsn,$options);return AmazonSqsTransport::create($connection,newAmazonSqsReceiver($connection,$serializer),newAmazonSqsSender($connection,$serializer));

I didn't replace that code in the factory because I didn't find it essential but I will certainly do it in my custom factory in my project, passing my own receiver implementation.

Using the main constructor

You can still use the main constructor but it's most suited for backwards compatibility, i.e. when you don't want to inject a receiver or a sender. With the full list of arguments it gets a bit messy due to their optionality.

Minimal call

newAmazonSqsTransport($connection);

As before this PR, a receiver and a sender will be created using the default serializer, i.e.Symfony\Component\Messenger\Transport\Serialization\PhpSerializer.

With a custom serializer

newAmazonSqsTransport($connection,$serializer);

As before this PR, a receiver and a sender will be created using the passed serializer.

With a custom receiver and sender

newAmazonSqsTransport($connection,null,$receiver,$sender);

The injected services will be used. The second parameter (serializer) is unnecessary because it was only ever used while creating a receiver and a sender inside the transport. Because of this, I recommend using the new staticcreate method.

jacekwilczynski requested a review fromsroze as acode owner

October 27, 2020 18:02

carsonbot added Status: Needs Review Messenger Feature labels

Oct 27, 2020

jderusse reviewed

Oct 27, 2020

View reviewed changes

Copy link

Member

jderusse left a comment

Choose a reason for hiding this comment

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

Thank for this great work.

All transports (amqp,doctrine,redis,sqs...) are currently using the same pattern. And theXxxTransport are identical.

I wonder if it wouldn't make sens to apply this PR to all transports for consistency?

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

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

src/Symfony/Component/Messenger/Bridge/AmazonSqs/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Messenger/Bridge/AmazonSqs/Transport/AmazonSqsTransport.php Outdated

		$this->connection =$connection;
		$this->serializer =$serializer ??newPhpSerializer();
		$serializer =$serializer ??newPhpSerializer();
		$this->receiver =$receiver ??newAmazonSqsReceiver($connection,$serializer);

Copy link

Member

jderusseOct 27, 2020

Choose a reason for hiding this comment

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

Creation of receiver and sender were lay-loaded on purpose. Let these properties null and revert code in methods below.

Copy link

ContributorAuthor

jacekwilczynskiOct 27, 2020

Choose a reason for hiding this comment

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

No problem, I can do it, but may I ask why? The default receiver and sender have constructors with just basic assignments so they're extremely cheap to create, I thought it wasn't worth the additional code complexity.

chalasr requested changes

Oct 27, 2020

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

src/Symfony/Component/Messenger/Bridge/AmazonSqs/CHANGELOG.md OutdatedShow resolvedHide resolved

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

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

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

Oct 27, 2020

jderusse added this to the5.x milestone

Oct 27, 2020

Copy link

Member

chalasr commentedOct 27, 2020

I wonder if it wouldn't make sens to apply this PR to all transports for consistency?

I think so (let's make sure we are all good on the first one, we have time for 5.3.)

Copy link

ContributorAuthor

jacekwilczynski commentedOct 28, 2020

@jderusse ,@chalasr , thanks for the review, I'm really happy to see it so fast :D Forgive me if the question is banal but I couldn't find info in the guidelines: should I push another commit with the fixes or amend the previous one?

Copy link

Member

chalasr commentedOct 28, 2020

@jacekwilczynski You can amend. If you don't, we will squash your commits when merging the PR

jacekwilczynski force-pushed theamazon-sqs-extensibility branch from15a9024 tof48e7c7Compare

October 28, 2020 10:40

jderusse reviewed

Oct 28, 2020

View reviewed changes

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

jacekwilczynski force-pushed theamazon-sqs-extensibility branch 3 times, most recently from0291fd5 to828a0f4Compare

October 28, 2020 16:01

jderusse approved these changes

Oct 28, 2020

View reviewed changes

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

Oct 28, 2020

jacekwilczynski force-pushed theamazon-sqs-extensibility branch 2 times, most recently fromefd1863 to913d581Compare

November 3, 2020 13:37

Copy link

ContributorAuthor

jacekwilczynski commentedNov 3, 2020

@chalasr , anything I should still change in this PR?

derrabus approved these changes

Nov 3, 2020

View reviewed changes

Copy link

Member

derrabus left a comment

Choose a reason for hiding this comment

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

What I was wondering: Do we really need two ways to construct aAmazonSqsTransport? If not, can we change the constructor to this?

public function __construct(Connection $connection, ReceiverInterface $receiver = null, SenderInterface $sender = null)

Other than that, the change looks reasonable.

Copy link

Member

jderusse commentedNov 3, 2020

What I was wondering: Do we really need two ways to construct aAmazonSqsTransport? If not, can we change the constructor to this?

Hmm aSerializer is required to build aReceiver and aSender.. I'm not sure to get what you mean 🤔

Copy link

Member

derrabus commentedNov 3, 2020

Hmm aSerializer is required to build aReceiver and aSender..

Is it? Right now, the constructor is able to build aReceiver and aSender even if I provide a connection only.

And if you want to change the serializer implementation, you could still do that with the signature I proposed by constructingReceiver andSender yourself.

Copy link

Member

jderusse commentedNov 4, 2020

And if you want to change the serializer implementation, you could still do that with the signature I proposed by constructing Receiver and Sender yourself.

I mean, today, all bridges lazy build the sender/receiver (I don't know why. As already pointed in this PR, this looks like a micro-optimization).
Anyway, removing the serializer (which is currently injected by the TransportFactory regarding the semantic configuration) would break the initial behavior.
When people provides a custom implementation of Serializer, The factory would have to create a Transport with both a Sender AND a Receiver (which will not be lazy built anymore).