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

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

Open
wants to merge 1 commit into
base: master
from

Conversation

@ogizanagi
Copy link
Member

ogizanagi commented Nov 8, 2019

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

✏️ Opening the PR to discuss about the feature. The current code is more or like what I used in userland for now.

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 force-pushed the ogizanagi:send_mail_after_current_bus branch from 8c71fc3 to 6f2e464 Nov 8, 2019
@ogizanagi ogizanagi force-pushed the ogizanagi:send_mail_after_current_bus branch from 6f2e464 to 7622cea Nov 19, 2019
@ogizanagi ogizanagi changed the base branch from 4.4 to master Nov 19, 2019
@ogizanagi ogizanagi force-pushed the ogizanagi:send_mail_after_current_bus branch from 7622cea to 2a2b091 Nov 19, 2019
@ogizanagi ogizanagi marked this pull request as ready for review Nov 19, 2019
@ogizanagi ogizanagi force-pushed the ogizanagi:send_mail_after_current_bus branch from 2a2b091 to cc4c01b Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.