Catch mail exceptions and still remove sent emails from queue

[tomneedham]

We have an issue where an exception is triggered when too many mails are sent in one mail connection. This is uncaught which causes the background job to fail. This means the items that are sent at the beginning of this loop dont get removed from the queue and are sent multiple times.

This PR logs the exception and maintains a list of successful email attempts to cleanup after the emails have been attempted.

[tomneedham]

Verified on an instance that faced this issue 👍

[PVince81]

I touched a similar bit there #569

[tomneedham]

I touched a similar bit there #569

Ah good work, sorry! I could merge these two tomorrow.

But then I was thinking, is it right for the activity app to be doing the validation of emails here? With this it will catch and log the exception if the email is invalid?

[PVince81]

But then I was thinking, is it right for the activity app to be doing the validation of emails here? With this it will catch and log the exception if the email is invalid?

where to catch it then ?

[tomneedham]

where to catch it then ?

Maybe $mailer->send() shoudl call the validator and emit a validation exception? which is caught by the application calling send()> Or no? Just feels weird having every use of the mailing having to run the validation method as well

[PVince81]

@tomneedham sounds good. We still need to make sure the apps like activity catch the exception and properly skip instead of failing horribly

[tomneedham]

That's what I implemented here :)

[PVince81]

isn't that too tight an exception ? we likely only want to catch and skip validation exceptions.

If the mail server (SMTP) isn't available, we should directly abort instead of continuing the queue.

This was a bit the idea in my PR, except that instead of inventing a new exception in the mailer I just did the validation directly in activity. Probably not the best idea. Should we invent a new exception in our mailer code then ?

[tomneedham]

Well we have to catch everything here so that we can later delete the items that were sent. I was seeing an error where the SMTP server was going away which was uncaught. Maybe we can catch here, exit the loop, run a cleanup method then rethrow?

[PVince81]

sounds good

[tomneedham]

Ok, I catch any exception that comes out of the mailer, run the cleanup, then throw it again which is caught by the background job handler and logged appropriately.

This means:

1. You get to know in the logs when something goes wrong with sending an email
2. Successful emails get cleared so they dont spam their users

[PVince81]

JS fail unrelated, merging.

@tomneedham please backport.

I'll have to figure a way to rebase my PRs to be integrated with your fresh changes

[tomneedham]

I'll have to figure a way to rebase my PRs to be integrated with your fresh changes

Sorry!!! :)

[PVince81]

@tomneedham if this is important and needed for stable9.1 or so, please backport. Else remove the lable.