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

New email service based on Symfony Mailer #12096

Closed
4 tasks
escopecz opened this issue Mar 20, 2023 · 38 comments
Closed
4 tasks

New email service based on Symfony Mailer #12096

escopecz opened this issue Mar 20, 2023 · 38 comments
Labels
stale Issues which have not received an update within 90 days

Comments

@escopecz
Copy link
Sponsor Member

escopecz commented Mar 20, 2023

As the SwiftMailer library is dead and we must replace it with Symfony Mailer then this is a great opportunity to refactor the email sending in Mautic. This crucial part of Mautic has evolved in the years and each new feature added new if in the existing implementation. So the maintenance of this code is very costly and every change in the code can break some unrelated transport for example. The transport that support toketinzation (sending more than 1 email at once via API) use different code than SMTP-based transports. So debugging is complicated.

Symfony Mailer can send only one email per request and this won't change in the future. See symfony/symfony#32991

There is WIP PR which removes SwiftMailer with Symfony Mailer: #11613

I suggest to improve this PR with more maintainable solution when we are forced to refactor it anyway.

What the system must support:

  • Sending emails in batches by default. This is not considered in Symfony mailer at all. Each mailer sends one email at a time even if the API-based mailers can send 10k emails in 1 request. This is crucial to support for fast email sending that Mautic users are used to. Imagine you can send 1 email/second or 10k emails/second difference. These mailers will have to be created by our community.
  • Ideally support the pre-made Symfony mailers.
  • At least prepare the possibility to have different mailer for marketing and another for transactional emails.
  • Support bounce callbacks.

An iterable collection of recipients that can have 1 or several thousands or records.

interface RecipientsInterface extends Iterable
{
    public function add(Recipient $recipient): void;
}

DTO class that will hold the info we need to send a personalized email.

class Recipient
{
    public function getRecipietTokens(): array;
    public function getEmailAddress(): Address;
}

Wrapper DTO that will be used to send a batch email. Can be easily extended.

class BatchEmail
{
    public function getRecipients(): RecipientsInterface;
    public function getEmail(): Email;
    public function getGlobalTokens(): array;
}

A factory that can check whether it supports a DSN and if so, create the transport with it.

interface BatchTransportFactoryInterface
{
    public function supports(Dsn $dsn): bool;
    public function create(Dsn $dsn): BatchTransportInterface;
}

The batch transports will have to implement this interface to send batch (and/or single) emails

interface BatchTransportInterface
{
    public function send(BatchEmail $batchEmail): void;
}

Optional interface to handle bounces. It's not used anywhere in this example but will be implemented similarly like BatchTransportInterface for bounce requests.

interface TransportBounceInterface
{
    public function processCallbackRequest(RequestInterface $request): void;
}

Example of how we can use the classic Symfony transports that can use only 1 email per request.

class BasicTransport implements BatchTransportInterface
{
    private AnyExistingSymfonyTransport $transport
    public function __construct(AnyExistingSymfonyTransport $transport)
    {
        $this->transport = $transport;
    }

    public function send(BatchEmail $batchEmail): void
    {
        foreach ($batchEmail->getRecipients() as $recipient) {
            $batchEmail->getEmail()->to($recipient->getEmail();
            $this->replaceTokens($recipient->getRecipietTokens() + $batchEmail->getGlobalTokens(), $batchEmail->getEmail()->getHtml());
            $this->transport->send($email);
        }
    }
}

Example of how we can implement our own batch transports that can send thousands of emails per request.

class SparkpostTransport implements BatchTransportInterface
{
    private SparkpostClient $sparkPostClient
    public function __construct(SparkpostClient $sparkPostClient)
    {
        $this->sparkPostClient = $sparkPostClient;
    }

    public function send(BatchEmail $batchEmail): void
    {
        $sparkPostMessage = $this->makeSparkPostMessage($batchEmail);
        $sparkPostClient->transmissions->post($sparkPostMessage);
    }
}

A collector that will be filled on cache build with transports that are ready to send emails. There are examples of 4 methods of how to get some specific transport. We may have 4 fields for primary, backup, marketing, transport DSNs in the configuration. If some is not configured then the primary will be used.

class BatchTransportCollector
{
    private CoreParametersHelper $coreParametersHelper;
    private ContainerInterface $serviceLocator;
    private array $transports;
    
    public function __construct(CoreParametersHelper $coreParametersHelper, ContainerInterface $serviceLocator)
    {
        $this->coreParametersHelper = $coreParametersHelper;
        $this->serviceLocator = $serviceLocator;
    }

    // There can be various transports for various use cases:
    public function getPrimaryTransport(): BatchTransportInterface;
    public function getBackupTransport(): BatchTransportInterface;
    public function getMarketingTransport(): BatchTransportInterface;
    public function getTransactionalTransport(): BatchTransportInterface;

    private function init()
    {
        if (isset($this->transport)) {
            return;
        }

        $dsns = $this->coreParametersHelper->get('dsns'); // an array of DSN strings

        foreach ($this->serviceLocator->getProvidedServices() as $serviceId) {
            $transportFactory = $this->serviceLocator->get($serviceId);
            
            foreach ($dsns as $dsn) {
                if ($transportFactory->supports($dsn)) {
                    $this->transports[] = $transportFactory->create($dsn);
                }
            }
        }
    }
}

Autowiring the batch transports. Any new class implementing the BatchTransportInterface will be automatically tagged in app/bundles/CoreBundles/Config/services.php:

$services->instanceof(\Mautic\EmailBundle\Mailer\BatchTransportFactoryInterface::class)->tag('batch.transport.factory');

Then in a new BatchTransportCompilerPass.php we'll do something like

class BatchTransportCompilerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $containerBuilder)
    {
        if (!$containerBuilder->hasDefinition(BatchTransportCollector::class)) {
            return;
        }

        $configuratorDef = $containerBuilder->findDefinition(BatchTransportCollector::class);
        $locateableServices = [];

        foreach ($containerBuilder->findTaggedServiceIds('batch.transport.factory') as $id => $tags) {
            $locateableServices[$id] = new Reference($id);
        }

        $configuratorDef->addArgument(ServiceLocatorTagPass::register($containerBuilder, $locateableServices));
    }
}

Example usage of how to send a batch/segment email:

$symfonyEmail = (new Email())
    ->from($this->getFrom()) // Similar for cc, bcc. To will be empty here.
    ->subject($mauticEmail->getSubject())
    ->html($mauticEmail->getCustomHtml());

$recipients = new Recipients();
foreach ($contacts as $contact) {
    $recipients->add(new Recipient($contact->getFields(true), $contact->getEmail()));
}

$bathEmail = new BatchEmail($symfonyEmail, $recipients, $this->getGlobalTokens());
$batchTransportCollector->getPrimaryTransport()->send($batchEmail);

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@YosuCadilla
Copy link

I don't know if anything at all survived from DB's dream of using Microservices or even headless Mautic, but I think this is an excellent time to try to decouple email sending from the rest of Mautic.
Decoupling does not (necessarily) mean cutting email off of Mautic, it just means freedom to implement it as a standalone service that could (or could not) be part of Mautic, but not anymore a part of the main monolithic application, hereafter it could then be built and maintained independently.
This would not only facilitate the change to the new, non-SM paradigm, it would also help with the Mautic streamlining process recommended by Acquia and would also open the door to optionally use other languages and frameworks that might be better suited for the task of sending email...

@escopecz
Copy link
Sponsor Member Author

Thanks for the feedback, Yosu! Queuing the emails will be the second part of the email refactoring. Once the emails are in a queue then you can use any microservice to process it. @mabumusa1 implemented the queue into in 1 PR before but it was too big to review and test and so he split it into replacing the Swiftmailer library first and the Symfony Mailer implementation comes right after that.

@YosuCadilla
Copy link

John @escopecz , thanks for the extended explanation. If it is faster, more reliable and easier to extend, you are doing a great job indeed.

I'm just sorry I missed the entire discussion about email refactoring that clearly happens somewhere else...
@RCheesley I just wish these important decisions could be discussed, by the community, in a planning phase instead of letting us know about it after the fact.

@YouBuiltThat
Copy link

In this case the core functionality of the system from my perspective is about sending messages. Not explicitly emails.

Though emails is the primary channel most of us use it seems like it would be better to create an interface that abstracts away from implementation of email specifically.

I know this would be a massive shift but after it the interface should be very stable.

@escopecz
Copy link
Sponsor Member Author

@YosuCadilla I would expect that you would be more upset about the dead dependency that needs replacing than the fact that the community is actively working on the replacement. It was discussed in:

It all happens in open forums where anyone can join and comment or suggest a better solution. In fact, notice that there is call for reviewers and testers every Friday and sometimes even more times per week. The SwiftMailer replacement is still an open PR. There is nothing set in stone. Please join the reviewers and testers with your expertise. We really need more active testers and reviewers to move faster.

@YouBuiltThat Good point! Do you know of another channel that can use batch messages other than some email service providers? I don't want to refactor all the channels at this point but we can definitely prepare the interfaces to be used by other channels too.

@kuzmany
Copy link
Member

kuzmany commented Apr 20, 2023

@escopecz nice conception.
We need times ago send thousands of SMS in one hour, then we add threads support for SMS #11549 and we were able to do it

Batch support for it would be great for SMS, but let's figure out emails in the first phase.

@YosuCadilla
Copy link

YosuCadilla commented May 2, 2023

Thank you @escopecz, I see you publish in several important spaces.

What I'm trying to convey, is that certain topics could/should be presented to a broader audience, these spaces you listed, are mostly frequented by programmers, hence, a marketer, a data scientist, even a Devops, will probably not be reading those messages that you posted.

That is why, I believe that in cases where a decision will impact what these kinds of professionals do for a living, It might be a good idea to let them know, or even ask them for input, for the good of the community and for a better quality of the software product we are building together, Mautic.

The more people participating, the better the end result (If we can get to a solution, of course).

@escopecz
Copy link
Sponsor Member Author

escopecz commented May 3, 2023

@YosuCadilla I completely agree with you. That's why I created this issue. Please suggest:

  1. what other channels to use?
  2. what is the rule of thumb that a community member can use to tell which change deserves more attention than others?
  3. can you help to spread the word about this particular change in the channels you think have been missed?

@YosuCadilla
Copy link

@escopecz Sorry for the delay, I know this is urgent.

  1. What other channels to use?
    Working on this, as well as some processes for different cases/scenarios. More on my next post, right here.

  2. What is the rule of thumb that a community member can use to tell which change deserves more attention than others?
    This is a very complicated one and I would not expect to have an answer right away.
    As a generic rule of thumb, a person should consider the impact of an action, improvement or change and communicate with those potentially affected, ideally before starting said changes or improvements.

  3. Can you help to spread the word about this particular change in the channels you think have been missed?
    Yes, I will be helping with defining the processes (guidelines?) so anyone can use that as a reference in the future.
    Ideally, the processes will also include a "facilitator", a person who already knows the processes and how to make things easier and simpler for everyone involved.

@YosuCadilla
Copy link

Today I had a quick chat with @RCheesley on this topic, she already had advanced a lot in this direction, she shared with me some docs and broadened my point of view.

I'll be reading the existing content and meeting with Ruth again tomorrow.
It would be great if you and/or other developers could join that conversation happening tomorrow May 4 at 16:00 UTC

@escopecz
Copy link
Sponsor Member Author

escopecz commented May 4, 2023

Where do we meet?

@mabumusa1
Copy link
Member

I would love to join as well, please ping me

@YosuCadilla
Copy link

Sorry @escopecz I totally missed your msg here.
@mabumusa1 joined and had very interesting proposals, mostly in the realm of new developer's mentorship.

On the topic of facilitating inter-silos communications, I basically explained what I had in mind to Ruth and then we discussed possibilities.

If you have the time, I would be delighted to meet with you @escopecz and with any other interested developers, explain the current concept we arrived to, and then we can discuss the developer's side of it, so you can show me how it would work best on the devs side?

@escopecz
Copy link
Sponsor Member Author

escopecz commented May 5, 2023

Let's schedule a call with fixed time and place so everyone interested can join. Perhaps announce it in the #t-product and #dev channels in Slack where most active devs hang out? I was looking for some link to join the call yesterday but did not find any here nor in Slack. @mabumusa1 is better detective than I am :)

@YosuCadilla
Copy link

YosuCadilla commented May 5, 2023

@escopecz Yes, let's do that, what about next Wednesday? (so we have time to communicate first).
I'm also going to propose 18:00 UTC, feel free to propose other days and times, of course.
I can do the announcements, after we agree on date/time. Let me know if you want to do them yourself (might be more effective this way?)

@escopecz
Copy link
Sponsor Member Author

escopecz commented May 9, 2023

Apologies. I was off on Monday due to state holiday. Wednesday is tomorrow. I'll move this discussion to Slack, the #t-product channel as it's not related to this issue.

Edit: Here is the Slack discussion: https://mautic.slack.com/archives/CQMKV0RU1/p1683629679843659

@k040711
Copy link

k040711 commented May 16, 2023

Is there a simple way to get mautic to send emails as HTTP post? I used the mailgun plugin for a while but now i'm using a different mail server which offers sending my API ( http post )

I looked around, all other plugins / addons always add some extra layers that I can't easily modify,

Is there a simple script I can use with Mautic where I can define the post URL, and fields to be sent?

tnx

@escopecz
Copy link
Sponsor Member Author

@k040711 sadly, there is no such simple option. Each email service provider has different way how to authenticate and different API. So each must have its own plugin that implements that.

@k040711
Copy link

k040711 commented May 16, 2023

Is there a plugin for Postal Mail Server's API?, I've looked around but did not find one

@escopecz
Copy link
Sponsor Member Author

I don't see such library for Symfony Mailer, no. It would have to be created from scratch.

@YosuCadilla
Copy link

Hello @escopecz

Not sure to what extend the changes in the email service would affect the following or should be taking into consideration the following:

Also, as reference, this was found on Github:
#10428

And this is on the forums:
https://forum.mautic.org/t/multiple-smtp-settings-for-different-mailing-segments/18446/17

Only tangentially related, but could be added to the development:
https://forum.mautic.org/t/dnc-transactional-emails/21619/7

And this could be the solution?
https://github.com/1FF/mautic-custom-email-settings

Or maybe it is the perfect time for the entire mailing system to be reconsidered?
#12096

@mautibot
Copy link

This issue has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/dnc-transactional-emails/21619/10

@mautibot
Copy link

This issue has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/multiple-smtp-settings-for-different-mailing-segments/18446/18

@escopecz
Copy link
Sponsor Member Author

@YosuCadilla thanks for the links!

The first one is already solved I think.

In this proposal we plan to add the option for multiple email transport configuration. It's this point in the description:

At least prepare the possibility to have different mailer for marketing and another for transactional emails.

The main goal is to switch to the new library but also open doors for the multiple transport feature to be implemented across the platform.

The plugin looks great. It is a bit cumbersome to configure as it must fit in the existing Mautic configuration. So we can simplify that. And it is still based on Swiftmailer so it does not solve the main reason we need to refactor the email transport layer.

@YosuCadilla
Copy link

OK, how can I help "to have different mailer for marketing and another for transactional emails."
If possible for Mautic 4 as well as 5.

@escopecz
Copy link
Sponsor Member Author

There will be no new features coming into Mautic 4.

I'm not sure how you can help other than motivate or pay a developer to start working on this.

@kuzmany
Copy link
Member

kuzmany commented May 25, 2023

@escopecz Probably we could start calculating approx hours for the tasks. People need to see how difficult it is...

@escopecz
Copy link
Sponsor Member Author

escopecz commented May 25, 2023

@kuzmany good idea. I'm currently in the middle of https://mautic.atlassian.net/browse/TPROD-428 and I'd like to finish it before starting something different. If anyone wants to start a project for this and split it into tickets, please go ahead. It would be appreciated. This is going to be one big blocker for the M5 release.

@YosuCadilla
Copy link

YosuCadilla commented May 25, 2023

Just a bit of (healthy?) copy & paste... sorry if you had already read this:

What about implementing it in a way that the same "component" can be used by both M4 or M5 (or M6?).
In a microservices fashion, where it makes sense... maybe as an "email delivery API" or maybe gRPC?
Distributed monoliths (whatever that means) are the new thing, give it a try?

I mean, we need to get started with distributed architecture sooner rather than later, and email is like the cornerstone of Mautic, and there's so much that marketers need for Mautic email to be on par with other MA tools out there...
I really see the need for the new email service as a golden opportunity to gather the community around and start working together on this.

@YosuCadilla
Copy link

It seems to me the solution currently proposed does a good job to further the current way email works in Mautic, but we could use this opportunity to improve it, both technically and functionally, among other things, by discussing the requirements with the community and even more specifically with who sends the freaking emails, the marketers!

I understand would be harder and would take extra time, but it is not like we have a deadline for M5... or do we?

@mautibot
Copy link

This issue has been mentioned on Mautic Community Forums. There might be relevant details there:

https://forum.mautic.org/t/dnc-transactional-emails/21619/12

@escopecz
Copy link
Sponsor Member Author

@YosuCadilla I'll copy-paste the answer then:

Feel free to propose such solution in more detail so the community can comment on it.

Yes, there is a deadline. And it is in the past. M4 is running on dependencies with known security vulnerabilities. PHP 8.0 will stop getting security patches later this year. But that's not a discussion I want to have on this ticket. Please start your proposal so it can be reviewed. There's also going to be Mautic 6 in the future so you can start building the email micro service for M4, M5 and then it can be baked into M6.

@YosuCadilla
Copy link

The deadline is whatever we make it to be, it was in the past and now it is in the future, basically undefined, and it could remain undefined, the world did not end when we missed the deadlines.
I am plenty confident that if waiting a bit longer means a much better way of dealing with email delivery, most of the community will be OK with it.

Personally, I think it should rather be baked into M4, and fixing those vulnerabilities on M4 should be dealt with in M4, not pushed forward to M5.
M5 won't be usable in production until, lets say, after a year of the first release. Which means pushing vulnerability fixes to M5, will leave everyone either eating bugs for 1 year or with vulnerabilities for 18 months.
Not a good prospect!

@YosuCadilla
Copy link

I do not know who made those decisions nor why, but we need to start thinking differently...
Who exactly needs M5 so badly that we cannot make M4 better first?

It does not make any sense to produce yet another buggy version of Mautic when we can polish the current one which already works pretty damn well!

Mautic is known to be buggy and difficult precisely because of that way of "solving" problems, we really need to start turning around this image, and we do that by prioritizing code quality over new versions.

@escopecz
Copy link
Sponsor Member Author

Solving the security vulnerabilities comes with beckward compatibility breaks. Are you saying we should not follow Semantic versioning?

@YosuCadilla
Copy link

YosuCadilla commented May 30, 2023

I have no idea what you follow or don't follow, it is hard to keep track really, maybe whatever versioning system you were using when you added those security fixes in the middle of major releases?
Whatever that was... use that!!!
https://github.com/mautic/mautic/releases/tag/4.3.0
https://github.com/mautic/mautic/releases/tag/3.3.5
https://github.com/mautic/mautic/releases/tag/3.3.4
https://github.com/mautic/mautic/releases/tag/3.3.2
https://github.com/mautic/mautic/releases/tag/3.2.4
https://github.com/mautic/mautic/releases/tag/3.2.3
https://github.com/mautic/mautic/releases/tag/2.16.5
Anyhow, as long as you don't push too much shit under the rug and you don't remove too many features the community actually needs, we'll be fine!

@stale
Copy link

stale bot commented Sep 16, 2023

This issue or PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you would like to keep it open please let us know by replying and confirming that this is still relevant to the latest version of Mautic and we will try to get to it as soon as we can. Thank you for your contributions.

@stale stale bot added the stale Issues which have not received an update within 90 days label Sep 16, 2023
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If this issue is continuing with the lastest stable version of Mautic, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues which have not received an update within 90 days
Projects
None yet
Development

No branches or pull requests

7 participants