Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Implement Symfony Mailer #38481

Merged
merged 38 commits into from Sep 14, 2021
Merged

[9.x] Implement Symfony Mailer #38481

merged 38 commits into from Sep 14, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Aug 20, 2021

This PR replaces SwiftMailer with Symfony Mailer. Special thanks to @Jubeki for his help with this PR.

See https://symfony.com/blog/the-end-of-swiftmailer

Todo

Breaking changes

Possible improvements

  • We could wrap Symfony\Component\Mailer\SentMessage to add some nice API methods like retrieving recipients, etc
  • It might be interesting to provide support for MAILER_DSN strings to set up transports. This will allow much more thorough configuration for transports and we could at the same time provide BC support for the old configuration schemes.

Design Choices

Symfony Mailer vs Symfony Transport

Previously a Swift_Mailer instance was provided to the laravel Mailer instance. I didn't opt to instantiate a Symfony Mailer instance because in some situations you'd need to interact with the transport (like the array and log transport) and there's no getter for the transport on the Symfony Mailer instance. Since we don't use the Symfony logger or events functionality, I decided to pass the TransportInterface instead. You can think of Laravel's Mailer class to be a direct replacement for Symfony's Mailer class.

Docs

laravel/docs#7306

Copy link
Contributor

@michael-rubel michael-rubel left a comment

Choose a reason for hiding this comment

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

A lot of duplicates with $this->email->method.
Maybe it's better to create a separate method with passing cc, bcc, etc?

@driesvints
Copy link
Member Author

A lot of duplicates with $this->email->method.

@michael-rubel it's fine as is I think.

@driesvints driesvints force-pushed the symfony-mailer branch 3 times, most recently from b5895d8 to 4bc725b Compare September 2, 2021 12:47
composer.json Outdated Show resolved Hide resolved
@taylorotwell
Copy link
Member

taylorotwell commented Sep 3, 2021

I don't think we can remove this: The Postmark X-PM-Message-Stream header won't be set anymore (see [8.x] Make it possible to set Postmark Message Stream ID #35755)

If the Symfony version of the Postmark driver doesn't support it we should probably just write our own Postmark transport for Symfony Mailer.

@Jubeki
Copy link
Contributor

Jubeki commented Sep 3, 2021

I don't think we can remove this: The Postmark X-PM-Message-Stream header won't be set anymore (see [8.x] Make it possible to set Postmark Message Stream ID #35755)

If the Symfony version of the Postmark driver doesn't support it we should probably just write our own Postmark transport for Symfony Mailer.

You probably would only need to change the getPayload function for the Postmark(API) Transport.
https://github.com/symfony/postmark-mailer/blob/bd270db549b314e60da8991485137d68dbbd68de/Transport/PostmarkApiTransport.php#L77-L116

There you would need to add the key MessageStream (https://postmarkapp.com/developer/api/email-api)

If you were using the Postmark SMTP Transport it would still be possible to use the header like before. (https://postmarkapp.com/developer/user-guide/send-email-with-smtp in the Section Connection-Details)

Edit: Maybe doing a PR to Symfony would be an idea.

@Jubeki
Copy link
Contributor

Jubeki commented Sep 6, 2021

@driesvints Regarding the Postmark X-PM-Message-Stream header.

The current Swift Mailer Implementation of the Postmark Transport also works with the API. There it also just attaches the Header to the request and it seems to be working fine right now.
Because Symfony Postmark Mailer works the same way as the Swift Version. The headers are attached to the request and it may just work out of the box.
I can't verify this because I never worked with Postmark before and don't have an account there.

Edit: I also can't find any documentation regarding this. This could change anytime if Postmark would want to.

@driesvints
Copy link
Member Author

@Jubeki thanks for all your investigation. I did some of my own and it turns out that Message Streams are a documented feature after all. I've sent a PR to Symfony to provide support for it and all the details are in the PR: symfony/symfony#42941

@Jubeki
Copy link
Contributor

Jubeki commented Sep 8, 2021

@driesvints yeah it is documented for sure. What I meant was that the current implementation (Laravel 8.x) uses the X-PM-Message-Stream header together with the Postmark API. I only found documentation for the Header in the SMTP Section of Postmark and not in the API Section. So I was just wondering how the current Implementation is even working as intended.

@driesvints
Copy link
Member Author

@Jubeki I guess Postmark both accepts a X-PM-Message-Stream header and a MessageStream value set on its payload for the API endpoint. Either will set the Message Stream I suspect.

@Jubeki
Copy link
Contributor

Jubeki commented Sep 8, 2021

Also maybe another concern which should be addressed:

What if somebody sets the X-PM-Message-Stream header on a email instance. Which value will be choosen?
In Laravel 8.x the header would overwrite the default.

With the new implementation in 9.x there is both the header and the json entry present. Which has a higher priority?
Maybe the header should be transformed into a MessageStreamHeader or be filtered out to overwrite the default?

taylorotwell and others added 3 commits September 13, 2021 17:04
Co-authored-by: Julius Kiekbusch <jubeki99@gmail.com>
* Create SentMessage wrapper for Symfony's SentMessage

* Wrap Symfony SentMessage

* Update Docblocks to Illuminate\Mail\SentMessage

* Fix sendMailable

* Update SentMessage.php

Co-authored-by: Dries Vints <dries@vints.io>
@driesvints
Copy link
Member Author

@taylorotwell @Jubeki has added the SentMessage instance. I suggest adding the new methods to it later on in a separate PR so we don't let this PR become too large.

@taylorotwell taylorotwell merged commit 097107a into master Sep 14, 2021
@taylorotwell taylorotwell deleted the symfony-mailer branch September 14, 2021 13:57
@taylorotwell
Copy link
Member

@Jubeki thanks for all your help with this! ❤️

@Jubeki
Copy link
Contributor

Jubeki commented Sep 14, 2021

I also have to say thanks to you @taylorotwell and @driesvints. You are doing really great work with Laravel.

Learning by doing is the best way to learn for me. Even more so if you don't want to work on your bachelor thesis or learn for your exams. 😂

@driesvints
Copy link
Member Author

Thanks a lot for all your help @Jubeki!

LukeTowers added a commit to wintercms/storm that referenced this pull request Dec 9, 2021
@dyanakiev
Copy link

dyanakiev commented Jan 24, 2022

@driesvints hey, i think there could be issue with this.
Im simple doing:

Mail::to(['me@gmail.com'])->send(new test());

and i get this error: Object of class Symfony\Component\Mime\Address could not be converted to string on that line above.
Im using "laravel/framework": "^9.0", i also tried multiple different ways and the issue is always the same, alsso theres no way to specify recipient name?
This should be working as per the docs.

I also tried from symfony docs:

        Mail::to(new Address('me@gmail.com'))->send(new test());

But then i get Undefined property: Symfony\Component\Mime\Address::$email

@Jubeki
Copy link
Contributor

Jubeki commented Jan 24, 2022

@dyanakiev I just tried your example on a fresh Laravel 9 App and it seems to be working fine. Can you please provide a repository?

If you have GitHub CLI:

laravel new bug-symfony-mailer --dev --github="--public"

If you don't have GitHub CLI:

laravel new bug-symfony-mailer --dev
git init
git add .
git commit -m "Setup Laravel Project"

Then add your changes and commit them.

You can also take a look at: https://github.com/Jubeki/bug-symfony-mailer

composer install
cp .env.example .env
php artisan key:generate
php artisan mail

Then take a look in storage/logs/laravel.log

@dyanakiev
Copy link

dyanakiev commented Jan 24, 2022

@Jubeki thanks, i just tried with your repo and theres no issue, im really confused about what is happening..

Sorry i found the issue i forgot to look at the stacktrace.. it was clockwork fault...

@mohdkasim01911
Copy link

Method Illuminate\Mail\Mailer::getSwiftMailer does not exist.

i have error please give me solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants