From 8f44b1f1b4120826dc5c8d71c2f89dadaab90b27 Mon Sep 17 00:00:00 2001 From: bidi Date: Thu, 29 Oct 2026 17:01:48 +0200 Subject: [PATCH] code refactor - low risk Signed-off-by: bidi --- composer.json | 3 ++- src/Email.php | 4 ++- src/Event/MailEventListenerAwareTrait.php | 14 ++++------- src/Factory/AbstractMailFactory.php | 2 +- src/Factory/MailServiceAbstractFactory.php | 9 ++++--- src/Options/MailOptions.php | 6 ++--- src/Options/SmtpOptions.php | 2 +- test/EmailTest.php | 19 ++++++++++++++ test/Event/MailEventListenerTest.php | 19 ++++++++++++++ test/Factory/AbstractMailFactoryTest.php | 25 +++++++++++++++++++ .../MailServiceAbstractFactoryTest.php | 2 ++ test/Options/MailOptionsTest.php | 15 +++++++++++ test/Options/SmtpOptionsTest.php | 12 +++++++++ 13 files changed, 112 insertions(+), 20 deletions(-) diff --git a/composer.json b/composer.json index 033d71b..df90d82 100644 --- a/composer.json +++ b/composer.json @@ -27,11 +27,12 @@ } }, "require": { - "php": "~8.2.0 || ~8.3.0 || ~8.4.0 || ~8.5.0", + "php": "~8.3.0 || ~8.4.0 || ~8.5.0 || ~8.6.0", "ext-fileinfo": "*", "ext-json": "*", "dotkernel/dot-event": "^4.0", "laminas/laminas-servicemanager": "^3.22 || ^4.0", + "laminas/laminas-stdlib": "^3.19", "symfony/mailer": "^v7.1.6" }, "require-dev": { diff --git a/src/Email.php b/src/Email.php index 2712fd6..7640ce7 100644 --- a/src/Email.php +++ b/src/Email.php @@ -398,7 +398,9 @@ private function createAddresses(array $addresses, ?string $name = null): array { $createdAddresses = []; foreach ($addresses as $address) { - $createdAddresses[] = new Address($address, $name ?? ''); + $createdAddresses[] = $address instanceof Address + ? $address + : new Address($address, $name ?? ''); } return $createdAddresses; diff --git a/src/Event/MailEventListenerAwareTrait.php b/src/Event/MailEventListenerAwareTrait.php index 7a182b8..cefa815 100644 --- a/src/Event/MailEventListenerAwareTrait.php +++ b/src/Event/MailEventListenerAwareTrait.php @@ -6,6 +6,8 @@ use Laminas\EventManager\EventManagerAwareTrait; +use function array_search; + trait MailEventListenerAwareTrait { use EventManagerAwareTrait; @@ -22,16 +24,10 @@ public function detachListener(MailEventListenerInterface $listener): void { $listener->detach($this->getEventManager()); - $idx = 0; - foreach ($this->listeners as $l) { - if ($l === $listener) { - break; - } - - $idx++; + $idx = array_search($listener, $this->listeners, true); + if ($idx !== false) { + unset($this->listeners[$idx]); } - - unset($this->listeners[$idx]); } public function clearListeners(): void diff --git a/src/Factory/AbstractMailFactory.php b/src/Factory/AbstractMailFactory.php index 6655558..6ada22a 100644 --- a/src/Factory/AbstractMailFactory.php +++ b/src/Factory/AbstractMailFactory.php @@ -31,7 +31,7 @@ public function canCreate(ContainerInterface $container, $requestedName): bool { $parts = explode('.', $requestedName); - if ($parts[0] !== self::DOT_MAIL_PART || $parts[1] !== static::SPECIFIC_PART || count($parts) !== 3) { + if (count($parts) !== 3 || $parts[0] !== self::DOT_MAIL_PART || $parts[1] !== static::SPECIFIC_PART) { return false; } diff --git a/src/Factory/MailServiceAbstractFactory.php b/src/Factory/MailServiceAbstractFactory.php index 1a82fd9..d104550 100644 --- a/src/Factory/MailServiceAbstractFactory.php +++ b/src/Factory/MailServiceAbstractFactory.php @@ -156,7 +156,7 @@ protected function createTransport(ContainerInterface $container): TransportInte return $this->setupTransportConfig($transport); } else { throw new InvalidArgumentException( - 'Provided mail_adapter service does not return a ' . TransportInterface::class . ' instance' + 'Provided transport service does not return a ' . TransportInterface::class . ' instance' ); } } @@ -169,9 +169,9 @@ protected function createTransport(ContainerInterface $container): TransportInte //the adapter is not valid - throw exception throw new InvalidArgumentException( sprintf( - 'mail_adapter must be an instance of %s or string, "%s" provided', + '"transport" must be a known alias, a container service or a class implementing %s; "%s" provided', TransportInterface::class, - gettype($adapter) + $adapter ) ); } @@ -230,7 +230,8 @@ protected function getListenerObject(ContainerInterface $container, string $type if (! $listener instanceof MailEventListenerInterface) { throw new RuntimeException(sprintf( - 'Mail event listener must be an instance of `%s`, but `%s was given`', + 'Mail event listener "%s" must be an instance of `%s`, but `%s` was given', + $type, MailEventListenerInterface::class, is_object($listener) ? $listener::class : gettype($listener) )); diff --git a/src/Options/MailOptions.php b/src/Options/MailOptions.php index 10153ce..7da067c 100644 --- a/src/Options/MailOptions.php +++ b/src/Options/MailOptions.php @@ -46,9 +46,9 @@ public function getTransport(): string|TransportInterface public function setTransport(string|TransportInterface $transport): void { - if (is_string($transport) && array_key_exists(strtolower($transport), $this->transportMap)) { - $transport = $this->transportMap[$transport]; - foreach ($transport as $class) { + $key = is_string($transport) ? strtolower($transport) : null; + if ($key !== null && array_key_exists($key, $this->transportMap)) { + foreach ($this->transportMap[$key] as $class) { if (class_exists($class)) { $transport = $class; break; diff --git a/src/Options/SmtpOptions.php b/src/Options/SmtpOptions.php index 43f430c..be8ccf0 100644 --- a/src/Options/SmtpOptions.php +++ b/src/Options/SmtpOptions.php @@ -84,7 +84,7 @@ public function setPort(int $port): static { if ($port < 1) { throw new InvalidArgumentException(sprintf( - 'Port must be greater than 1; received "%d"', + 'Port must be greater than 0; received "%d"', $port )); } diff --git a/test/EmailTest.php b/test/EmailTest.php index 629b8d6..3d4f871 100644 --- a/test/EmailTest.php +++ b/test/EmailTest.php @@ -66,4 +66,23 @@ public function testEnsureValidity(): void $this->expectException(LogicException::class); $subject->ensureValidity(); } + + public function testAddressObjectsInArraysAreKept(): void + { + $subject = new Email(); + + $subject->addTo([new Address('first@example.com', 'First'), 'second@example.com'], 'Ignored'); + $subject->setCc([new Address('cc@example.com', 'Copy')]); + + $to = $subject->getTo(); + $this->assertCount(2, $to); + $this->assertSame('first@example.com', $to[0]->getAddress()); + $this->assertSame('First', $to[0]->getName()); + $this->assertSame('second@example.com', $to[1]->getAddress()); + $this->assertSame('Ignored', $to[1]->getName()); + + $cc = $subject->getCc(); + $this->assertCount(1, $cc); + $this->assertSame('Copy', $cc[0]->getName()); + } } diff --git a/test/Event/MailEventListenerTest.php b/test/Event/MailEventListenerTest.php index 3c357e8..fd8c686 100644 --- a/test/Event/MailEventListenerTest.php +++ b/test/Event/MailEventListenerTest.php @@ -43,4 +43,23 @@ public function testListenerDetachingAndClearing(): void $listeners = $listenersProperty->getValue($trait); $this->assertEmpty($listeners); } + + /** + * @throws Exception + * @throws ReflectionException + */ + public function testDetachingUnknownListenerKeepsOthers(): void + { + $trait = new class { + use MailEventListenerAwareTrait; + }; + $attached = $this->createMock(MailEventListenerInterface::class); + $unknown = $this->createMock(MailEventListenerInterface::class); + + $trait->attachListener($attached); + $trait->detachListener($unknown); + + $listenersProperty = (new ReflectionObject($trait))->getProperty('listeners'); + $this->assertSame([$attached], $listenersProperty->getValue($trait)); + } } diff --git a/test/Factory/AbstractMailFactoryTest.php b/test/Factory/AbstractMailFactoryTest.php index 40bc367..319a6b5 100644 --- a/test/Factory/AbstractMailFactoryTest.php +++ b/test/Factory/AbstractMailFactoryTest.php @@ -7,6 +7,7 @@ use Dot\Mail\Factory\AbstractMailFactory; use Dot\Mail\Options\MailOptions; use DotTest\Mail\CommonTrait; +use ErrorException; use org\bovigo\vfs\vfsStream; use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; @@ -14,6 +15,9 @@ use Psr\Container\ContainerInterface; use Psr\Container\NotFoundExceptionInterface; +use function restore_error_handler; +use function set_error_handler; + class AbstractMailFactoryTest extends TestCase { use CommonTrait; @@ -77,4 +81,25 @@ public function testWillNotCreateRequestedNameFromWrongParts(): void $result = $this->subject->canCreate($container, $requestedName); $this->assertFalse($result); } + + /** + * @throws Exception + * @throws ContainerExceptionInterface + * @throws NotFoundExceptionInterface + */ + public function testWillNotCreateRequestedNameWithoutParts(): void + { + $container = $this->createMock(ContainerInterface::class); + + set_error_handler(static function (int $severity, string $message): never { + throw new ErrorException($message, 0, $severity); + }); + + try { + $this->assertFalse($this->subject->canCreate($container, 'dot-mail')); + $this->assertFalse($this->subject->canCreate($container, 'dot-mail.testPart')); + } finally { + restore_error_handler(); + } + } } diff --git a/test/Factory/MailServiceAbstractFactoryTest.php b/test/Factory/MailServiceAbstractFactoryTest.php index 34177f2..0519efb 100644 --- a/test/Factory/MailServiceAbstractFactoryTest.php +++ b/test/Factory/MailServiceAbstractFactoryTest.php @@ -196,6 +196,7 @@ public function testGenerateServiceWithoutProvidedAdapter(): void ->willReturn($this->smtpOptions); $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Mail event listener "Invalid Listener Test" must be an instance of'); $mailService = ($this->subject)($this->container, $requestedName); @@ -226,6 +227,7 @@ public function testGenerateServiceInvalidAdapter(): void ]); $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/"transport" must be a known alias.*"test" provided/'); ($this->subject)($this->container, $requestedName); } diff --git a/test/Options/MailOptionsTest.php b/test/Options/MailOptionsTest.php index 4382364..ab427c7 100644 --- a/test/Options/MailOptionsTest.php +++ b/test/Options/MailOptionsTest.php @@ -9,6 +9,7 @@ use Dot\Mail\Options\MessageOptions; use Dot\Mail\Options\SmtpOptions; use PHPUnit\Framework\TestCase; +use Symfony\Component\Mailer\Transport\SendmailTransport; use Symfony\Component\Mailer\Transport\Smtp\EsmtpTransport; class MailOptionsTest extends TestCase @@ -35,4 +36,18 @@ public function testGettersAndSetters(): void $this->assertContainsOnlyInstancesOf(SmtpOptions::class, [$subject->getSmtpOptions()]); $this->assertSame($eventListeners, $subject->getEventListeners()); } + + public function testTransportAliasIsCaseInsensitive(): void + { + $subject = new MailOptions(); + + $subject->setTransport('ESMTP'); + $this->assertSame(EsmtpTransport::class, $subject->getTransport()); + + $subject->setTransport('SendMail'); + $this->assertSame(SendmailTransport::class, $subject->getTransport()); + + $subject->setTransport('Custom\\Transport'); + $this->assertSame('Custom\\Transport', $subject->getTransport()); + } } diff --git a/test/Options/SmtpOptionsTest.php b/test/Options/SmtpOptionsTest.php index 22f7619..81cdc08 100644 --- a/test/Options/SmtpOptionsTest.php +++ b/test/Options/SmtpOptionsTest.php @@ -29,4 +29,16 @@ public function testSmtpGettersAndSetters(): void $this->expectException(InvalidArgumentException::class); $subject->setPort(0); } + + public function testPortBoundaries(): void + { + $subject = new SmtpOptions(); + + $subject->setPort(1); + $this->assertSame(1, $subject->getPort()); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Port must be greater than 0; received "0"'); + $subject->setPort(0); + } }