Handle email send errors in Mailer class

2023-10-01 21:52:42 +02:00 · 2023-10-01 21:52:42 +02:00 · 38838352e2
parent d251b4c7f7
commit 38838352e2
11 changed files with 154 additions and 185 deletions
--- 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]
+    );
 }

 /**
--- a/includes/helper/email_helper.php
+++ b/includes/helper/email_helper.php
@ -1,9 +1,7 @@
 <?php

-use Engelsystem\Helpers\Translation\Translator;
 use Engelsystem\Mail\EngelsystemMailer;
 use Engelsystem\Models\User\User;
-use Psr\Log\LogLevel;

 /**
 * @param User   $recipientUser
@ -18,34 +16,14 @@ function engelsystem_email_to_user($recipientUser, $title, $message, $notIfItsMe
        return true;
    }

-    /** @var Translator $translator */
-    $translator = app()->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));
--- 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,
+            ]
+        );
    }
 }
--- 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,
+            ]
+        );
    }
 }
--- 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]
+        );
    }
 }
--- 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
--- 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
--- 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();
--- 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
--- 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);
    }
 }
--- 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'));
    }
 }