[Messenger] Make all the dependencies of AmazonSqsTransport injectable

[jacekwilczynski]

Q	A
Branch?	5.x for features
Bug fix?	no
New feature?	yes - updated changelog
Deprecations?	no
Tickets	Fix #38640
License	MIT
Doc PR	symfony/symfony-docs#...

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 into Symfony\Component\Messenger\Bridge\AmazonSqs\Transport\AmazonSqsTransport.

Recommended way - AmazonSqsTransport::create

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

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

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

return new AmazonSqsTransport(Connection::fromDsn($dsn, $options), $serializer);

could be replaced with this:

$connection = Connection::fromDsn($dsn, $options);

return AmazonSqsTransport::create(
    $connection,
    new AmazonSqsReceiver($connection, $serializer),
    new AmazonSqsSender($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

new AmazonSqsTransport($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

new AmazonSqsTransport($connection, $serializer);

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

With a custom receiver and sender

new AmazonSqsTransport($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 static create method.

[jderusse]

Thank for this great work.

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

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

[jderusse]

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

[jacekwilczynski]

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]

Thanks for the PR

[chalasr]

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

[jacekwilczynski]

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

[chalasr]

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

[jacekwilczynski]

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

[derrabus]

What I was wondering: Do we really need two ways to construct a AmazonSqsTransport? 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.

[jderusse]

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

Hmm a Serializer is required to build a Receiver and a Sender.. I'm not sure to get what you mean 🤔

[derrabus]

Hmm a Serializer is required to build a Receiver and a Sender..

Is it? Right now, the constructor is able to build a Receiver and a Sender 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 constructing Receiver and Sender yourself.

[jderusse]

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

[derrabus]

Let's keep it that way, thanks for the explanation!

[derrabus]

Let's wait until 5.2 has been branched out and merge to 5.x then.

[derrabus]

Thank you @jacekwilczynski.