[Messenger] Make all the dependencies of AmazonSqsTransport injectable #38846

jacekwilczynski · 2020-10-27T18:02:08Z

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?

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

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

src/Symfony/Component/Messenger/Bridge/AmazonSqs/CHANGELOG.md

jderusse · 2020-10-27T19:58:26Z

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

    {
        $this->connection = $connection;
-        $this->serializer = $serializer ?? new PhpSerializer();
+        $serializer = $serializer ?? new PhpSerializer();
+        $this->receiver = $receiver ?? new AmazonSqsReceiver($connection, $serializer);


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

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

src/Symfony/Component/Messenger/Bridge/AmazonSqs/CHANGELOG.md

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

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

chalasr · 2020-10-27T20:26:37Z

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 · 2020-10-28T10:03:25Z

@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 · 2020-10-28T10:05:38Z

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

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

jacekwilczynski · 2020-11-03T14:16:47Z

@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 · 2020-11-03T16:34:36Z

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 · 2020-11-03T16:55:43Z

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 · 2020-11-04T02:33:23Z

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 · 2020-11-04T06:49:31Z

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 · 2020-11-15T22:33:22Z

Thank you @jacekwilczynski.

jacekwilczynski requested a review from sroze as a code 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

chalasr requested changes Oct 27, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 27, 2020

jderusse added this to the 5.x milestone Oct 27, 2020

jacekwilczynski force-pushed the amazon-sqs-extensibility branch from 15a9024 to f48e7c7 Compare October 28, 2020 10:40

jderusse reviewed Oct 28, 2020

View reviewed changes

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

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

jacekwilczynski force-pushed the amazon-sqs-extensibility branch 3 times, most recently from 0291fd5 to 828a0f4 Compare 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 the amazon-sqs-extensibility branch 2 times, most recently from efd1863 to 913d581 Compare November 3, 2020 13:37

derrabus approved these changes Nov 3, 2020

View reviewed changes

derrabus approved these changes Nov 4, 2020

View reviewed changes

[Messenger] Make all the dependencies of AmazonSqsTransport injectable

281af26

derrabus force-pushed the amazon-sqs-extensibility branch from 913d581 to 281af26 Compare November 15, 2020 22:33

derrabus merged commit b77ef9f into symfony:5.x Nov 15, 2020

jacekwilczynski deleted the amazon-sqs-extensibility branch January 26, 2021 08:06

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Make all the dependencies of AmazonSqsTransport injectable #38846

[Messenger] Make all the dependencies of AmazonSqsTransport injectable #38846

jacekwilczynski commented Oct 27, 2020

jderusse left a comment

jderusse Oct 27, 2020

jacekwilczynski Oct 27, 2020

chalasr left a comment

chalasr commented Oct 27, 2020

jacekwilczynski commented Oct 28, 2020

chalasr commented Oct 28, 2020

jacekwilczynski commented Nov 3, 2020

derrabus left a comment

jderusse commented Nov 3, 2020

derrabus commented Nov 3, 2020

jderusse commented Nov 4, 2020

derrabus commented Nov 4, 2020

derrabus left a comment

derrabus commented Nov 15, 2020

[Messenger] Make all the dependencies of AmazonSqsTransport injectable #38846

[Messenger] Make all the dependencies of AmazonSqsTransport injectable #38846

Conversation

jacekwilczynski commented Oct 27, 2020

Receiver & sender injection into AmazonSqsTransport

Recommended way - AmazonSqsTransport::create

Using the main constructor

Minimal call

With a custom serializer

With a custom receiver and sender

jderusse left a comment

Choose a reason for hiding this comment

jderusse Oct 27, 2020

Choose a reason for hiding this comment

jacekwilczynski Oct 27, 2020

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

chalasr commented Oct 27, 2020

jacekwilczynski commented Oct 28, 2020

chalasr commented Oct 28, 2020

jacekwilczynski commented Nov 3, 2020

derrabus left a comment

Choose a reason for hiding this comment

jderusse commented Nov 3, 2020

derrabus commented Nov 3, 2020

jderusse commented Nov 4, 2020

derrabus commented Nov 4, 2020

derrabus left a comment

Choose a reason for hiding this comment

derrabus commented Nov 15, 2020