[Mailer] Added OhMySMTP Bridge #42710

paul-oms · 2021-08-24T15:53:42Z

This adds the https://ohmysmtp.com bridge to enable sending over the OhMySMTP API.

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	none
License	MIT
Doc PR	symfony/symfony-docs#15747...

This commit adds the https://ohmysmtp.com bridge to enable sending over the OhMySMTP API.

carsonbot · 2021-08-24T15:53:44Z

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

carsonbot · 2021-08-24T15:53:46Z

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

paul-oms · 2021-08-24T20:14:42Z

Hi Core team - from what I can tell the failing tests are all failing on the 5.4 branch as well as here. So I assume I don't need to try to fix any more of these. Let me know if that's not correct. Would love to get this into the 5.4 release, thanks!

derrabus · 2021-08-24T22:23:31Z

Hi Core team - from what I can tell the failing tests are all failing on the 5.4 branch as well as here. So I assume I don't need to try to fix any more of these.

You don't need to fix tests you didn't break. 😃

OskarStark

Thanks for providing a new bridge, this is much appreciated 👍🏻

I left some comments

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/CHANGELOG.md

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/README.md

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/Tests/Transport/OhmysmtpApiTransportTest.php

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/Tests/Transport/OhmysmtpTransportFactoryTest.php

OskarStark · 2021-08-25T06:02:32Z

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/Transport/OhmysmtpApiTransport.php

+    {
+        $response = $this->client->request('POST', 'https://'.$this->getEndpoint().'/send', [
+            'headers' => [
+                'Accept' => 'application/json',


Can be removed, as you are using the json key below, which send the header automatically

Hi @OskarStark I tested this end to end and the content-type and accept headers were not set on the outbound request, so the OhMySMTP API rejected the request - so I think we do need to leave this in?

cc @nicolas-grekas am I missing sth? It should set the headers, right?

I double checked and only Content-Type can be removed, Accept is needed if needed by the API

OskarStark · 2021-08-25T06:02:38Z

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/Transport/OhmysmtpApiTransport.php

+            'headers' => [
+                'Accept' => 'application/json',
+                'OhMySMTP-Server-Token' => $this->key,
+                'Content-Type' => 'application/json',


Can be removed, as you are using the json key below, which send the header automatically

Hi @OskarStark I tested this end to end and the content-type and accept headers were not set on the outbound request, so the OhMySMTP API rejected the request - so I think we do need to leave this in?

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/Transport/OhmysmtpSmtpTransport.php

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/Transport/OhmysmtpTransportFactory.php

src/Symfony/Component/Mailer/Bridge/Ohmysmtp/composer.json

paul-oms · 2021-08-25T19:46:14Z

@OskarStark regarding the class names, it seems Fabbot is enforcing lowercase e.g. OhmysmtpTransportFactory over OhMySmtpTransportFactory

Which way should it be?

OskarStark · 2021-08-25T20:10:15Z

To me it should be the second one

cc @fabpot

OskarStark · 2021-08-25T20:10:56Z

Maybe it is because the folder is named lowercase?

paul-oms · 2021-08-25T20:38:12Z

Maybe it is because the folder is named lowercase?

You were right, took me a bit of wrestling with git to get it right but it's there now. Thanks!

src/Symfony/Component/Mailer/Bridge/OhMySmtp/CHANGELOG.md

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Tests/Transport/OhMySmtpApiTransportTest.php

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Tests/Transport/OhMySmtpSmtpTransportTest.php

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Tests/Transport/OhMySmtpTransportFactoryTest.php

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Transport/OhMySmtpApiTransport.php

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Transport/OhMySmtpSmtpTransport.php

fabpot · 2021-08-26T05:35:12Z

@OskarStark regarding the class names, it seems Fabbot is enforcing lowercase e.g. OhmysmtpTransportFactory over OhMySmtpTransportFactory

Which way should it be?

OhMySmtpTransportFactory is the one to use to be consistent with the Symfony class naming convention.

src/Symfony/Component/Mailer/Bridge/OhMySmtp/composer.json

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

src/Symfony/Component/Mailer/Tests/Exception/UnsupportedSchemeExceptionTest.php

…o oh-my-smtp-mailer,

paul-oms · 2021-08-26T08:05:24Z

Thanks @fabpot and @OskarStark for the review. All requested changes have been completed.

src/Symfony/Component/Mailer/Bridge/OhMySmtp/CHANGELOG.md

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Tests/Transport/OhMySmtpApiTransportTest.php

OskarStark · 2021-08-26T11:29:41Z

After my comments we are good to go

fabpot · 2021-08-26T11:55:25Z

Thank you @paul-oms.

This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Mailer] Added OhMySMTP Bridge This adds the https://ohmysmtp.com bridge to enable sending over the [OhMySMTP](https://ohmysmtp.com) API. | Q | A | ------------- | --- | Branch? | 5.4  | Bug fix? | no | New feature? | yes  | Deprecations? | no  | Tickets | none  | License | MIT | Doc PR | symfony/symfony-docs#15747...   Commits ------- ce72fd8 [Mailer] Added OhMySMTP Bridge

This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Mailer] Added OhMySMTP mailer details Refs symfony/symfony#42710 Commits ------- b3dc6a8 [Mailer] Added OhMySMTP mailer details

[Mailer] Added OhMySMTP Bridge

97264a9

This commit adds the https://ohmysmtp.com bridge to enable sending over the OhMySMTP API.

carsonbot added the Status: Needs Review label Aug 24, 2021

carsonbot added Feature Mailer labels Aug 24, 2021

paul-oms added 2 commits August 24, 2021 17:00

This commit fixes the Fabbot coding standard issues

11c21e5

Fix license year

d376a5e

OskarStark reviewed Aug 25, 2021

View reviewed changes

OskarStark mentioned this pull request Aug 25, 2021

[Mailer] Added OhMySMTP mailer details symfony/symfony-docs#15747

Closed

OskarStark added this to the 5.4 milestone Aug 25, 2021

paul-oms added 2 commits August 25, 2021 16:17

Rename Ohmysmtp to OhMySmtp, add DSN example and Author

b2bffb2

Fix breaking test and move getUser after scheme check

570f27f

paul-oms added 6 commits August 25, 2021 21:15

Change directory case to match class names

01b9e82

Readd files

8f7afaf

Change directory case to match class names

1344414

Reverting back to unchanged file case

7421a13

Removed everything in ohmysmtp folders

35bbd57

Readding with the correct dir case

bd15c56

OskarStark reviewed Aug 26, 2021

View reviewed changes

fabpot requested changes Aug 26, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 26, 2021

Fixed underline in changelog, added final to class, renamed package t…

cda397c

…o oh-my-smtp-mailer,

paul-oms requested a review from fabpot August 26, 2021 08:05

carsonbot added Status: Needs Review and removed Status: Needs Work labels Aug 26, 2021

OskarStark reviewed Aug 26, 2021

View reviewed changes

src/Symfony/Component/Mailer/Bridge/OhMySmtp/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Mailer/Bridge/OhMySmtp/Tests/Transport/OhMySmtpApiTransportTest.php Outdated Show resolved Hide resolved

Fix underline & added Array type

2af88d3

fabpot approved these changes Aug 26, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Aug 26, 2021

fabpot closed this Aug 26, 2021

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mailer] Added OhMySMTP Bridge #42710

[Mailer] Added OhMySMTP Bridge #42710

paul-oms commented Aug 24, 2021 •

edited

Loading

carsonbot commented Aug 24, 2021

carsonbot commented Aug 24, 2021

paul-oms commented Aug 24, 2021

derrabus commented Aug 24, 2021

OskarStark left a comment

OskarStark Aug 25, 2021

paul-oms Aug 25, 2021

OskarStark Aug 25, 2021

OskarStark Aug 26, 2021

OskarStark Aug 25, 2021

paul-oms Aug 25, 2021

paul-oms commented Aug 25, 2021

OskarStark commented Aug 25, 2021

OskarStark commented Aug 25, 2021

paul-oms commented Aug 25, 2021

fabpot commented Aug 26, 2021

paul-oms commented Aug 26, 2021

OskarStark commented Aug 26, 2021

fabpot commented Aug 26, 2021

[Mailer] Added OhMySMTP Bridge #42710

[Mailer] Added OhMySMTP Bridge #42710

Conversation

paul-oms commented Aug 24, 2021 • edited Loading

carsonbot commented Aug 24, 2021

carsonbot commented Aug 24, 2021

paul-oms commented Aug 24, 2021

derrabus commented Aug 24, 2021

OskarStark left a comment

Choose a reason for hiding this comment

OskarStark Aug 25, 2021

Choose a reason for hiding this comment

paul-oms Aug 25, 2021

Choose a reason for hiding this comment

OskarStark Aug 25, 2021

Choose a reason for hiding this comment

OskarStark Aug 26, 2021

Choose a reason for hiding this comment

OskarStark Aug 25, 2021

Choose a reason for hiding this comment

paul-oms Aug 25, 2021

Choose a reason for hiding this comment

paul-oms commented Aug 25, 2021

OskarStark commented Aug 25, 2021

OskarStark commented Aug 25, 2021

paul-oms commented Aug 25, 2021

fabpot commented Aug 26, 2021

paul-oms commented Aug 26, 2021

OskarStark commented Aug 26, 2021

fabpot commented Aug 26, 2021

paul-oms commented Aug 24, 2021 •

edited

Loading