[BUGFIX] Add tests and fix broken MailMessage 08/61908/7
authorFrank Naegler <frank.naegler@typo3.org>
Tue, 8 Oct 2019 11:44:13 +0000 (13:44 +0200)
committerDaniel Goerz <daniel.goerz@posteo.de>
Thu, 10 Oct 2019 14:50:24 +0000 (16:50 +0200)
This patch add tests for the MailMessage class and fix broken
functionality since migration to Symfony Mailer.

Resolves: #89083
Related: #88643
Releases: master
Change-Id: Ieb7616aa24cee26505d0e2260a26b56713caebc1
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61908
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Tested-by: Daniel Goerz <daniel.goerz@posteo.de>
Reviewed-by: Andreas Fernandez <a.fernandez@scripting-base.de>
Reviewed-by: Daniel Goerz <daniel.goerz@posteo.de>
typo3/sysext/core/Classes/Exception/InvalidArgumentException.php [new file with mode: 0644]
typo3/sysext/core/Classes/Mail/MailMessage.php
typo3/sysext/core/Tests/Unit/Mail/MailMessageTest.php [new file with mode: 0644]

diff --git a/typo3/sysext/core/Classes/Exception/InvalidArgumentException.php b/typo3/sysext/core/Classes/Exception/InvalidArgumentException.php
new file mode 100644 (file)
index 0000000..a70752d
--- /dev/null
@@ -0,0 +1,21 @@
+<?php
+declare(strict_types = 1);
+
+namespace TYPO3\CMS\Core\Exception;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+class InvalidArgumentException extends \RuntimeException
+{
+}
index b09ae2d..d412056 100644 (file)
@@ -17,6 +17,7 @@ namespace TYPO3\CMS\Core\Mail;
 use Symfony\Component\Mime\Address;
 use Symfony\Component\Mime\Email;
 use Symfony\Component\Mime\NamedAddress;
+use TYPO3\CMS\Core\Exception\InvalidArgumentException;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 
 /**
@@ -32,18 +33,13 @@ class MailMessage extends Email
     protected $mailer;
 
     /**
-     * @var string This will be added as X-Mailer to all outgoing mails
-     */
-    protected $mailerHeader = 'TYPO3';
-
-    /**
      * TRUE if the message has been sent.
      *
      * @var bool
      */
     protected $sent = false;
 
-    private function initializeMailer()
+    private function initializeMailer(): void
     {
         $this->mailer = GeneralUtility::makeInstance(Mailer::class);
     }
@@ -56,7 +52,7 @@ class MailMessage extends Email
      *
      * @return bool whether the message was accepted or not
      */
-    public function send()
+    public function send(): bool
     {
         $this->initializeMailer();
         $this->sent = false;
@@ -73,7 +69,7 @@ class MailMessage extends Email
      *
      * @return bool
      */
-    public function isSent()
+    public function isSent(): bool
     {
         return $this->sent;
     }
@@ -90,20 +86,22 @@ class MailMessage extends Email
      * Set the subject of the message.
      *
      * @param string $subject
+     * @return MailMessage
      */
-    public function setSubject($subject)
+    public function setSubject($subject): self
     {
-        $this->subject($subject);
+        return $this->subject($subject);
     }
 
     /**
      * Set the origination date of the message as a UNIX timestamp.
      *
      * @param int $date
+     * @return MailMessage
      */
-    public function setDate($date)
+    public function setDate($date): self
     {
-        $this->date(new \DateTime('@' . $date));
+        return $this->date(new \DateTime('@' . $date));
     }
 
     /**
@@ -112,7 +110,7 @@ class MailMessage extends Email
      * @param string $address
      * @return MailMessage
      */
-    public function setReturnPath($address)
+    public function setReturnPath($address): self
     {
         return $this->returnPath($address);
     }
@@ -126,10 +124,9 @@ class MailMessage extends Email
      * @param string $name optional
      * @return MailMessage
      */
-    public function setSender($address, $name = null)
+    public function setSender($address, $name = null): self
     {
-        $address = $this->convertNamedAddress($address, $name);
-        return $this->sender($address);
+        return $this->sender(...$this->convertNamedAddress($address, $name));
     }
 
     /**
@@ -139,15 +136,16 @@ class MailMessage extends Email
      *
      * If $name is passed and the first parameter is a string, this name will be
      * associated with the address.
+     * If $name is passed and the first parameter is not a string, an exception is thrown.
      *
      * @param string|array $addresses
      * @param string $name optional
      * @return MailMessage
      */
-    public function setFrom($addresses, $name = null)
+    public function setFrom($addresses, $name = null): self
     {
-        $addresses = $this->convertNamedAddress($addresses, $name);
-        return $this->from($addresses, $name);
+        $this->checkArguments($addresses, $name);
+        return $this->from(...$this->convertNamedAddress($addresses, $name));
     }
 
     /**
@@ -157,15 +155,16 @@ class MailMessage extends Email
      *
      * If $name is passed and the first parameter is a string, this name will be
      * associated with the address.
+     * If $name is passed and the first parameter is not a string, an exception is thrown.
      *
      * @param string|array $addresses
      * @param string $name optional
      * @return MailMessage
      */
-    public function setReplyTo($addresses, $name = null)
+    public function setReplyTo($addresses, $name = null): self
     {
-        $addresses = $this->convertNamedAddress($addresses, $name);
-        return $this->replyTo($addresses);
+        $this->checkArguments($addresses, $name);
+        return $this->replyTo(...$this->convertNamedAddress($addresses, $name));
     }
 
     /**
@@ -176,71 +175,80 @@ class MailMessage extends Email
      *
      * If $name is passed and the first parameter is a string, this name will be
      * associated with the address.
+     * If $name is passed and the first parameter is not a string, an exception is thrown.
      *
      * @param string|array $addresses
      * @param string $name optional
      * @return MailMessage
      */
-    public function setTo($addresses, $name = null)
+    public function setTo($addresses, $name = null): self
     {
-        $addresses = $this->convertNamedAddress($addresses, $name);
-        return $this->to($addresses);
+        $this->checkArguments($addresses, $name);
+        return $this->to(...$this->convertNamedAddress($addresses, $name));
     }
 
     /**
      * Set the Cc addresses of this message.
      *
+     * If multiple recipients will receive the message an array should be used.
+     * Example: array('receiver@domain.org', 'other@domain.org' => 'A name')
+     *
      * If $name is passed and the first parameter is a string, this name will be
      * associated with the address.
+     * If $name is passed and the first parameter is not a string, an exception is thrown.
      *
      * @param string|array $addresses
      * @param string $name optional
      * @return MailMessage
      */
-    public function setCc($addresses, $name = null)
+    public function setCc($addresses, $name = null): self
     {
-        $addresses = $this->convertNamedAddress($addresses, $name);
-        return $this->cc($addresses);
+        $this->checkArguments($addresses, $name);
+        return $this->cc(...$this->convertNamedAddress($addresses, $name));
     }
 
     /**
      * Set the Bcc addresses of this message.
      *
+     * If multiple recipients will receive the message an array should be used.
+     * Example: array('receiver@domain.org', 'other@domain.org' => 'A name')
+     *
      * If $name is passed and the first parameter is a string, this name will be
      * associated with the address.
+     * If $name is passed and the first parameter is not a string, an exception is thrown.
      *
      * @param string|array $addresses
      * @param string $name optional
      * @return MailMessage
      */
-    public function setBcc($addresses, $name = null)
+    public function setBcc($addresses, $name = null): self
     {
-        $addresses = $this->convertNamedAddress($addresses, $name);
-        return $this->bcc($addresses);
+        $this->checkArguments($addresses, $name);
+        return $this->bcc(...$this->convertNamedAddress($addresses, $name));
     }
 
     /**
      * Ask for a delivery receipt from the recipient to be sent to $addresses.
      *
-     * @param array $addresses
+     * @param string $address
      * @return MailMessage
      */
-    public function setReadReceiptTo($addresses)
+    public function setReadReceiptTo(string $address): self
     {
-        $addresses = $this->convertNamedAddress($addresses);
-        return $this->setReadReceiptTo($addresses);
+        $this->getHeaders()->addMailboxHeader('Disposition-Notification-To', $address);
+        return $this;
     }
 
     /**
      * Converts Addresses into Address/NamedAddress objects.
      *
      * @param string|array $args
-     * @return string|array
+     * @return Address[]
      */
-    protected function convertNamedAddress(...$args)
+    protected function convertNamedAddress(...$args): array
     {
         if (isset($args[1])) {
-            return new NamedAddress($args[0], $args[1]);
+            return [new NamedAddress($args[0], $args[1])];
         }
         if (is_string($args[0]) || is_array($args[0])) {
             return $this->convertAddresses($args[0]);
@@ -252,12 +260,12 @@ class MailMessage extends Email
      * Converts Addresses into Address/NamedAddress objects.
      *
      * @param string|array $addresses
-     * @return string|array
+     * @return Address[]
      */
-    protected function convertAddresses($addresses)
+    protected function convertAddresses($addresses): array
     {
         if (!is_array($addresses)) {
-            return Address::create($addresses);
+            return [Address::create($addresses)];
         }
         $newAddresses = [];
         foreach ($addresses as $email => $name) {
@@ -271,53 +279,39 @@ class MailMessage extends Email
         return $newAddresses;
     }
 
-    /**
-     * compatibility methods to allow for associative arrays as [name => email address]
-     * as it was possible in TYPO3 v9 / SwiftMailer.
-     */
+    //
+    // Compatibility methods, as it was possible in TYPO3 v9 / SwiftMailer.
+    //
 
-    /**
-     * @inheritdoc
-     */
-    public function addFrom(...$addresses)
+    public function addFrom(...$addresses): Email
     {
-        $addresses = $this->convertNamedAddress(...$addresses);
-        return parent::addFrom(...$addresses);
+        return parent::addFrom(...$this->convertNamedAddress(...$addresses));
     }
 
-    /**
-     * @inheritdoc
-     */
-    public function addReplyTo(...$addresses)
+    public function addReplyTo(...$addresses): Email
     {
-        $addresses = $this->convertNamedAddress(...$addresses);
-        return parent::addReplyTo(...$addresses);
+        return parent::addReplyTo(...$this->convertNamedAddress(...$addresses));
     }
 
-    /**
-     * @inheritdoc
-     */
-    public function addTo(...$addresses)
+    public function addTo(...$addresses): Email
     {
-        $addresses = $this->convertNamedAddress(...$addresses);
-        return parent::addTo(...$addresses);
+        return parent::addTo(...$this->convertNamedAddress(...$addresses));
     }
 
-    /**
-     * @inheritdoc
-     */
-    public function addCc(...$addresses)
+    public function addCc(...$addresses): Email
     {
-        $addresses = $this->convertNamedAddress(...$addresses);
-        return parent::addCc(...$addresses);
+        return parent::addCc(...$this->convertNamedAddress(...$addresses));
     }
 
-    /**
-     * @inheritdoc
-     */
-    public function addBcc(...$addresses)
+    public function addBcc(...$addresses): Email
     {
-        $addresses = $this->convertNamedAddress(...$addresses);
-        return parent::addBcc(...$addresses);
+        return parent::addBcc(...$this->convertNamedAddress(...$addresses));
+    }
+
+    protected function checkArguments($addresses, string $name = null): void
+    {
+        if ($name !== null && !is_string($addresses)) {
+            throw new InvalidArgumentException('The combination of a name and an array of addresses is invalid.', 1570543657);
+        }
     }
 }
diff --git a/typo3/sysext/core/Tests/Unit/Mail/MailMessageTest.php b/typo3/sysext/core/Tests/Unit/Mail/MailMessageTest.php
new file mode 100644 (file)
index 0000000..0a8bfee
--- /dev/null
@@ -0,0 +1,323 @@
+<?php
+declare(strict_types = 1);
+namespace TYPO3\CMS\Core\Tests\Unit\Mail;
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+use Symfony\Component\Mime\Address;
+use TYPO3\CMS\Core\Exception\InvalidArgumentException;
+use TYPO3\CMS\Core\Mail\MailMessage;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+/**
+ * Test case
+ */
+class MailMessageTest extends UnitTestCase
+{
+    /**
+     * @var bool Reset singletons created by subject
+     */
+    protected $resetSingletonInstances = true;
+
+    /**
+     * @var MailMessage
+     */
+    protected $subject;
+
+    /**
+     * Set up
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->subject = new MailMessage();
+    }
+
+    /**
+     * @test
+     */
+    public function isSentReturnsFalseIfMailWasNotSent(): void
+    {
+        $this->assertFalse($this->subject->isSent());
+    }
+
+    /**
+     * @test
+     */
+    public function setSubjectWorksAsExpected(): void
+    {
+        $this->subject->setSubject('Test');
+        $this->assertSame('Test', $this->subject->getSubject());
+        $this->subject->setSubject('Test2');
+        $this->assertSame('Test2', $this->subject->getSubject());
+    }
+
+    /**
+     * @test
+     */
+    public function setDateWorksAsExpected(): void
+    {
+        $time = time();
+        $this->subject->setDate($time);
+        $this->assertSame($time, (int)$this->subject->getDate()->format('U'));
+        $time++;
+        $this->subject->setDate($time);
+        $this->assertSame($time, (int)$this->subject->getDate()->format('U'));
+    }
+
+    /**
+     * @test
+     */
+    public function setReturnPathWorksAsExpected(): void
+    {
+        $this->subject->setReturnPath('noreply@typo3.com');
+        $this->assertInstanceOf(Address::class, $this->subject->getReturnPath());
+        $this->assertSame('noreply@typo3.com', $this->subject->getReturnPath()->getAddress());
+        $this->subject->setReturnPath('no-reply@typo3.com');
+        $this->assertInstanceOf(Address::class, $this->subject->getReturnPath());
+        $this->assertSame('no-reply@typo3.com', $this->subject->getReturnPath()->getAddress());
+    }
+
+    public function setSenderAddressDataProvider(): array
+    {
+        return [
+            'address without name' => ['admin@typo3.com', null, 'admin@typo3.com'],
+            'address with name' => ['admin@typo3.com', 'Admin', 'Admin <admin@typo3.com>'],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider setSenderAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param string $expectedString
+     */
+    public function setSenderWorksAsExpected($address, $name, $expectedString): void
+    {
+        $this->subject->setSender($address, $name);
+        $this->assertInstanceOf(Address::class, $this->subject->getSender());
+        $this->assertSame($address, $this->subject->getSender()->getAddress());
+        $this->assertSame($expectedString, $this->subject->getSender()->toString());
+    }
+
+    public function globalSetAddressDataProvider(): array
+    {
+        return [
+            'address without name' => ['admin@typo3.com', null, ['admin@typo3.com']],
+            'address with name' => ['admin@typo3.com', 'Admin', ['Admin <admin@typo3.com>']],
+            'multiple addresses without name' => [['admin@typo3.com', 'system@typo3.com'], null, ['admin@typo3.com', 'system@typo3.com']],
+            'address as array' => [['admin@typo3.com' => 'Admin'], null, ['Admin <admin@typo3.com>']],
+            'multiple addresses as array' => [['admin@typo3.com' => 'Admin', 'system@typo3.com' => 'System'], null, ['Admin <admin@typo3.com>', 'System <system@typo3.com>']],
+            'multiple addresses as array mixed' => [['admin@typo3.com' => 'Admin', 'it@typo3.com', 'system@typo3.com' => 'System'], null, ['Admin <admin@typo3.com>', 'it@typo3.com', 'System <system@typo3.com>']],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider globalSetAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function setFromWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        // We first add one address, because set should override / remove existing addresses
+        $this->subject->addFrom('foo@bar.com', 'Foo');
+        $this->subject->setFrom($address, $name);
+        $this->assertCorrectAddresses($this->subject->getFrom(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalSetAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function setReplyToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        // We first add one address, because set should override / remove existing addresses
+        $this->subject->addReplyTo('foo@bar.com', 'Foo');
+        $this->subject->setReplyTo($address, $name);
+        $this->assertCorrectAddresses($this->subject->getReplyTo(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalSetAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function setToToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        // We first add one address, because set should override / remove existing addresses
+        $this->subject->addTo('foo@bar.com', 'Foo');
+        $this->subject->setTo($address, $name);
+        $this->assertCorrectAddresses($this->subject->getTo(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalSetAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function setCcToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        // We first add one address, because set should override / remove existing addresses
+        $this->subject->addCc('foo@bar.com', 'Foo');
+        $this->subject->setCc($address, $name);
+        $this->assertCorrectAddresses($this->subject->getCc(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalSetAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function setBccToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        // We first add one address, because set should override / remove existing addresses
+        $this->subject->addBcc('foo@bar.com', 'Foo');
+        $this->subject->setBcc($address, $name);
+        $this->assertCorrectAddresses($this->subject->getBcc(), $expectedAddresses);
+    }
+
+    public function globalAddAddressDataProvider(): array
+    {
+        return [
+            'address without name' => ['admin@typo3.com', null, ['admin@typo3.com']],
+            'address with name' => ['admin@typo3.com', 'Admin', ['Admin <admin@typo3.com>']],
+            'address as array' => [['admin@typo3.com' => 'Admin'], null, ['Admin <admin@typo3.com>']],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider globalAddAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function addFromToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        $this->subject->addFrom($address, $name);
+        $this->assertCorrectAddresses($this->subject->getFrom(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalAddAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function addReplyToToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        $this->subject->addReplyTo($address, $name);
+        $this->assertCorrectAddresses($this->subject->getReplyTo(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalAddAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function addToToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        $this->subject->addTo($address, $name);
+        $this->assertCorrectAddresses($this->subject->getTo(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalAddAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function addCcToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        $this->subject->addCc($address, $name);
+        $this->assertCorrectAddresses($this->subject->getCc(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     * @dataProvider globalAddAddressDataProvider
+     * @param string $address
+     * @param string $name
+     * @param array $expectedAddresses
+     */
+    public function addBccToWorksAsExpected($address, $name, array $expectedAddresses): void
+    {
+        $this->subject->addBcc($address, $name);
+        $this->assertCorrectAddresses($this->subject->getBcc(), $expectedAddresses);
+    }
+
+    /**
+     * @test
+     */
+    public function setReadReceiptToToWorksAsExpected(): void
+    {
+        $this->subject->setReadReceiptTo('foo@example.com');
+        $this->assertSame('foo@example.com', $this->subject->getHeaders()->get('Disposition-Notification-To')->getAddress()->getAddress());
+    }
+
+    public function exceptionIsThrownForInvalidArgumentCombinationsDataProvider(): array
+    {
+        return [
+          'setFrom' => ['setFrom'],
+          'setReplyTo' => ['setReplyTo'],
+          'setTo' => ['setTo'],
+          'setCc' => ['setCc'],
+          'setBcc' => ['setBcc'],
+        ];
+    }
+
+    /**
+     * @test
+     * @dataProvider exceptionIsThrownForInvalidArgumentCombinationsDataProvider
+     * @param string $method
+     */
+    public function exceptionIsThrownForInvalidArgumentCombinations(string $method): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionCode(1570543657);
+        $this->subject->{$method}(['foo@example.com'], 'A name');
+    }
+
+    /**
+     * Assert that the correct address data are resolved after setting to the object.
+     * This is a helper method to prevent duplicated code in this test.
+     *
+     * @param array $dataToCheck
+     * @param array $expectedAddresses
+     */
+    protected function assertCorrectAddresses(array $dataToCheck, array $expectedAddresses): void
+    {
+        $this->assertIsArray($dataToCheck);
+        $this->assertCount(count($expectedAddresses), $dataToCheck);
+        foreach ($dataToCheck as $singleAddress) {
+            $this->assertContains($singleAddress->toString(), $expectedAddresses);
+        }
+    }
+}