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

[Mailer] Return SentMessage from Mailer::send if available #38517

Closed
wants to merge 2 commits into from

Conversation

pableu
Copy link
Contributor

@pableu pableu commented Oct 11, 2020

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets #35670
License MIT
Doc PR Coming Soon :-)

Since #36424, you can get the SentMessage if sending was handled by the messenger. But there's no easy and developer friendly way to do it without messenger.

This has been discussed a few times already (threads in #35670, #33681, #35670), but none of these discussions have resulted in a solution with good DX.

The SentMessage helps you get the Messge-ID, which some transports (such as Amazon SES) set by themselves. That might be useful as proof-of-delivery or when building something like a ticketing system (for matching replies using In-Reply-To and References headers).

I think returning SentMessage here is dev-friendly and pragmatic: If we have it, we return it. If we don't have it, we return null.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. This PR cannot be merge since it contains a BC break.

Just a question, why don't you use TransportInterface directly in your code?

@@ -26,5 +26,5 @@ interface MailerInterface
/**
* @throws TransportExceptionInterface
*/
public function send(RawMessage $message, Envelope $envelope = null): void;
public function send(RawMessage $message, Envelope $envelope = null): ?SentMessage;
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break and must be reverted. :/

@pableu
Copy link
Contributor Author

pableu commented Oct 11, 2020

I was afraid that this is a BC break. Could we target that for Symfony 6? :)

Could I add an additional method to MailerInterface? Like.. MailerInterface::getLastSentMessage? That would work for me but doesn't feel right.

why don't you use TransportInterface directly in your code?

That's a possible solution, but then I have to build more code around it than I'd like. I think returning the SentMessage if available would give a better DX.

@Nyholm
Copy link
Member

Nyholm commented Oct 11, 2020

Could I add an additional method to MailerInterface?

Im sorry, no. See https://symfony.com/doc/current/contributing/code/bc.html

why don't you use TransportInterface directly in your code?

That's a possible solution, but then I have to build more code around it than I'd like. I think returning the SentMessage if available would give a better DX.

I dont understand what you mean with "more code around it".

I assume that you have a class like this (that uses your patch):

class WelcomeEmailSender
{
    private MailerInterface $mailer;

    private function sendWelcome() {
        $message = new RawMessage(/**/);
        $sentMessage = $this->mailer->send($message);

        // Or store it in a log file. 
        echo $sentMessage->getMessageId();
    }
}

Isn't just to do this and you are all done:

-    private MailerInterface $mailer;
+    private TransportInterface $mailer;

@pableu
Copy link
Contributor Author

pableu commented Oct 11, 2020

Oh, you're right, I see! I didn't realize that I can also get the TransportInterface through DI. That works fine for my use-case.

Thanks a lot @Nyholm, sorry for abusing this PR for support.

@pableu pableu closed this Oct 11, 2020
@xabbuh xabbuh added this to the 5.x milestone Oct 12, 2020
@nicolas-grekas nicolas-grekas modified the milestones: 5.x, 5.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants