Skip to content

Commit 33a07cc

Browse files
ChiragAgg5kChiragAgg5k
committed
fix: address code review findings
- Unify normalization: cc/bcc now go through normalizeRecipient just like to, so all three fields accept both plain strings and arrays - Reject empty email strings in normalizeRecipient (was only checking isset, which passes for empty strings) - Remove separate validateRecipients method (redundant with unified normalizeRecipient) - Fix Mailgun from/reply-to format to use RFC 5322 space before < - Fix SMTP null guard on getCC()/getBCC() in result-reporting loop - Cache getTo() result in Mailgun to avoid redundant calls - Add tests for named to recipients, mixed string/array inputs, cc/bcc string normalization, and validation rejection of empty emails and missing email keys
1 parent b293444 commit 33a07cc

4 files changed

Lines changed: 151 additions & 30 deletions

File tree

‎src/Utopia/Messaging/Adapter/Email/Mailgun.php‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,23 +51,24 @@ protected function process(EmailMessage $message): array
5151

5252
$domain = $this->isEU ? $euDomain : $usDomain;
5353

54-
$toEmails = \array_map(fn ($to) => $to['email'], $message->getTo());
54+
$recipients = $message->getTo();
55+
$toEmails = \array_map(fn ($to) => $to['email'], $recipients);
5556

5657
$body = [
5758
'to' => \implode(',', \array_map(
5859
fn ($to) => !empty($to['name'])
5960
? "{$to['name']} <{$to['email']}>"
6061
: $to['email'],
61-
$message->getTo()
62+
$recipients
6263
)),
63-
'from' => "{$message->getFromName()}<{$message->getFromEmail()}>",
64+
'from' => "{$message->getFromName()} <{$message->getFromEmail()}>",
6465
'subject' => $message->getSubject(),
6566
'text' => $message->isHtml() ? null : $message->getContent(),
6667
'html' => $message->isHtml() ? $message->getContent() : null,
67-
'h:Reply-To: '."{$message->getReplyToName()}<{$message->getReplyToEmail()}>",
68+
'h:Reply-To: '."{$message->getReplyToName()} <{$message->getReplyToEmail()}>",
6869
];
6970

70-
if (\count($message->getTo()) > 1) {
71+
if (\count($recipients) > 1) {
7172
$body['recipient-variables'] = json_encode(array_fill_keys($toEmails, []));
7273
}
7374

‎src/Utopia/Messaging/Adapter/Email/SMTP.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,15 +161,15 @@ protected function process(EmailMessage $message): array
161161
$response->addResult($to['email'], $sent ? '' : $error);
162162
}
163163

164-
foreach ($message->getCC() as $cc) {
164+
foreach ($message->getCC() ?? [] as $cc) {
165165
$error = empty($mail->ErrorInfo)
166166
? 'Unknown error'
167167
: $mail->ErrorInfo;
168168

169169
$response->addResult($cc['email'], $sent ? '' : $error);
170170
}
171171

172-
foreach ($message->getBCC() as $bcc) {
172+
foreach ($message->getBCC() ?? [] as $bcc) {
173173
$error = empty($mail->ErrorInfo)
174174
? 'Unknown error'
175175
: $mail->ErrorInfo;

‎src/Utopia/Messaging/Messages/Email.php‎

Lines changed: 10 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ class Email implements Message
3030
* @param string $fromEmail The email address of the sender.
3131
* @param string|null $replyToName The name of the reply to.
3232
* @param string|null $replyToEmail The email address of the reply to.
33-
* @param array<array<string,string>>|null $cc The CC recipients of the email. Each recipient should be an array containing an "email" and optional "name" key.
34-
* @param array<array<string,string>>|null $bcc The BCC recipients of the email. Each recipient should be an array containing an "email" and optional "name" key.
33+
* @param array<string|array<string,string>>|null $cc The CC recipients of the email. Same format as $to.
34+
* @param array<string|array<string,string>>|null $bcc The BCC recipients of the email. Same format as $to.
3535
* @param array<Attachment>|null $attachments The attachments of the email.
3636
* @param bool $html Whether the message is HTML or not.
3737
*/
@@ -49,8 +49,8 @@ public function __construct(
4949
private bool $html = false,
5050
) {
5151
$this->to = \array_map([self::class, 'normalizeRecipient'], $to);
52-
$this->cc = !\is_null($cc) ? self::validateRecipients($cc) : null;
53-
$this->bcc = !\is_null($bcc) ? self::validateRecipients($bcc) : null;
52+
$this->cc = !\is_null($cc) ? \array_map([self::class, 'normalizeRecipient'], $cc) : null;
53+
$this->bcc = !\is_null($bcc) ? \array_map([self::class, 'normalizeRecipient'], $bcc) : null;
5454

5555
if (\is_null($this->replyToName)) {
5656
$this->replyToName = $this->fromName;
@@ -70,33 +70,20 @@ public function __construct(
7070
private static function normalizeRecipient(string|array $value): array
7171
{
7272
if (\is_string($value)) {
73+
if ($value === '') {
74+
throw new \InvalidArgumentException('Recipient email must not be empty.');
75+
}
76+
7377
return ['email' => $value];
7478
}
7579

76-
if (!isset($value['email'])) {
77-
throw new \InvalidArgumentException('Each recipient must have at least an "email" key.');
80+
if (!isset($value['email']) || $value['email'] === '') {
81+
throw new \InvalidArgumentException('Each recipient must have a non-empty "email" key.');
7882
}
7983

8084
return $value;
8185
}
8286

83-
/**
84-
* Validate an array of recipients.
85-
*
86-
* @param array<array<string,string>> $recipients
87-
* @return array<array<string,string>>
88-
*/
89-
private static function validateRecipients(array $recipients): array
90-
{
91-
foreach ($recipients as $recipient) {
92-
if (!isset($recipient['email'])) {
93-
throw new \InvalidArgumentException('Each recipient must have at least an "email" key.');
94-
}
95-
}
96-
97-
return $recipients;
98-
}
99-
10087
/**
10188
* @return array<array<string,string>>
10289
*/

‎tests/Messaging/Adapter/Email/EmailTest.php‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,4 +43,137 @@ public function testSendEmail(): void
4343
$this->assertEquals($cc[0]['email'], $lastEmail['cc'][0]['address']);
4444
$this->assertEquals($bcc[0]['email'], $lastEmail['envelope']['to'][2]['address']);
4545
}
46+
47+
public function testSendEmailWithNamedToRecipient(): void
48+
{
49+
$sender = new Mock();
50+
51+
$message = new Email(
52+
to: [['email' => 'tester@localhost.test', 'name' => 'Test User']],
53+
subject: 'Named To Test',
54+
content: 'Test Content',
55+
fromName: 'Test Sender',
56+
fromEmail: 'sender@localhost.test',
57+
);
58+
59+
$response = $sender->send($message);
60+
61+
$lastEmail = $this->getLastEmail();
62+
63+
$this->assertResponse($response);
64+
$this->assertEquals('tester@localhost.test', $lastEmail['to'][0]['address']);
65+
$this->assertEquals('Test User', $lastEmail['to'][0]['name']);
66+
}
67+
68+
public function testSendEmailWithMixedToFormats(): void
69+
{
70+
$sender = new Mock();
71+
72+
$message = new Email(
73+
to: [
74+
'plain@localhost.test',
75+
['email' => 'named@localhost.test', 'name' => 'Named User'],
76+
],
77+
subject: 'Mixed To Test',
78+
content: 'Test Content',
79+
fromName: 'Test Sender',
80+
fromEmail: 'sender@localhost.test',
81+
);
82+
83+
$response = $sender->send($message);
84+
85+
$this->assertResponse($response);
86+
87+
// Verify both recipients are normalized to array format
88+
$to = $message->getTo();
89+
$this->assertEquals('plain@localhost.test', $to[0]['email']);
90+
$this->assertArrayNotHasKey('name', $to[0]);
91+
$this->assertEquals('named@localhost.test', $to[1]['email']);
92+
$this->assertEquals('Named User', $to[1]['name']);
93+
}
94+
95+
public function testCcAcceptsPlainStrings(): void
96+
{
97+
$message = new Email(
98+
to: ['tester@localhost.test'],
99+
subject: 'CC String Test',
100+
content: 'Test Content',
101+
fromName: 'Test Sender',
102+
fromEmail: 'sender@localhost.test',
103+
cc: ['cc@localhost.test'],
104+
);
105+
106+
$cc = $message->getCC();
107+
$this->assertNotNull($cc);
108+
$this->assertEquals('cc@localhost.test', $cc[0]['email']);
109+
}
110+
111+
public function testBccAcceptsPlainStrings(): void
112+
{
113+
$message = new Email(
114+
to: ['tester@localhost.test'],
115+
subject: 'BCC String Test',
116+
content: 'Test Content',
117+
fromName: 'Test Sender',
118+
fromEmail: 'sender@localhost.test',
119+
bcc: ['bcc@localhost.test'],
120+
);
121+
122+
$bcc = $message->getBCC();
123+
$this->assertNotNull($bcc);
124+
$this->assertEquals('bcc@localhost.test', $bcc[0]['email']);
125+
}
126+
127+
public function testRejectsEmptyEmailString(): void
128+
{
129+
$this->expectException(\InvalidArgumentException::class);
130+
131+
new Email(
132+
to: [''],
133+
subject: 'Test',
134+
content: 'Test',
135+
fromName: 'Test',
136+
fromEmail: 'sender@localhost.test',
137+
);
138+
}
139+
140+
public function testRejectsEmptyEmailInArray(): void
141+
{
142+
$this->expectException(\InvalidArgumentException::class);
143+
144+
new Email(
145+
to: [['email' => '', 'name' => 'Ghost']],
146+
subject: 'Test',
147+
content: 'Test',
148+
fromName: 'Test',
149+
fromEmail: 'sender@localhost.test',
150+
);
151+
}
152+
153+
public function testRejectsMissingEmailKey(): void
154+
{
155+
$this->expectException(\InvalidArgumentException::class);
156+
157+
new Email(
158+
to: [['name' => 'No Email']],
159+
subject: 'Test',
160+
content: 'Test',
161+
fromName: 'Test',
162+
fromEmail: 'sender@localhost.test',
163+
);
164+
}
165+
166+
public function testRejectsEmptyEmailInCc(): void
167+
{
168+
$this->expectException(\InvalidArgumentException::class);
169+
170+
new Email(
171+
to: ['valid@localhost.test'],
172+
subject: 'Test',
173+
content: 'Test',
174+
fromName: 'Test',
175+
fromEmail: 'sender@localhost.test',
176+
cc: [''],
177+
);
178+
}
46179
}

0 commit comments

Comments
 (0)