Sylius / Sylius

Open Source eCommerce Framework on Symfony
https://sylius.com
MIT License
7.9k stars 2.09k forks source link

Better transactional email infrastructure #9632

Closed gabiudrescu closed 4 years ago

gabiudrescu commented 6 years ago

Describe the proposed solution Currently, the transactional emails sent from Sylius suffer for two major issues:

I would approach this by implementing translation keys in those template as well, keeping in mind that some of the emails might be sent in a CLI environment and this requires special attention.

As for design, I would propose using MJML to implement a basic templating for emails, making them look more appealing and human.

for those who don't know, MJML is a mark-up language that compiles to HTML that plays nice with email clients. if you ever worked with email HTML, you know it's a painful experience.

MJML comes to the rescue and helps you avoid such issues. Read more here: https://mjml.io/

There are several ways we can do this implementation:

  1. Include MJML as a dependency in package.json and compile one time only, on build time, from .mjml to html; this has the downside that you need adapt MJML to play nice with Twig.

  2. Include this bundle and rewrite files such as src/Sylius/Bundle/ShopBundle/Resources/views/Email/passwordReset.html.twig to MJML and have them compiled on runtime, when the email is dispatch; the downside is that the compilation takes between 1-2 seconds, depending on the server and you don't want to do that on the request.

  3. implement 2nd option together with this idea and have emails dispatch in an async process.

my vote goes for option 3.

Describe alternatives you've considered We could leave them as they are and let Sylius user to provide their own implementation, from scratch. Also this can be implemented as a plugin, too, I guess. Haven't put much thought in it.

pamil commented 6 years ago

Sounds good, how could we provide a BC layer if someone has already overwritten an email template?

gabiudrescu commented 6 years ago

@pamil I guess that, for example with this file, if we overwrite what's inside body block, it kinda doesn't matter if someone extended or also overwrote the same template or block.

right?

11mb commented 6 years ago

Is it possible to create this functionality that is easy to replace with another email provider? E.g. we are currently investigating to send all transactional mails to an external service like Campaign Monitor. In that case I don't want Sylius to email at all, only send events to such an external service.

gabiudrescu commented 6 years ago

Campaign monitor, Amazon SES or Mandrill are all email service providers that abstract the SMTP infrastructure behind a simple set of credentials.

Basically, you can use even Microsoft Outlook to send emails through that SMTP gateway.

another service they all provide is generally sending emails via HTTP API Calls. you have an endpoint where you usually send a POST request with a payload that is transformed by their service into an email.

@11mb I assume this is what you are talking about.

as far as I understand Sylius, if you replace this class with a custom one that implements the logic of making an API call to Campaign Monitor, you're good to go.

my idea brought under discussion in this issue is not about this. it's about what happens before the payload reaches the Sender.

it's about how can we improve the look of Sylius transactional emails and recommend a good practice of creating responsive transactional emails without having to worry about compatibility between weird HTML rendering practices by various email clients.

11mb commented 6 years ago

@gabiudrescu, you are absolutely correct! Thanks for clarifying my point. I support your case for better responsive transactional emails as well.

The reason I suggest this point is that it may be an extra reason to set things up differently. Instead of Sylius first setting up an email, and then sending them via the Sender class, I suggest:

  1. Event occurs (customer placed order)
  2. Event triggers EmailSender (with payload with data) <--- no rendering of email yet

3a. Sylius default behaviour: set up an email with template, and send via swiftMailer

3b. Overwrite default behaviour: send event with payload to external email service provider.

In the case of 3a: Your proposal still stands and this could be improved indeed in regards of responsiveness (e.g. with the help of MJML).

In case of 3b: I was thinking of using MJML in combination with an external provider (so HTML template is managed by this external provider). In this way I can separate the responsibilities better.

gabiudrescu commented 6 years ago

@11mb I agree with your proposal on how emails should work on Sylius - it is flexible enough to cover almost all needs.

wondering on how to proceed next. cc @CoderMaggie

bruno-ds commented 5 years ago

I contribute with a more low-brow point of view: This is how I decorated the EmailTwigAdapter in order to add a layout. maybe this could be made native easily. styling email is still hard, had having some boilerplate would be great but maybe it's not the role of sylius to go this further in the implementation...

app.email_renderer.adapter.twig:
    class: AppBundle\EmailManager\Renderer\EmailTwigAdapter
    decorates: sylius.email_renderer.adapter.twig
    arguments: ['@app.email_renderer.adapter.twig.inner', "AppBundle:Email:_layout.html.twig", '@twig']
<?php
/**
 * Created by Bruno DA SILVA, working for Combodo
 * Date: 30/10/18
 * Time: 14:59
 */

namespace AppBundle\EmailManager\Renderer;

use Sylius\Component\Mailer\Model\EmailInterface;
use Sylius\Component\Mailer\Renderer\Adapter\AdapterInterface;
use Sylius\Component\Mailer\Renderer\RenderedEmail;

class EmailTwigAdapter implements AdapterInterface
{
    /**
     * @var \Twig_Environment
     */
    protected $twig;
    /**
     * @var AdapterInterface
     */
    private $decoratedAdapter;
    /**
     * @var string
     */
    private $layout;

    /**
     * @param \Twig_Environment $twig
     */
    public function __construct(AdapterInterface $decoratedAdapter, string $layout, \Twig_Environment $twig)
    {
        $this->decoratedAdapter = $decoratedAdapter;
        $this->layout = $layout;
        $this->twig = $twig;
    }

    /**
     * {@inheritdoc}
     */
    public function render(EmailInterface $email, array $data = []): RenderedEmail
    {
        $renderedEmail = $this->decoratedAdapter->render($email, $data);

        $messageWithLayout = $this->twig->render($this->layout, ['message' => $renderedEmail->getBody()]);
        $renderedEmail->setBody($messageWithLayout);

        return $renderedEmail;
    }
}
Roshyo commented 5 years ago

Just a point about MJML. I believe Sylius does not have to force any markup language other than HTML. Having an implementation of it would be cool for those who use it, but keeping a native way of rendering mails in HTML is mandatory in my opinion.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

pamil commented 4 years ago

After a conversation with @gabiudrescu last month, we decided not to go with MJML as it'd require us to have Node.js installed on the application server and it'd add unnecessary complexity. However, we've polished email infrastructure and now all emails have a default design (#10893), are aware of the channel (#10894) and are translatable (#10914).