[Notifier] Add Amazon SNS bridge #39141

adrien-chinour · 2020-11-22T21:03:00Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	symfony/symfony-docs#15486
Recipe PR	symfony/recipes#847

Hi,

This PR add a bridge on Notifier component for Amazon SNS.

This bridge use async-aws/sns and only work on actual dev-master version of asyc-aws/core.

I'm working on recipe and doc PR.

carsonbot · 2020-11-22T21:03:04Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransportFactory.php

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransport.php

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransportFactory.php

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransport.php

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransportFactory.php

src/Symfony/Component/Notifier/Bridge/Amazon/Tests/AmazonTransportFactoryTest.php

src/Symfony/Component/Notifier/Bridge/Amazon/.gitattributes

src/Symfony/Component/Notifier/Bridge/Amazon/LICENSE

src/Symfony/Component/Notifier/Bridge/Amazon/README.md

src/Symfony/Component/Notifier/Bridge/Amazon/Tests/AmazonTransportFactoryTest.php

src/Symfony/Component/Notifier/Bridge/Amazon/composer.json

src/Symfony/Component/Notifier/Bridge/Amazon/.gitattributes

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransport.php

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransportFactory.php

composer.json

src/Symfony/Component/Notifier/Bridge/Amazon/LICENSE

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransport.php

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsOptions.php

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransport.php

src/Symfony/Component/Notifier/Bridge/AmazonSns/README.md

src/Symfony/Component/Notifier/Exception/UnsupportedSchemeException.php

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransportFactory.php

OskarStark · 2021-07-02T08:45:06Z

Please create Doc PR, thanks

jderusse · 2021-07-02T09:13:47Z

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransport.php

+        $response = $this->snsClient->publish($options);
+
+        $message = new SentMessage($message, (string) $this);
+        $message->setMessageId($response->getMessageId());


I would wrap this in a try/catch block to convert AsyncAws exceptions into TransportException

fabpot · 2021-07-04T12:23:43Z

@adrien-chinour Are you still interested in finishing this implementation?

adrien-chinour · 2021-07-04T23:30:53Z

@adrien-chinour Are you still interested in finishing this implementation?

Yes, I will work on it.

composer.json

OskarStark · 2021-07-13T19:18:35Z

src/Symfony/Component/Notifier/Bridge/AmazonSns/composer.json

+    "require": {
+        "php": ">=7.2.5",
+        "symfony/http-client": "^4.4|^5.2",
+        "symfony/notifier": "^5.3",


Suggested change

"symfony/notifier": "^5.3",

"symfony/notifier": "^5.4",

OskarStark · 2021-07-13T19:19:24Z

Please add the bridge to the UnsupportedSchemeExceptionTest. Thanks

jderusse · 2021-07-13T19:24:08Z

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsOptions.php

+    public function toArray(): array
+    {
+        $options = $this->options;
+        unset($options['recipient_id']);


Why storing the recipient in the option property if we don't use it not a standard option?

@jderusse Yes, I change it to a class property and a required parameter on constructor.

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransport.php

Nyholm

Thank you.

I just had some small comments.

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransport.php

OskarStark · 2021-07-20T06:59:03Z

Final review @Nyholm ?

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransport.php

fabpot · 2021-07-25T15:14:52Z

Thank you @adrien-chinour.

This PR was merged into the 5.4 branch. Discussion ---------- [Notifier] Add AmazonSns Bridge AmazonSns Bridge : [symfony/symfony #39141](symfony/symfony#39141) Commits ------- 6ab1a8d [Notifier] Add AmazonSns Bridge

carsonbot added Status: Needs Review Notifier Feature labels Nov 22, 2020

adrien-chinour mentioned this pull request Nov 22, 2020

add recipe for amazon-notifier symfony/recipes#847

Merged

jderusse reviewed Nov 22, 2020

View reviewed changes

OskarStark reviewed Nov 23, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransport.php Outdated Show resolved Hide resolved

adrien-chinour requested a review from jderusse November 30, 2020 22:22

jderusse reviewed Dec 1, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/AmazonTransportFactory.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Amazon/Tests/AmazonTransportFactoryTest.php Outdated Show resolved Hide resolved

jderusse modified the milestones: 5.1, 5.x Dec 4, 2020

adrien-chinour requested a review from jderusse December 6, 2020 16:43

OskarStark reviewed Dec 8, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/README.md Outdated Show resolved Hide resolved

OskarStark reviewed Dec 8, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/README.md Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Bridge/Amazon/README.md Outdated Show resolved Hide resolved

OskarStark reviewed Dec 10, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/README.md Outdated Show resolved Hide resolved

OskarStark reviewed Dec 10, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/Tests/AmazonTransportFactoryTest.php Outdated Show resolved Hide resolved

OskarStark reviewed Dec 10, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/Amazon/composer.json Outdated Show resolved Hide resolved

jderusse reviewed Dec 10, 2020

View reviewed changes

fabpot requested changes Dec 11, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 11, 2020

adrien-chinour requested a review from fabpot December 11, 2020 18:20

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 11, 2020

OskarStark reviewed Dec 12, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransportFactory.php Outdated Show resolved Hide resolved

OskarStark reviewed Dec 12, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransportFactory.php Outdated Show resolved Hide resolved

OskarStark reviewed Dec 12, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransportFactory.php Outdated Show resolved Hide resolved

OskarStark reviewed Dec 12, 2020

View reviewed changes

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransportFactory.php Outdated Show resolved Hide resolved

OskarStark requested review from jderusse and removed request for jderusse July 2, 2021 08:45

carsonbot added Status: Needs Review and removed Status: Reviewed labels Jul 2, 2021

OskarStark requested a review from Nyholm July 2, 2021 08:45

jderusse reviewed Jul 2, 2021

View reviewed changes

adrien-chinour mentioned this pull request Jul 4, 2021

[Notifier] Add AmazonSns Bridge symfony/symfony-docs#15486

Merged

adrien-chinour requested a review from OskarStark July 11, 2021 22:53

OskarStark requested a review from jderusse July 13, 2021 19:14

OskarStark approved these changes Jul 13, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 13, 2021

OskarStark reviewed Jul 13, 2021

View reviewed changes

composer.json Show resolved Hide resolved

OskarStark reviewed Jul 13, 2021

View reviewed changes

jderusse reviewed Jul 13, 2021

View reviewed changes

Nyholm reviewed Jul 19, 2021

View reviewed changes

jderusse reviewed Jul 20, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/AmazonSns/AmazonSnsTransport.php Outdated Show resolved Hide resolved

fabpot approved these changes Jul 25, 2021

View reviewed changes

[Notifier] Add Amazon SNS bridge

3dc6ad4

fabpot merged commit ccd3a15 into symfony:5.4 Jul 25, 2021

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

This comment was marked as off-topic.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Notifier] Add Amazon SNS bridge #39141

[Notifier] Add Amazon SNS bridge #39141

adrien-chinour commented Nov 22, 2020 •

edited

Loading

carsonbot commented Nov 22, 2020

OskarStark commented Jul 2, 2021

jderusse Jul 2, 2021 •

edited

Loading

fabpot commented Jul 4, 2021

adrien-chinour commented Jul 4, 2021

OskarStark Jul 13, 2021

OskarStark commented Jul 13, 2021

jderusse Jul 13, 2021

adrien-chinour Jul 19, 2021

Nyholm left a comment

OskarStark commented Jul 20, 2021

fabpot commented Jul 25, 2021

This comment was marked as off-topic.

[Notifier] Add Amazon SNS bridge #39141

[Notifier] Add Amazon SNS bridge #39141

Conversation

adrien-chinour commented Nov 22, 2020 • edited Loading

carsonbot commented Nov 22, 2020

OskarStark commented Jul 2, 2021

jderusse Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

fabpot commented Jul 4, 2021

adrien-chinour commented Jul 4, 2021

OskarStark Jul 13, 2021

Choose a reason for hiding this comment

OskarStark commented Jul 13, 2021

jderusse Jul 13, 2021

Choose a reason for hiding this comment

adrien-chinour Jul 19, 2021

Choose a reason for hiding this comment

Nyholm left a comment

Choose a reason for hiding this comment

OskarStark commented Jul 20, 2021

fabpot commented Jul 25, 2021

This comment was marked as off-topic.

adrien-chinour commented Nov 22, 2020 •

edited

Loading

jderusse Jul 2, 2021 •

edited

Loading