From 38838352e21d81a4309d50d163411ff55ad04ad6 Mon Sep 17 00:00:00 2001 From: Igor Scheller Date: Sun, 1 Oct 2023 21:52:42 +0200 Subject: [PATCH] Handle email send errors in Mailer class --- .../controller/user_angeltypes_controller.php | 52 ++++++------------- includes/helper/email_helper.php | 38 +++----------- includes/helper/shift_helper.php | 37 +++++-------- src/Events/Listener/Messages.php | 28 ++++------ src/Events/Listener/News.php | 20 +++---- src/Mail/EngelsystemMailer.php | 25 +++++---- src/Mail/Mailer.php | 26 ++++++++-- tests/Unit/Events/Listener/MessagesTest.php | 30 +---------- tests/Unit/Events/Listener/NewsTest.php | 16 ++---- tests/Unit/Mail/EngelsystemMailerTest.php | 30 ++++++++--- tests/Unit/Mail/MailerTest.php | 37 +++++++++++-- 11 files changed, 154 insertions(+), 185 deletions(-) diff --git a/includes/controller/user_angeltypes_controller.php b/includes/controller/user_angeltypes_controller.php index 7b43d19f..7b77a739 100644 --- a/includes/controller/user_angeltypes_controller.php +++ b/includes/controller/user_angeltypes_controller.php @@ -5,8 +5,6 @@ use Engelsystem\Models\AngelType; use Engelsystem\Models\User\User; use Engelsystem\Models\UserAngelType; use Illuminate\Database\Eloquent\Collection; -use Psr\Log\LoggerInterface; -use Symfony\Component\Mailer\Exception\TransportException; /** * Display a hint for team/angeltype supporters if there are unconfirmed users for his angeltype. @@ -188,23 +186,14 @@ function user_angeltype_confirm_email(User $user, AngelType $angeltype): void return; } - try { - /** @var EngelsystemMailer $mailer */ - $mailer = app(EngelsystemMailer::class); - $mailer->sendViewTranslated( - $user, - 'notification.angeltype.confirmed', - 'emails/angeltype-confirmed', - ['name' => $angeltype->name, 'angeltype' => $angeltype, 'username' => $user->displayName] - ); - } catch (TransportException $e) { - /** @var LoggerInterface $logger */ - $logger = app('logger'); - $logger->error( - 'Unable to send email "{title}" to user {user} with {exception}', - ['title' => __('notification.angeltype.confirmed'), 'user' => $user->name, 'exception' => $e] - ); - } + /** @var EngelsystemMailer $mailer */ + $mailer = app(EngelsystemMailer::class); + $mailer->sendViewTranslated( + $user, + 'notification.angeltype.confirmed', + 'emails/angeltype-confirmed', + ['name' => $angeltype->name, 'angeltype' => $angeltype, 'username' => $user->displayName] + ); } function user_angeltype_add_email(User $user, AngelType $angeltype): void @@ -213,23 +202,14 @@ function user_angeltype_add_email(User $user, AngelType $angeltype): void return; } - try { - /** @var EngelsystemMailer $mailer */ - $mailer = app(EngelsystemMailer::class); - $mailer->sendViewTranslated( - $user, - 'notification.angeltype.added', - 'emails/angeltype-added', - ['name' => $angeltype->name, 'angeltype' => $angeltype, 'username' => $user->displayName] - ); - } catch (TransportException $e) { - /** @var LoggerInterface $logger */ - $logger = app('logger'); - $logger->error( - 'Unable to send email "{title}" to user {user} with {exception}', - ['title' => __('notification.angeltype.added'), 'user' => $user->name, 'exception' => $e] - ); - } + /** @var EngelsystemMailer $mailer */ + $mailer = app(EngelsystemMailer::class); + $mailer->sendViewTranslated( + $user, + 'notification.angeltype.added', + 'emails/angeltype-added', + ['name' => $angeltype->name, 'angeltype' => $angeltype, 'username' => $user->displayName] + ); } /** diff --git a/includes/helper/email_helper.php b/includes/helper/email_helper.php index fb340107..1d84cb34 100644 --- a/includes/helper/email_helper.php +++ b/includes/helper/email_helper.php @@ -1,9 +1,7 @@ get('translator'); - $locale = $translator->getLocale(); - - $status = true; - try { - /** @var EngelsystemMailer $mailer */ - $mailer = app('mailer'); - - $translator->setLocale($recipientUser->settings->language); - $mailer->sendView( - $recipientUser->contact->email ?: $recipientUser->email, - $title, - 'emails/mail', - ['username' => $recipientUser->displayName, 'message' => $message] - ); - } catch (Exception $e) { - $status = false; - engelsystem_log(sprintf( - 'An exception occurred while sending a mail to %s in %s:%u: %s', - $recipientUser->name, - $e->getFile(), - $e->getLine(), - $e->getMessage() - ), LogLevel::CRITICAL); - } - - $translator->setLocale($locale); + /** @var EngelsystemMailer $mailer */ + $mailer = app('mailer'); + $status = $mailer->sendViewTranslated( + $recipientUser, + $title, + 'emails/mail', + ['username' => $recipientUser->displayName, 'message' => $message] + ); if (!$status) { error(sprintf(__('User %s could not be notified by email due to an error.'), $recipientUser->displayName)); diff --git a/includes/helper/shift_helper.php b/includes/helper/shift_helper.php index e6edb614..0452079e 100644 --- a/includes/helper/shift_helper.php +++ b/includes/helper/shift_helper.php @@ -9,7 +9,6 @@ use Engelsystem\Models\Room; use Engelsystem\Models\User\User; use Engelsystem\Models\Worklog; use Psr\Log\LoggerInterface; -use Symfony\Component\Mailer\Exception\TransportException; class Shift { @@ -71,27 +70,19 @@ class Shift return; } - $subject = 'notification.shift.deleted'; - try { - $this->mailer->sendViewTranslated( - $user, - $subject, - 'emails/worklog-from-shift', - [ - 'name' => $name, - 'title' => $title, - 'start' => $start, - 'end' => $end, - 'room' => $room, - 'freeloaded' => $freeloaded, - 'username' => $user->displayName, - ] - ); - } catch (TransportException $e) { - $this->log->error( - 'Unable to send email "{title}" to user {user} with {exception}', - ['title' => $subject, 'user' => $user->name, 'exception' => $e] - ); - } + $this->mailer->sendViewTranslated( + $user, + 'notification.shift.deleted', + 'emails/worklog-from-shift', + [ + 'name' => $name, + 'title' => $title, + 'start' => $start, + 'end' => $end, + 'room' => $room, + 'freeloaded' => $freeloaded, + 'username' => $user->displayName, + ] + ); } } diff --git a/src/Events/Listener/Messages.php b/src/Events/Listener/Messages.php index 528a601f..4d93e610 100644 --- a/src/Events/Listener/Messages.php +++ b/src/Events/Listener/Messages.php @@ -8,7 +8,6 @@ use Engelsystem\Mail\EngelsystemMailer; use Engelsystem\Models\Message; use Engelsystem\Models\User\User; use Psr\Log\LoggerInterface; -use Symfony\Component\Mailer\Exception\TransportException; class Messages { @@ -29,22 +28,15 @@ class Messages private function sendMail(Message $message, User $user, string $subject, string $template): void { - try { - $this->mailer->sendViewTranslated( - $user, - $subject, - $template, - [ - 'sender' => $message->sender->displayName, - 'send_message' => $message, - 'username' => $user->displayName, - ] - ); - } catch (TransportException $e) { - $this->log->error( - 'Unable to send email "{title}" to user {user} with {exception}', - ['title' => $subject, 'user' => $user->name, 'exception' => $e] - ); - } + $this->mailer->sendViewTranslated( + $user, + $subject, + $template, + [ + 'sender' => $message->sender->displayName, + 'send_message' => $message, + 'username' => $user->displayName, + ] + ); } } diff --git a/src/Events/Listener/News.php b/src/Events/Listener/News.php index 052a126a..8be208cb 100644 --- a/src/Events/Listener/News.php +++ b/src/Events/Listener/News.php @@ -10,7 +10,6 @@ use Engelsystem\Models\User\Settings as UserSettings; use Engelsystem\Models\User\User; use Illuminate\Database\Eloquent\Collection; use Psr\Log\LoggerInterface; -use Throwable; class News { @@ -36,18 +35,11 @@ class News protected function sendMail(NewsModel $news, User $user, string $subject, string $template): void { - try { - $this->mailer->sendViewTranslated( - $user, - $subject, - $template, - ['title' => $news->title, 'news' => $news, 'username' => $user->displayName] - ); - } catch (Throwable $e) { - $this->log->error( - 'Unable to send email "{title}" to user {user} with {exception}', - ['title' => $subject, 'user' => $user->name, 'exception' => $e] - ); - } + $this->mailer->sendViewTranslated( + $user, + $subject, + $template, + ['title' => $news->title, 'news' => $news, 'username' => $user->displayName] + ); } } diff --git a/src/Mail/EngelsystemMailer.php b/src/Mail/EngelsystemMailer.php index 51d7a7ca..bea73cc9 100644 --- a/src/Mail/EngelsystemMailer.php +++ b/src/Mail/EngelsystemMailer.php @@ -7,6 +7,7 @@ namespace Engelsystem\Mail; use Engelsystem\Helpers\Translation\Translator; use Engelsystem\Models\User\User; use Engelsystem\Renderer\Renderer; +use Psr\Log\LoggerInterface; use Symfony\Component\Mailer\MailerInterface; class EngelsystemMailer extends Mailer @@ -21,9 +22,13 @@ class EngelsystemMailer extends Mailer * @param Renderer|null $view * @param Translator|null $translation */ - public function __construct(MailerInterface $mailer, Renderer $view = null, Translator $translation = null) - { - parent::__construct($mailer); + public function __construct( + LoggerInterface $log, + MailerInterface $mailer, + Renderer $view = null, + Translator $translation = null + ) { + parent::__construct($log, $mailer); $this->translation = $translation; $this->view = $view; @@ -38,7 +43,7 @@ class EngelsystemMailer extends Mailer string $template, array $data = [], ?string $locale = null - ): void { + ): bool { if ($to instanceof User) { $locale = $locale ?: $to->settings->language; $to = $to->contact->email ?: $to->email; @@ -55,11 +60,13 @@ class EngelsystemMailer extends Mailer } $subject = $this->translation ? $this->translation->translate($subject, $data) : $subject; - $this->sendView($to, $subject, $template, $data); + $status = $this->sendView($to, $subject, $template, $data); if ($activeLocale) { $this->translation->setLocale($activeLocale); } + + return $status; } /** @@ -67,11 +74,11 @@ class EngelsystemMailer extends Mailer * * @param string|string[] $to */ - public function sendView(string|array $to, string $subject, string $template, array $data = []): void + public function sendView(string|array $to, string $subject, string $template, array $data = []): bool { $body = $this->view->render($template, $data); - $this->send($to, $subject, $body); + return $this->send($to, $subject, $body); } /** @@ -79,13 +86,13 @@ class EngelsystemMailer extends Mailer * * @param string|string[] $to */ - public function send(string|array $to, string $subject, string $body): void + public function send(string|array $to, string $subject, string $body): bool { if ($this->subjectPrefix) { $subject = sprintf('[%s] %s', $this->subjectPrefix, trim($subject)); } - parent::send($to, $subject, $body); + return parent::send($to, $subject, $body); } public function getSubjectPrefix(): string diff --git a/src/Mail/Mailer.php b/src/Mail/Mailer.php index de640218..eb905d79 100644 --- a/src/Mail/Mailer.php +++ b/src/Mail/Mailer.php @@ -4,8 +4,10 @@ declare(strict_types=1); namespace Engelsystem\Mail; +use Psr\Log\LoggerInterface; use Symfony\Component\Mailer\MailerInterface; use Symfony\Component\Mime\Email; +use Throwable; class Mailer { @@ -13,7 +15,7 @@ class Mailer protected ?string $fromName = null; - public function __construct(protected MailerInterface $mailer) + public function __construct(protected LoggerInterface $log, protected MailerInterface $mailer) { } @@ -22,7 +24,7 @@ class Mailer * * @param string|string[] $to */ - public function send(string|array $to, string $subject, string $body): void + public function send(string|array $to, string $subject, string $body): bool { $message = (new Email()) ->to(...(array) $to) @@ -30,7 +32,25 @@ class Mailer ->subject($subject) ->text($body); - $this->mailer->send($message); + try { + $this->mailer->send($message); + } catch (Throwable $e) { + $this->log->error( + 'Unable to send e-mail "{subject}" to {to} in {file}:{line}: {type}: {message}', + [ + 'subject' => $subject, + 'to' => $to, + 'file' => $e->getFile(), + 'line' => $e->getLine(), + 'type' => get_class($e), + 'message' => $e->getMessage(), + ] + ); + + return false; + } + + return true; } public function getFromAddress(): string diff --git a/tests/Unit/Events/Listener/MessagesTest.php b/tests/Unit/Events/Listener/MessagesTest.php index c7ae7a66..502269d4 100644 --- a/tests/Unit/Events/Listener/MessagesTest.php +++ b/tests/Unit/Events/Listener/MessagesTest.php @@ -14,7 +14,6 @@ use Engelsystem\Test\Unit\HasDatabase; use Engelsystem\Test\Unit\TestCase; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\Test\TestLogger; -use Symfony\Component\Mailer\Exception\TransportException; class MessagesTest extends TestCase { @@ -46,13 +45,14 @@ class MessagesTest extends TestCase string $subject, string $template, array $data - ) use ($user): void { + ) use ($user): bool { $this->assertEquals($user->id, $receiver->id); $this->assertEquals('notification.messages.new', $subject); $this->assertEquals('emails/messages-new', $template); $this->assertArrayHasKey('username', $data); $this->assertArrayHasKey('sender', $data); $this->assertArrayHasKey('send_message', $data); + return true; }); $handler = new Messages($this->log, $mailer); @@ -79,32 +79,6 @@ class MessagesTest extends TestCase $handler->created($message); } - /** - * @covers \Engelsystem\Events\Listener\Messages::sendMail - */ - public function testSendMailExceptionHandling(): void - { - /** @var EngelsystemMailer|MockObject $mailer */ - $mailer = $this->createMock(EngelsystemMailer::class); - /** @var User $user */ - $user = User::factory() - ->has(Settings::factory([ - 'email_messages' => true, - ])) - ->create(); - $message = Message::factory()->create(['receiver_id' => $user->id]); - $mailer->expects($this->once()) - ->method('sendViewTranslated') - ->willReturnCallback(function (): void { - throw new TransportException(); - }); - - $handler = new Messages($this->log, $mailer); - - $handler->created($message); - $this->assertTrue($this->log->hasErrorThatContains('Unable to send email')); - } - protected function setUp(): void { $this->log = new TestLogger(); diff --git a/tests/Unit/Events/Listener/NewsTest.php b/tests/Unit/Events/Listener/NewsTest.php index 050570c6..8b2388b8 100644 --- a/tests/Unit/Events/Listener/NewsTest.php +++ b/tests/Unit/Events/Listener/NewsTest.php @@ -12,7 +12,6 @@ use Engelsystem\Models\User\Settings; use Engelsystem\Models\User\User; use Engelsystem\Test\Unit\HasDatabase; use Engelsystem\Test\Unit\TestCase; -use Exception; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Psr\Log\Test\TestLogger; @@ -38,29 +37,20 @@ class NewsTest extends TestCase /** @var NewsModel $news */ $news = NewsModel::factory(['title' => 'Foo'])->create(); - $i = 0; - $this->mailer->expects($this->exactly(2)) + $this->mailer->expects($this->once()) ->method('sendViewTranslated') - ->willReturnCallback(function (User $user, string $subject, string $template, array $data) use (&$i): void { + ->willReturnCallback(function (User $user, string $subject, string $template, array $data): bool { $this->assertEquals(1, $user->id); $this->assertEquals('notification.news.new', $subject); $this->assertEquals('emails/news-new', $template); $this->assertEquals('Foo', array_values($data)[0]); - if ($i++ > 0) { // On second run - throw new Exception('Oops'); - } + return true; }); /** @var News $listener */ $listener = $this->app->make(News::class); - $error = 'Unable to send email'; - $listener->created($news); - $this->assertFalse($this->log->hasErrorThatContains($error)); - - $listener->created($news); - $this->assertTrue($this->log->hasErrorThatContains($error)); } protected function setUp(): void diff --git a/tests/Unit/Mail/EngelsystemMailerTest.php b/tests/Unit/Mail/EngelsystemMailerTest.php index 04c842d9..6f558ce9 100644 --- a/tests/Unit/Mail/EngelsystemMailerTest.php +++ b/tests/Unit/Mail/EngelsystemMailerTest.php @@ -13,6 +13,7 @@ use Engelsystem\Renderer\Renderer; use Engelsystem\Test\Unit\HasDatabase; use Engelsystem\Test\Unit\TestCase; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\NullLogger; use Symfony\Component\Mailer\Envelope; use Symfony\Component\Mailer\MailerInterface; use Symfony\Component\Mime\RawMessage; @@ -33,13 +34,14 @@ class EngelsystemMailerTest extends TestCase $symfonyMailer = $this->getMockForAbstractClass(MailerInterface::class); /** @var EngelsystemMailer|MockObject $mailer */ $mailer = $this->getMockBuilder(EngelsystemMailer::class) - ->setConstructorArgs(['mailer' => $symfonyMailer, 'view' => $view]) + ->setConstructorArgs(['log' => new NullLogger(), 'mailer' => $symfonyMailer, 'view' => $view]) ->onlyMethods(['send']) ->getMock(); - $this->setExpects($mailer, 'send', ['foo@bar.baz', 'Lorem dolor', 'Rendered Stuff!']); + $this->setExpects($mailer, 'send', ['foo@bar.baz', 'Lorem dolor', 'Rendered Stuff!'], true); $this->setExpects($view, 'render', ['test/template.tpl', ['dev' => true]], 'Rendered Stuff!'); - $mailer->sendView('foo@bar.baz', 'Lorem dolor', 'test/template.tpl', ['dev' => true]); + $status = $mailer->sendView('foo@bar.baz', 'Lorem dolor', 'test/template.tpl', ['dev' => true]); + $this->assertTrue($status); } /** @@ -63,11 +65,21 @@ class EngelsystemMailerTest extends TestCase /** @var EngelsystemMailer|MockObject $mailer */ $mailer = $this->getMockBuilder(EngelsystemMailer::class) - ->setConstructorArgs(['mailer' => $symfonyMailer, 'view' => $view, 'translation' => $translator]) + ->setConstructorArgs([ + 'log' => new NullLogger(), + 'mailer' => $symfonyMailer, + 'view' => $view, + 'translation' => $translator, + ]) ->onlyMethods(['sendView']) ->getMock(); - $this->setExpects($mailer, 'sendView', ['foo@bar.baz', 'Lorem dolor', 'test/template.tpl', ['dev' => true]]); + $this->setExpects( + $mailer, + 'sendView', + ['foo@bar.baz', 'Lorem dolor', 'test/template.tpl', ['dev' => true]], + true + ); $this->setExpects($translator, 'getLocales', null, ['de_DE' => 'de_DE', 'en_US' => 'en_US']); $this->setExpects($translator, 'getLocale', null, 'en_US'); $this->setExpects($translator, 'translate', ['translatable.text', ['dev' => true]], 'Lorem dolor'); @@ -75,13 +87,14 @@ class EngelsystemMailerTest extends TestCase ->method('setLocale') ->withConsecutive(['de_DE'], ['en_US']); - $mailer->sendViewTranslated( + $status = $mailer->sendViewTranslated( $user, 'translatable.text', 'test/template.tpl', ['dev' => true], 'de_DE' ); + $this->assertTrue($status); } /** @@ -104,13 +117,14 @@ class EngelsystemMailerTest extends TestCase $this->assertStringContainsString('Lorem Ipsum!', $message->toString()); }); - $mailer = new EngelsystemMailer($symfonyMailer); + $mailer = new EngelsystemMailer(new NullLogger(), $symfonyMailer); $mailer->setFromAddress('foo@bar.baz'); $mailer->setFromName('Foo Bar'); $mailer->setSubjectPrefix('Mail test'); $this->assertEquals('Mail test', $mailer->getSubjectPrefix()); - $mailer->send('to@xam.pel', 'Foo Bar ', 'Lorem Ipsum!'); + $status = $mailer->send('to@xam.pel', 'Foo Bar ', 'Lorem Ipsum!'); + $this->assertTrue($status); } } diff --git a/tests/Unit/Mail/MailerTest.php b/tests/Unit/Mail/MailerTest.php index 79f497ee..8afd94f1 100644 --- a/tests/Unit/Mail/MailerTest.php +++ b/tests/Unit/Mail/MailerTest.php @@ -7,7 +7,10 @@ namespace Engelsystem\Test\Unit\Mail; use Engelsystem\Mail\Mailer; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Psr\Log\NullLogger; +use Psr\Log\Test\TestLogger; use Symfony\Component\Mailer\Envelope; +use Symfony\Component\Mailer\Exception\TransportException; use Symfony\Component\Mailer\MailerInterface; use Symfony\Component\Mime\RawMessage; @@ -22,10 +25,11 @@ class MailerTest extends TestCase */ public function testInitAndSettersAndGetters(): void { + $log = new NullLogger(); /** @var MailerInterface|MockObject $symfonyMailer */ $symfonyMailer = $this->createMock(MailerInterface::class); - $mailer = new Mailer($symfonyMailer); + $mailer = new Mailer($log, $symfonyMailer); $mailer->setFromName('From Name'); $this->assertEquals('From Name', $mailer->getFromName()); @@ -39,6 +43,7 @@ class MailerTest extends TestCase */ public function testSend(): void { + $log = new NullLogger(); /** @var MailerInterface|MockObject $symfonyMailer */ $symfonyMailer = $this->createMock(MailerInterface::class); $symfonyMailer->expects($this->once()) @@ -51,10 +56,36 @@ class MailerTest extends TestCase $this->assertStringContainsString('Lorem Ipsum!', $message->toString()); }); - $mailer = new Mailer($symfonyMailer); + $mailer = new Mailer($log, $symfonyMailer); $mailer->setFromAddress('foo@bar.baz'); $mailer->setFromName('Test Tester'); - $mailer->send('to@xam.pel', 'Foo Bar', 'Lorem Ipsum!'); + $status = $mailer->send('to@xam.pel', 'Foo Bar', 'Lorem Ipsum!'); + $this->assertTrue($status); + } + + + /** + * @covers \Engelsystem\Mail\Mailer::send + */ + public function testSendException(): void + { + $log = new TestLogger(); + /** @var MailerInterface|MockObject $symfonyMailer */ + $symfonyMailer = $this->createMock(MailerInterface::class); + $symfonyMailer->expects($this->once()) + ->method('send') + ->willReturnCallback(function (RawMessage $message, Envelope $envelope = null): void { + throw new TransportException('Unable to connect to port 42'); + }); + + $mailer = new Mailer($log, $symfonyMailer); + $mailer->setFromAddress('foo@bar.baz'); + $mailer->setFromName('Test Tester'); + + $status = $mailer->send('to@xam.pel', 'Foo Bar', 'Lorem Ipsum!'); + $this->assertFalse($status); + + $this->assertTrue($log->hasErrorThatContains('Unable to send e-mail')); } }