The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Issue:
The interface method \Drupal\Core\Mail\MailInterface::mail does not throw any exceptions.
If there is an error, the implementation needs to log it and return "FALSE"
It's possible to have mail errors, and we don't want that causing WSODs.
Mainly this seems to be a problem with allowing exceptions from fastglass/sendgrid to percolate, when they should be caught and logged.
Steps to reproduce:
There are countless ways to send an email that fastglass/sendgrid considers invalid.
Proposed resolution:
* all calls to fastglass/sendgrid methods need to be wrapped with try/catch
* exceptions should be logged, rather than percolated
* return FALSE on error, rather than allowing the exception to be thrown up the chain
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | sendgrid_integration-catch_exceptions-3310167-2.patch | 834 bytes | aaronbauman |
Issue fork sendgrid_integration-3310167
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
aaronbaumanSimplest solution: add another layer of indirection.
Comment #3
gregglesThis solution seems reasonable to me. Thanks for identifying this problem and suggesting the solution.
Comment #5
rymcveighI'm marking this as needing work and creating a new merge request because the current patch can not be applied to the 8.x-2.x branch.
Comment #7
rymcveighI moved your changes into an MR and based it off of the 8.x-2.x branch so it should be mergeable. I'm confused why we need the extra catch with the log. It looks like most (if not all) of the exceptions are logged in the original
::mail()function. Is there an exception that isn't covered? If so, would it be better to adjust the original function rather than add a new function to the code base?Comment #8
lkacenjaI looked this over and presume that the reason for creating a "doMail" method was to facilitate easy wrapping of the rather lengthy business logic that was formerly in the "mail" method. We do want to make sure we catch all the SendgridExceptions raised in that code. Creating a new method prevents the "mail" method from taking up extra horizontal space, but I do see the virtue in not adding new methods willy-nilly. I think either creating a new function (as authored) or wrapping all the contents of the original mail method and not creating a new function are both fine.
In my opinion, if a "doMail" method is added it should be protected. Otherwise, the intention of the original interface is violated.
Another consideration is the use case of this plugin being overridden. Adding an extra method, requires users to override a non-interface method and potentially two methods. Not sure if that matters.
Comment #9
rymcveighI adjusted the doMail function to be protected in commit 10ca0226.
Comment #10
rymcveighComment #12
perignon commentedPR merged.
Thanks for the help and contribution