Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Mailer] [Transport] Allow exception logging forRoundRobinTransport mailer#60110
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
stof commentedApr 1, 2025
Injecting an exception handler callback looks uncommon to me. This looks like a use case for injecting a logger and performing some logging. |
40b9b2c to4de5ed8Comparejnoordsij commentedApr 1, 2025
Thanks for your suggestion! While I figured a callback would allow for more flexibility, I do agree that a logger is probably a more standard way of approaching this. I've adjusted to inject a |
RoundRobinTransport mailer3c28f9e to8f6473eCompare8f6473e to13cfa6aComparejnoordsij commentedJun 21, 2025
Rebased to resolve merge conflicts. Let me know if there is anything I can do to help get this merged. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
RoundRobinTransport mailerRoundRobinTransport mailera324570 to4e62b59Comparejnoordsij commentedJun 30, 2025
Thanks for the review suggestions! Changes have been applied; this is ready for review once more. |
4e62b59 to834f876Comparefabpot commentedJul 8, 2025
Thank you@jnoordsij. |
dc81e07 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
holtkamp commentedJul 8, 2025
@jnoordsij would it be an idea to also be able to provide the preferred LogLevel (which defaults to "error") in the constructor and use I can imagine scenarios / systems in which not being able to send e-mails is a bit more urgent than the currently hard-coded LogLevel "error"... |
jnoordsij commentedJul 8, 2025
@holtkamp This definitely can be a viable user scenario I'd say, although I think defaulting to Given that this PR is now merged, I'd say if you have any ideas about how to achieve such a thing and why this is important, you can create a new PR targeting |
Uh oh!
There was an error while loading.Please reload this page.
The
RoundRobinTransportclass (and by extension theFailoverTransportclass) allow one to mail using different mail transports, with automatic failover in case a mailer fails. In case of such a failure, exceptions are caught (and grouped in case of multiple failures) and the next transport is attempted. This ensures that as long as one transport succeeds, the mail will be delivered.However, the current way of exception handling also means that exceptions on failing transports will never bubble up, unless all of them fail at once. This means that in case of a prolonged failure of a transport, e.g. due to misconfiguration, the only way to detect such a failure is by noting that a intended mailer is never sending mail, rather then be warned by actual exceptions of your application. If this is not detected, then one will only discover once all senders are failing, that one of them might have been failing all of the time already.
To allow one to be notified of all exceptions happening within the handling of sending a mail, I propose to add an
$loggerproperty to the class that implementsLoggerInterface, that would allow one log exceptions from individual mailers used. For example, this would allow one to manually send the exception to some logging service in a structured manner, so one can address the failing transport, while sending would still continue properly as long as there's still a succeeding transport left.