feat: added possibility to use Sendgrid, AWS SES, and SMTP for mails#3977
feat: added possibility to use Sendgrid, AWS SES, and SMTP for mails#3977
Conversation
📝 WalkthroughWalkthroughThis PR adds support for multiple mail providers (SMTP, SendGrid, SES, Mailgun) to phpMyFAQ. The Mail class is refactored to support asynchronous delivery via queue with provider-based dispatch, introducing a MailProviderInterface with concrete provider implementations. Configuration, migrations, and documentation are updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Mail
participant Queue
participant SendMailHandler
participant Provider
Client->>Mail: send(forceSynchronousDelivery=false)
alt Queue Available & Not Forced Sync
Mail->>Queue: enqueueForDelivery(recipients, headers, body)
Queue-->>Mail: queued
Mail-->>Client: returns 1
Note over Queue,Provider: Background Processing
Queue->>SendMailHandler: invoke(message with envelope)
SendMailHandler->>Mail: sendPreparedEnvelope(recipients, headers, body)
else Forced Sync or No Queue
Mail->>Mail: createProvider(mail.provider)
Mail->>Provider: send(recipients, headers, body)
Provider-->>Mail: recipient count
Mail-->>Client: returns count
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/development.md`:
- Around line 266-283: There are two problems: the new "### 9.6.4 Background
jobs during development" heading duplicates the later "### 9.6.4 How to get the
phpMyFAQ source code?" heading and an orphaned line "$ git config core.autocrlf"
sits between them; fix by renumbering the later heading and all subsequent 9.6.x
headings (change "### 9.6.4 How to get the phpMyFAQ source code?" → "### 9.6.5"
and bump 9.6.5→9.6.6, etc.), and move the orphaned line "$ git config
core.autocrlf" back into the Git configuration section (the original 9.6.3) or
under its appropriate heading so it is no longer standalone.
In `@phpmyfaq/src/phpMyFAQ/Configuration/MailSettings.php`:
- Around line 26-27: Remove 'mailgun' from the ALLOWED_PROVIDERS constant in
MailSettings.php so it no longer advertises an unimplemented provider;
specifically edit the private const array ALLOWED_PROVIDERS (leaving
DEFAULT_PROVIDER unchanged) to exclude 'mailgun' and keep the existing explicit
check/error in sendPreparedEnvelope() so callers still get the current "Mailgun
provider is not implemented yet." message if used elsewhere.
In `@phpmyfaq/src/phpMyFAQ/Mail.php`:
- Around line 432-434: In send(), when the enqueueForDelivery($recipients,
$this->headers, $this->body) path succeeds, do not unconditionally return 1;
instead compute and return the actual recipient count (e.g., sum of counts of
$this->to, $this->cc and $this->bcc) or return a clear sentinel (e.g., -1) to
indicate queued delivery; update the return expression in the branch that
currently returns 1 inside the send() method (and keep the
$forceSynchronousDelivery check and enqueueForDelivery call as-is) so callers
receive a meaningful value.
- Around line 439-454: Update the PHPDoc for the headers parameter to reflect
that header values can be string or int (not only string): change the `@param` on
sendPreparedEnvelope to array<string, string|int> (matching the class-level doc
and the value set by createHeaders for 'X-Priority'), and make the same change
for the `@param` annotation on sendViaSmtpAgent so both methods accurately
describe header value types.
🧹 Nitpick comments (7)
tests/phpMyFAQ/Mail/Provider/SendGridProviderTest.php (1)
1-61: Tests look good; consider adding a failure-response test.The two tests cover the key paths (missing API key and successful send). You may want to add a test for non-2xx responses from SendGrid to verify the exception message includes the status code and response body, matching the
SendGridProvider::senderror branch at lines 86–93 ofSendGridProvider.php.tests/phpMyFAQ/Mail/Provider/SesProviderTest.php (1)
1-42: Consider adding a success-path test with a mockedSesClient.Unlike
SendGridProviderTest, there is no test that exercises the happy path (valid credentials, successfulsendEmailcall). The constructor already accepts an optional?SesClient $sesClientfor exactly this purpose — you could pass a mockedSesClientto verifysendEmailis called with the expected arguments and that the method returns the correct recipient count.phpmyfaq/src/phpMyFAQ/Mail.php (1)
794-804:createProviderduplicates provider knowledge withsendPreparedEnvelope.Both
sendPreparedEnvelope(line 447) andcreateProvider(line 799) maintain independentmatchexpressions over provider names. If a new provider is added, both must be updated. Consider consolidating:sendPreparedEnvelopecould delegate tocreateProviderfor all non-SMTP providers by havingcreateProviderhandle the 'mailgun' case (throw not-implemented) and the default case.Sketch
public function sendPreparedEnvelope(string $recipients, array $headers, string $body): int { $provider = $this->configuration->getMailProvider(); - return match ($provider) { - 'smtp' => $this->sendViaSmtpAgent($recipients, $headers, $body), - 'sendgrid' => $this->createProvider('sendgrid')->send($recipients, $headers, $body), - 'ses' => $this->createProvider('ses')->send($recipients, $headers, $body), - 'mailgun' => throw new Exception('Mailgun provider is not implemented yet.'), - default => $this->sendViaSmtpAgent($recipients, $headers, $body), - }; + if ($provider === 'smtp' || !in_array($provider, ['sendgrid', 'ses', 'mailgun'], true)) { + return $this->sendViaSmtpAgent($recipients, $headers, $body); + } + + return $this->createProvider($provider)->send($recipients, $headers, $body); } private function createProvider(string $provider): MailProviderInterface { return match ($provider) { 'sendgrid' => new SendGridProvider($this->configuration), 'ses' => new SesProvider($this->configuration), + 'mailgun' => throw new Exception('Mailgun provider is not implemented yet.'), default => throw new Exception('Unsupported mail provider: ' . $provider), }; }phpmyfaq/src/phpMyFAQ/Mail/Provider/SesProvider.php (2)
55-83: BothTextandHtmlbody content are identical.Lines 67–74 set both the plaintext and HTML parts of the SES message to the same
$bodyvalue. If the body contains HTML markup, plaintext email clients will render raw tags; if it's plaintext, the HTML part won't be properly formatted. The same pattern exists inSendGridProvider(lines 73–74).Consider stripping HTML tags for the
Textpart (e.g., viastrip_tags($body)) to provide a proper plaintext fallback.Proposed fix
'Body' => [ 'Text' => [ - 'Data' => $body, + 'Data' => strip_tags($body), 'Charset' => 'UTF-8', ], 'Html' => [
101-108:extractEmailAddressdoes not validate the extracted value is an email.If the input is
<malformed>, this returnsmalformedwithout any validation. TheMail::validateEmailutility exists in the codebase and could be reused here to ensure only valid emails pass through. The same applies toSendGridProvider::extractEmailAddress.phpmyfaq/src/phpMyFAQ/Mail/Provider/SendGridProvider.php (2)
98-121:parseRecipientsandextractEmailAddressare duplicated across providers.These two methods are identical in both
SendGridProviderandSesProvider. Consider extracting them into a shared trait (e.g.,ParsesRecipientsTrait) or a small utility class to keep DRY.Trait sketch
// phpmyfaq/src/phpMyFAQ/Mail/Provider/ParsesRecipientsTrait.php trait ParsesRecipientsTrait { /** `@return` array<int, string> */ private function parseRecipients(string $recipients): array { $addresses = []; foreach (explode(',', $recipients) as $recipient) { $address = $this->extractEmailAddress($recipient); if ($address !== '') { $addresses[] = $address; } } return array_values(array_unique($addresses)); } private function extractEmailAddress(string $rawAddress): string { if (preg_match('/<([^>]+)>/', $rawAddress, $matches) === 1) { return trim($matches[1]); } return trim($rawAddress); } }
72-74: SameText/Htmlbody duplication as noted inSesProvider.Both content types use the raw
$bodyverbatim. Considerstrip_tags($body)for thetext/plainentry so plaintext clients get a readable fallback.Proposed fix
'content' => [ - ['type' => 'text/plain', 'value' => $body], + ['type' => 'text/plain', 'value' => strip_tags($body)], ['type' => 'text/html', 'value' => $body], ],
|
|
||
| ### 9.6.4 Background jobs during development | ||
|
|
||
| phpMyFAQ uses the internal queue for asynchronous tasks such as mail delivery. | ||
| Run the worker locally while testing features that send email: | ||
|
|
||
| ```bash | ||
| php bin/worker.php | ||
| ``` | ||
|
|
||
| Optionally limit processing to a fixed number of jobs: | ||
|
|
||
| ```bash | ||
| php bin/worker.php 10 | ||
| ``` | ||
| $ git config core.autocrlf # if you're on Windows | ||
|
|
||
| ### 9.6.4 How to get the phpMyFAQ source code? |
There was a problem hiding this comment.
Duplicate section number and orphaned line.
Two issues:
- Duplicate heading: The new section at line 267 and the existing section at line 283 are both numbered
### 9.6.4. The subsequent sections (9.6.5–9.6.9) need to be renumbered as well. - Orphaned content: Line 281 (
$ git config core.autocrlf) was previously part of the Git configuration section (9.6.3) but now sits between the new 9.6.4 and the old 9.6.4 without a heading, making it look detached.
Proposed fix
$ git config --global user.email you@example.com
+ $ git config core.autocrlf # if you're on Windows
-### 9.6.4 Background jobs during development
+### 9.6.4 Background jobs during development
phpMyFAQ uses the internal queue for asynchronous tasks such as mail delivery.
Run the worker locally while testing features that send email:
```bash
php bin/worker.phpOptionally limit processing to a fixed number of jobs:
php bin/worker.php 10- $ git config core.autocrlf # if you're on Windows
-### 9.6.4 How to get the phpMyFAQ source code?
+### 9.6.5 How to get the phpMyFAQ source code?
And renumber all subsequent sections (9.6.5 → 9.6.6, etc.).
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 272-272: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
---
[warning] 278-278: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
---
[warning] 281-281: Dollar signs used before commands without showing output
(MD014, commands-show-output)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/development.md around lines 266 - 283, There are two problems: the new
"### 9.6.4 Background jobs during development" heading duplicates the later "###
9.6.4 How to get the phpMyFAQ source code?" heading and an orphaned line "$ git
config core.autocrlf" sits between them; fix by renumbering the later heading
and all subsequent 9.6.x headings (change "### 9.6.4 How to get the phpMyFAQ
source code?" → "### 9.6.5" and bump 9.6.5→9.6.6, etc.), and move the orphaned
line "$ git config core.autocrlf" back into the Git configuration section (the
original 9.6.3) or under its appropriate heading so it is no longer standalone.
</details>
<!-- fingerprinting:phantom:poseidon:phoenix -->
<!-- This is an auto-generated comment by CodeRabbit -->
| private const string DEFAULT_PROVIDER = 'smtp'; | ||
| private const array ALLOWED_PROVIDERS = ['smtp', 'sendgrid', 'ses', 'mailgun']; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the createProvider method in Mail.php
fd -t f "Mail.php" phpmyfaq/src/phpMyFAQ/ | head -5Repository: thorsten/phpMyFAQ
Length of output: 91
🏁 Script executed:
# Check the structure of the codebase
ls -la phpmyfaq/src/phpMyFAQ/ | head -20Repository: thorsten/phpMyFAQ
Length of output: 1221
🏁 Script executed:
# Read the Mail.php file to examine createProvider method
wc -l phpmyfaq/src/phpMyFAQ/Mail.phpRepository: thorsten/phpMyFAQ
Length of output: 95
🏁 Script executed:
# Read MailSettings.php to confirm ALLOWED_PROVIDERS
cat -n phpmyfaq/src/phpMyFAQ/Configuration/MailSettings.phpRepository: thorsten/phpMyFAQ
Length of output: 2012
🏁 Script executed:
# Search for createProvider method in Mail.php
rg -n "createProvider" phpmyfaq/src/phpMyFAQ/Mail.phpRepository: thorsten/phpMyFAQ
Length of output: 333
🏁 Script executed:
# Search for provider handling logic
rg -n "match\|switch" phpmyfaq/src/phpMyFAQ/Mail.php -A 20 | head -50Repository: thorsten/phpMyFAQ
Length of output: 43
🏁 Script executed:
# Read the createProvider method
sed -n '797,836p' phpmyfaq/src/phpMyFAQ/Mail.php | cat -nRepository: thorsten/phpMyFAQ
Length of output: 1728
🏁 Script executed:
# Also check the broader send method to understand the flow (around line 449)
sed -n '430,470p' phpmyfaq/src/phpMyFAQ/Mail.php | cat -nRepository: thorsten/phpMyFAQ
Length of output: 1995
Remove mailgun from ALLOWED_PROVIDERS until implementation is complete.
mailgun is listed in ALLOWED_PROVIDERS in MailSettings.php but is not implemented. While it is explicitly caught in sendPreparedEnvelope() with a clear error message ('Mailgun provider is not implemented yet.'), including it in the allowed providers list creates a false expectation of availability. Remove it from the array until the provider is fully implemented.
🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Configuration/MailSettings.php` around lines 26 - 27,
Remove 'mailgun' from the ALLOWED_PROVIDERS constant in MailSettings.php so it
no longer advertises an unimplemented provider; specifically edit the private
const array ALLOWED_PROVIDERS (leaving DEFAULT_PROVIDER unchanged) to exclude
'mailgun' and keep the existing explicit check/error in sendPreparedEnvelope()
so callers still get the current "Mailgun provider is not implemented yet."
message if used elsewhere.
| if (!$forceSynchronousDelivery && $this->enqueueForDelivery($recipients, $this->headers, $this->body)) { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Enqueue path always returns 1 regardless of actual recipient count.
When the mail is enqueued successfully, send() returns 1, but the synchronous paths return the actual number of recipients. Callers relying on the return value to know how many recipients were addressed will get an inaccurate result for queued mails.
Consider returning the actual recipient count (e.g., count($this->to) + count($this->cc) + count($this->bcc)) or a sentinel value that clearly indicates the mail was queued rather than sent.
🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Mail.php` around lines 432 - 434, In send(), when the
enqueueForDelivery($recipients, $this->headers, $this->body) path succeeds, do
not unconditionally return 1; instead compute and return the actual recipient
count (e.g., sum of counts of $this->to, $this->cc and $this->bcc) or return a
clear sentinel (e.g., -1) to indicate queued delivery; update the return
expression in the branch that currently returns 1 inside the send() method (and
keep the $forceSynchronousDelivery check and enqueueForDelivery call as-is) so
callers receive a meaningful value.
| /** | ||
| * @param array<string, string> $headers | ||
| * @throws Exception|TransportExceptionInterface | ||
| */ | ||
| public function sendPreparedEnvelope(string $recipients, array $headers, string $body): int | ||
| { | ||
| $provider = $this->configuration->getMailProvider(); | ||
|
|
||
| return match ($provider) { | ||
| 'smtp' => $this->sendViaSmtpAgent($recipients, $headers, $body), | ||
| 'sendgrid' => $this->createProvider('sendgrid')->send($recipients, $headers, $body), | ||
| 'ses' => $this->createProvider('ses')->send($recipients, $headers, $body), | ||
| 'mailgun' => throw new Exception('Mailgun provider is not implemented yet.'), | ||
| default => $this->sendViaSmtpAgent($recipients, $headers, $body), | ||
| }; | ||
| } |
There was a problem hiding this comment.
PHPDoc type for $headers doesn't match actual header values.
The @param array<string, string> annotation declares all header values as strings, but createHeaders() stores an int value at $this->headers['X-Priority'] (line 552). The class-level doc (line 92) already acknowledges array<string|int>. The same mismatch applies to the sendViaSmtpAgent PHPDoc at line 720.
Proposed fix
/**
- * `@param` array<string, string> $headers
+ * `@param` array<string, string|int> $headers
* `@throws` Exception|TransportExceptionInterface
*/
public function sendPreparedEnvelope(string $recipients, array $headers, string $body): intAnd similarly for sendViaSmtpAgent:
/**
- * `@param` array<string, string> $headers
+ * `@param` array<string, string|int> $headers
* `@throws` Exception|TransportExceptionInterface
*/
private function sendViaSmtpAgent(string $recipients, array $headers, string $body): int🤖 Prompt for AI Agents
In `@phpmyfaq/src/phpMyFAQ/Mail.php` around lines 439 - 454, Update the PHPDoc for
the headers parameter to reflect that header values can be string or int (not
only string): change the `@param` on sendPreparedEnvelope to array<string,
string|int> (matching the class-level doc and the value set by createHeaders for
'X-Priority'), and make the same change for the `@param` annotation on
sendViaSmtpAgent so both methods accurately describe header value types.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation