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] add reject to MessageEvent to stop sending mail #48409

Merged
merged 3 commits into from Dec 31, 2022
Merged

[Mailer] add reject to MessageEvent to stop sending mail #48409

merged 3 commits into from Dec 31, 2022

Conversation

y4roc
Copy link

@y4roc y4roc commented Nov 30, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#...

Add reject() in MessageEvent to reject sending mail

@carsonbot carsonbot added this to the 6.3 milestone Nov 30, 2022
@y4roc y4roc changed the title add reject to MessageEvent to stop sending mail [Mailer] add reject to MessageEvent to stop sending mail Nov 30, 2022
@y4roc y4roc requested a review from Koc December 1, 2022 08:38
@OskarStark OskarStark changed the title [Mailer] add reject to MessageEvent to stop sending mail [Mailer] add reject to MessageEvent to stop sending mail Dec 1, 2022
@y4roc y4roc requested review from fabpot and removed request for Koc December 1, 2022 22:39
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I think we should do the same in Mailer to avoid sending an email on the queue if we don't want to send it.
Also, can you some tests?

@carsonbot carsonbot changed the title [Mailer] add reject to MessageEvent to stop sending mail add reject to MessageEvent to stop sending mail Dec 2, 2022
@carsonbot carsonbot changed the title add reject to MessageEvent to stop sending mail [Mailer] add reject to MessageEvent to stop sending mail Dec 2, 2022
@VincentLanglet
Copy link
Contributor

Hi @y4roc, do you have time to handle the request change ? I'm really interested by this feature. :)

@fabpot
Copy link
Member

fabpot commented Dec 29, 2022

@VincentLanglet Feel free to take over if you have time to finish this PR.

@VincentLanglet
Copy link
Contributor

@VincentLanglet Feel free to take over if you have time to finish this PR.

Hi @fabpot, sure I'll be happy to do it.
But I took a new look and what is missing in this PR ?

I think your review (#48409 (comment)) is incorrect, since the method need to return a SentMessage so we need to add the check

if ($event->isRejected()) {

after the line

$sentMessage = new SentMessage($message, $envelope);

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Would be great to add some tests

@fabpot
Copy link
Member

fabpot commented Dec 31, 2022

@VincentLanglet Feel free to take over if you have time to finish this PR.

Hi @fabpot, sure I'll be happy to do it. But I took a new look and what is missing in this PR ?

I think your review (#48409 (comment)) is incorrect, since the method need to return a SentMessage so we need to add the check

if ($event->isRejected()) {

after the line

$sentMessage = new SentMessage($message, $envelope);

You're right, so we are only missing some tests.

@fabpot
Copy link
Member

fabpot commented Dec 31, 2022

@VincentLanglet I'm going to take over.

@derrabus
Copy link
Member

Should rejecting the mail stop the propagation of the MessageEvent? Currently, it doesn't and I thought we should at least have talked about it.

@fabpot
Copy link
Member

fabpot commented Dec 31, 2022

Should rejecting the mail stop the propagation of the MessageEvent? Currently, it doesn't and I thought we should at least have talked about it.

That's a good question. My take here is that you might have another listener interested in knowing the fact that the email has been rejected. It looks more flexible that way.

@derrabus
Copy link
Member

Right, but that listener would need to operate on a lower priority than any listener that might potentially reject messages. 🤔

Does rejecting the mail trigger a FailedMessageEvent?

  • If yes, I'd probably attach my listener there or I might already have one here.
  • If no: should it?

@fabpot
Copy link
Member

fabpot commented Dec 31, 2022

I would not trigger a FailedMessageEvent as the message didn't fail, it was just rejected from being sent.

As you don't necessarily know all listeners that can be registered in advance, maybe stopping the propagation is the more flexible way after all :) I've added another commit for that change. Let me know what you think.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Looks good.

One last question: I think, a rejected message should appear in the logs. Right now, it would be the responsibility of the listener to do that, right? Since the listener should know best why it rejected a message, that's probably the best place to write this kind of log entry.

Should the Mailer add a generic log entry about the rejection, maybe on debug level, to make the rejection discoverable in case the rejecting listener "forgets" to do so?

@fabpot
Copy link
Member

fabpot commented Dec 31, 2022

I would let the listener do the logging when it makes sense. I tend to avoid logging whenever possible.

@fabpot
Copy link
Member

fabpot commented Dec 31, 2022

Thank you @y4roc.

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

7 participants