Skip to content

[Messenger][Mailer] Send mails after the main message succeeded #34291

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Messenger][Mailer] Send mails after the main message succeeded #34291

wants to merge 1 commit into from

Conversation

ogizanagi
Copy link
Contributor

@ogizanagi ogizanagi commented Nov 8, 2019 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR TODO

Description

When using the Mailer component with a message bus, given the following handler:

class FooCommandHandler
{
    private $mailer;

    public function __construct(MailerInterface $mailer)
    {
        $this->mailer = $mailer;
    }

    public function __invoke(FooCommand $command): void
    {
        $this->mailer->send((new Email())
            ->to('foo@example.com')
            ->subject('foo')
            ->text('Hello')
        );

        throw new \RuntimeException('Foo');
    }
}

the mail is sent despite the handler failed (because of the exception). Same would go with an async email, as it'll still send the SendEmailMessage to the transport, independently from the main handler failure or success.

In a real app, this means a mail can be sent for a treatment that has actually failed (for instance, because the Doctrine middleware failed to flush the changes made in the handler).

Possible solution

Rely on the DispatchAfterCurrentBusMiddleware and the DispatchAfterCurrentBusStamp stamp.
This stamps allows to only handle the stamped message if and only if the handler in which it was dispatched succeeded.

A new SendMailAfterCurrentBusMiddleware middleware can automatically add the stamp to SendEmailMessage when necessary.
But this middleware must be registered before DispatchAfterCurrentBusMiddleware to work (which make it difficult to register in userland, as you need to disable default middleware to prepend it before).

  1. Do we want this in core?
  2. Should it be registered by default?
    Right now, I don't see much drawbacks.
  3. Should it be a new default_middleware value? (allowing an array, i.e: accept something like ['allow_no_handlers', 'send_mail_after_current_bus'].
  4. This middleware will add the stamp to all mails. Do we need a more fine grained control?

@ogizanagi ogizanagi added this to the next milestone Nov 8, 2019
@ogizanagi ogizanagi changed the base branch from 4.4 to master November 19, 2019 16:02
@ogizanagi ogizanagi marked this pull request as ready for review November 19, 2019 16:02
@Nyholm
Copy link
Member

Nyholm commented Jan 19, 2020

I agree with this implementation. But Im not sure (or aware) if this is the correct solution to the problem.

I think the main issue is that we dont have much control over how the mails are sent. (Ie a messages put on the bus). Could an alternative solution give the user control over this, so they can configure what stamp or what bus to use?

@ogizanagi
Copy link
Contributor Author

ogizanagi commented Jan 20, 2020 •
edited
Loading

Not sure it'll answer all your doubts, but at least since #34648, we can now configure the message bus to use to send mails.
About configuring stamps, I think the usage of the message bus to send mails was made opaque on purpose in the main API. Any specificity can be configured at the global level using config and/or middleware. We could imagine a way to forward stamps to the SendEmailMessage message on caller's side, but for now I'm not aware of use-cases appart the one in this PR, which I think is better solved at the global level.

@weaverryan
Copy link
Member

Is there any reason to not add the DispatchAfterCurrentBusStamp stamp directly in Mailer instead of via a middleware? I'm guessing I'm missing something. It looks like Mailer is dispatching SendEmailMessage and then we have a middleware that always looks for SendEmailMessage and adds the stamp. Could that be simpler?

@ogizanagi
Copy link
Contributor Author

ogizanagi commented Oct 1, 2020 •
edited
Loading

@weaverryan : Previous to #37976, we had to know whether or not we already are dispatching or not. It could now be simplified.

About adding it directly from the Mailer component, I guess it needs the same considerations as the end of the PR description, aka, should it be enabled by default for all mails?

@weaverryan
Copy link
Member

About adding it directly from the Mailer component, I guess it needs the same considerations as the end of the PR description, aka, should it be enabled by default for all mails?

With the current implementation of this PR, assuming you're using FrameworkBundle, then we will now ALWAYS add the stamp. So that seems barely different than adding it always inside of Mailer.

Is there any practical downside to adding the stamp always (either via a middleware like in this PR or in Mailer)?

@ogizanagi
Copy link
Contributor Author

With the current implementation of this PR, assuming you're using FrameworkBundle, then we will now ALWAYS add the stamp. So that seems barely different than adding it always inside of Mailer.

That's what I meant, the same considerations apply 😄
The fact it was designed as a middleware at first was mostly driven by the exception thrown otherwise before #37976.

Is there any practical downside to adding the stamp always

If for whatever reason the mail should still be sent whenever the handler failed.

@weaverryan
Copy link
Member

If for whatever reason the mail should still be sent whenever the handler failed.

In that case, shouldn't this be something you can control on a case-by-case basis? And can't we "already" do that by putting your email logic at the end of the handler? I might be over-simplifying :)

@ogizanagi
Copy link
Contributor Author

And can't we "already" do that by putting your email logic at the end of the handler?

Nope, as something else after the handler can fail, e.g: a Doctrine transaction.
So, for instance, in the case of a user registration, if the transaction failed, your user will receive a confirmation mail while not being actually registered, no matter where the mail was sent in the handler as the transaction failed in the doctrine middleware, after the handler was executed.

@weaverryan
Copy link
Member

Ok, fair enough.

If for whatever reason the mail should still be sent whenever the handler failed.

But doesn't the point still stand that the user might want to control this behavior?

@fabpot fabpot closed this Oct 7, 2020
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

@ogizanagi
Copy link
Contributor Author

ogizanagi commented Oct 7, 2020 •
edited
Loading

JFYI, I'm not reopening this one as it needs work according to the latest merges and discussions about it. Reopening it would only pollute.
I'm gonna reopen another one when I have some time to work on it :)

(if anyone is willing to work on it till there, though, feel free of course!)

@ogizanagi ogizanagi deleted the send_mail_after_current_bus branch October 7, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature for 5.x Mailer Messenger Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@ogizanagi @Nyholm @weaverryan @fabpot @nicolas-grekas @carsonbot