Add missing endpoints#65
Conversation
📝 WalkthroughWalkthroughThis PR introduces a multi-feature SDK expansion adding webhook management (list/get/create/update/delete), account API token operations (list/get/create/reset/delete), organization-scoped sub-account management (list/create), and new sandbox message methods (getMailHeaders/forward). Permissions serialization is refactored to enforce validation in the DTO. All changes include comprehensive test coverage and example scripts. ChangesMailtrap SDK Feature Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
IgorDobryn
changed the title
IgorDobryn
force-pushed
the
MT-21863-php-sdk-add-webhooks-api-tokens-and-sub-accounts-endpoints
branch
from
d240511 to
743ef96
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/api-tokens/all.php`:
- Around line 12-14: Add a guard that checks the required environment variables
before using them: validate $_ENV['MAILTRAP_ACCOUNT_ID'] and
$_ENV['MAILTRAP_API_KEY'] and throw/exit with a clear error message if either is
missing so the example fails fast; update the bootstrap around $accountId, the
Config(...) call, and the MailtrapGeneralClient(...)->apiTokens(...) usage to
rely on the validated values (or sanitized variables) rather than accessing
$_ENV directly.
In `@examples/webhooks/all.php`:
- Around line 12-14: The example currently reads $_ENV['MAILTRAP_ACCOUNT_ID']
and $_ENV['MAILTRAP_API_KEY'] directly which can be null and emit notices; add a
small bootstrap guard before creating Config and calling (new
MailtrapSendingClient($config))->webhooks($accountId) that checks required env
vars (MAILTRAP_ACCOUNT_ID and MAILTRAP_API_KEY), optionally MAILTRAP_DOMAIN_ID
if domain-scoped webhooks are intended, and exits with a clear error message
when any are missing so Config is never constructed with null values and the
script fails fast and informatively.
In `@src/DTO/Request/Webhook/CreateWebhook.php`:
- Around line 21-37: CreateWebhook currently allows building invalid payloads
for type-specific webhooks; add validation in the CreateWebhook constructor (or
in toArray) to enforce required fields based on $webhookType (e.g., when
$webhookType === 'email_sending' require non-null $sendingStream and non-empty
$eventTypes), throw a clear exception (InvalidArgumentException) on violation,
or alternatively refactor by splitting CreateWebhook into separate DTOs per
webhook kind (e.g., EmailSendingWebhook with required $sendingStream and
EventTypes) and use the appropriate DTO where needed; update
CreateWebhook::toArray to assume validated fields so it cannot serialize invalid
requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60e92d97-9770-4f7f-abc9-d3da6787856a
📒 Files selected for processing (24)
.envrcCHANGELOG.mdexamples/api-tokens/all.phpexamples/sub-accounts/all.phpexamples/testing/messages.phpexamples/webhooks/all.phpsrc/Api/General/ApiToken.phpsrc/Api/Organization/OrganizationInterface.phpsrc/Api/Organization/SubAccount.phpsrc/Api/Sandbox/Message.phpsrc/Api/Sending/Webhook.phpsrc/DTO/Request/Webhook/CreateWebhook.phpsrc/DTO/Request/Webhook/UpdateWebhook.phpsrc/DTO/Request/Webhook/WebhookInterface.phpsrc/MailtrapGeneralClient.phpsrc/MailtrapOrganizationClient.phpsrc/MailtrapSendingClient.phptests/Api/General/ApiTokenTest.phptests/Api/Organization/SubAccountTest.phptests/Api/Sandbox/MessageTest.phptests/Api/Sending/WebhookTest.phptests/MailtrapGeneralClientTest.phptests/MailtrapOrganizationClientTest.phptests/MailtrapSendingClientTest.php
| $accountId = $_ENV['MAILTRAP_ACCOUNT_ID']; | ||
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens | ||
| $apiTokens = (new MailtrapGeneralClient($config))->apiTokens($accountId); |
There was a problem hiding this comment.
Fail fast when required example env vars are missing.
$_ENV['MAILTRAP_ACCOUNT_ID'] and $_ENV['MAILTRAP_API_KEY'] are accessed directly, so a missing environment setup will surface as notices or a later client error instead of a clear bootstrap failure. A small guard here would make the example much easier to run.
♻️ Suggested bootstrap guard
require __DIR__ . '/../../vendor/autoload.php';
+foreach (['MAILTRAP_ACCOUNT_ID', 'MAILTRAP_API_KEY'] as $envVar) {
+ if (empty($_ENV[$envVar])) {
+ throw new RuntimeException(sprintf('%s must be set for this example.', $envVar));
+ }
+}
+
$accountId = $_ENV['MAILTRAP_ACCOUNT_ID'];
$config = new Config($_ENV['MAILTRAP_API_KEY']); `#your` API token from here https://mailtrap.io/api-tokens🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/api-tokens/all.php` around lines 12 - 14, Add a guard that checks
the required environment variables before using them: validate
$_ENV['MAILTRAP_ACCOUNT_ID'] and $_ENV['MAILTRAP_API_KEY'] and throw/exit with a
clear error message if either is missing so the example fails fast; update the
bootstrap around $accountId, the Config(...) call, and the
MailtrapGeneralClient(...)->apiTokens(...) usage to rely on the validated values
(or sanitized variables) rather than accessing $_ENV directly.
| $accountId = $_ENV['MAILTRAP_ACCOUNT_ID']; | ||
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens | ||
| $webhooks = (new MailtrapSendingClient($config))->webhooks($accountId); |
There was a problem hiding this comment.
Fail fast when required example env vars are missing.
Reading $_ENV['MAILTRAP_ACCOUNT_ID'] / $_ENV['MAILTRAP_API_KEY'] directly will emit notices and hand null to Config when the example is run without setup. A small bootstrap guard would make the script much more robust. If you want this sample to demonstrate domain-scoped webhooks, consider documenting MAILTRAP_DOMAIN_ID alongside the other env vars as well.
♻️ Suggested bootstrap guard
<?php
use Mailtrap\Config;
@@
require __DIR__ . '/../../vendor/autoload.php';
+foreach (['MAILTRAP_ACCOUNT_ID', 'MAILTRAP_API_KEY'] as $envVar) {
+ if (empty($_ENV[$envVar])) {
+ throw new RuntimeException(sprintf('%s must be set for this example.', $envVar));
+ }
+}
+
$accountId = $_ENV['MAILTRAP_ACCOUNT_ID'];
$config = new Config($_ENV['MAILTRAP_API_KEY']); `#your` API token from here https://mailtrap.io/api-tokens📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $accountId = $_ENV['MAILTRAP_ACCOUNT_ID']; | |
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens | |
| $webhooks = (new MailtrapSendingClient($config))->webhooks($accountId); | |
| <?php | |
| use Mailtrap\Config; | |
| use Mailtrap\MailtrapSendingClient; | |
| require __DIR__ . '/../../vendor/autoload.php'; | |
| foreach (['MAILTRAP_ACCOUNT_ID', 'MAILTRAP_API_KEY'] as $envVar) { | |
| if (empty($_ENV[$envVar])) { | |
| throw new RuntimeException(sprintf('%s must be set for this example.', $envVar)); | |
| } | |
| } | |
| $accountId = $_ENV['MAILTRAP_ACCOUNT_ID']; | |
| $config = new Config($_ENV['MAILTRAP_API_KEY']); `#your` API token from here https://mailtrap.io/api-tokens | |
| $webhooks = (new MailtrapSendingClient($config))->webhooks($accountId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/webhooks/all.php` around lines 12 - 14, The example currently reads
$_ENV['MAILTRAP_ACCOUNT_ID'] and $_ENV['MAILTRAP_API_KEY'] directly which can be
null and emit notices; add a small bootstrap guard before creating Config and
calling (new MailtrapSendingClient($config))->webhooks($accountId) that checks
required env vars (MAILTRAP_ACCOUNT_ID and MAILTRAP_API_KEY), optionally
MAILTRAP_DOMAIN_ID if domain-scoped webhooks are intended, and exits with a
clear error message when any are missing so Config is never constructed with
null values and the script fails fast and informatively.
| * | ||
| * Class MailtrapOrganizationClient | ||
| */ | ||
| final class MailtrapOrganizationClient extends AbstractMailtrapClient |
There was a problem hiding this comment.
Move SubAccount into MailtrapGeneralClient instead of introducing a new client
Looking at Mailtrap's own UI, Organizations lives in the ACCOUNT MANAGEMENT group alongside Accounts, API Tokens, Permissions, and Billing — all of which are exposed through MailtrapGeneralClient. To keep the SDK aligned with that grouping, and to make ownership explicit (sub-accounts belong to an organization, not directly to the general client), I'd suggest introducing an intermediate organizations layer rather than exposing subAccounts at the top level.
DX would read:
(new MailtrapGeneralClient($config))
->organizations($organizationId)
->subAccounts()
->getSubAccounts();
// src/MailtrapGeneralClient.php
public const API_MAPPING = [
'accounts' => Api\General\Account::class,
'users' => Api\General\User::class,
'permissions' => Api\General\Permission::class,
'contacts' => Api\General\Contact::class,
'emailTemplates' => Api\General\EmailTemplate::class,
'billing' => Api\General\Billing::class,
'apiTokens' => Api\General\ApiToken::class,
'organizations' => Api\General\Organization::class, // <-- new entry point
];
| private function getPermissionsPayload(Permissions $permissions): array | ||
| { | ||
| $payload = []; | ||
| foreach ($permissions->getAll() as $permission) { | ||
| $payload[] = $permission->toArray(); | ||
| } | ||
|
|
||
| if (count($payload) === 0) { | ||
| throw new RuntimeException('At least one "permission" object should be added to create an API token'); | ||
| } | ||
|
|
||
| return $payload; | ||
| } | ||
| } |
There was a problem hiding this comment.
duplicates Permission::getPayload()
ApiToken::getPermissionsPayload() is essentially identical to Permission::getPayload() — same loop, same count() guard, same RuntimeException. Could we lift the conversion onto the DTO itself so any future caller benefits?
// src/DTO/Request/Permission/Permissions.php
public function toPayload(): array
{
$payload = array_map(fn($p) => $p->toArray(), $this->getAll());
if ($payload === []) {
throw new RuntimeException('At least one "permission" object must be provided');
}
return $payload;
}
Then both Permission::update() and ApiToken::createApiToken() call $permissions->toPayload() and the private duplicates go away.
|
|
||
| use Mailtrap\DTO\Request\RequestInterface; | ||
|
|
||
| interface WebhookInterface extends RequestInterface |
There was a problem hiding this comment.
WebhookInterface mixes a DTO marker with domain constants
WebhookInterface extends RequestInterface (a DTO contract) but also carries the entire webhook vocabulary (TYPE_, EVENT_, PAYLOAD_FORMAT_, SENDING_STREAM_). End-user code in examples/webhooks/all.php has to import a DTO marker just to reach WebhookInterface::EVENT_DELIVERY, and adding a new event type becomes an interface change.
Could we keep WebhookInterface as a tiny marker (no constants) and move the constants into a dedicated holder, e.g. a Webhook enum/constants class? Users would then write Webhook::EVENT_DELIVERY which reads more naturally.
| * Only `url`, `active`, `payload_format`, and `event_types` can be updated after creation. | ||
| * `webhook_type`, `sending_mode`, and `mailsend_domain_id` are immutable. |
There was a problem hiding this comment.
sending_mode and mailsend_domain_id don't appear anywhere in the codebase — CreateWebhook and the API use sending_stream and domain_id.
| * @param int|null $domainId Scope to a specific domain id (null = all account domains) | ||
| * @param bool|null $active Defaults to true on the server side | ||
| */ | ||
| public function __construct( |
There was a problem hiding this comment.
The constructor docblock says eventTypes and sendingStream are required for TYPE_EMAIL_SENDING, but both default to empty/null with no runtime check. Users only learn about the constraint via a 422 from the server — exactly the validation a typed DTO should catch:
if ($webhookType === WebhookInterface::TYPE_EMAIL_SENDING) {
if ($eventTypes === []) {
throw new InvalidArgumentException('"eventTypes" is required for email_sending webhooks');
}
if ($sendingStream === null) {
throw new InvalidArgumentException('"sendingStream" is required for email_sending webhooks');
}
}
It would also be worth validating webhookType against TYPE_EMAIL_SENDING / TYPE_AUDIT_LOG.
…class and updating related tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/webhooks/all.php (1)
12-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fail-fast guards for required env vars before constructing the client.
On Line 12 and Line 13, direct
$_ENV[...]access can produce notices and pass null intoConfigwhen setup is incomplete. This was already flagged and is still unresolved.Proposed fix
require __DIR__ . '/../../vendor/autoload.php'; +foreach (['MAILTRAP_ACCOUNT_ID', 'MAILTRAP_API_KEY'] as $envVar) { + if (empty($_ENV[$envVar])) { + throw new RuntimeException(sprintf('%s must be set for this example.', $envVar)); + } +} + $accountId = $_ENV['MAILTRAP_ACCOUNT_ID']; $config = new Config($_ENV['MAILTRAP_API_KEY']); `#your` API token from here https://mailtrap.io/api-tokens🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/webhooks/all.php` around lines 12 - 14, Add fail-fast checks for the required env vars before constructing Config and MailtrapSendingClient: validate $_ENV['MAILTRAP_ACCOUNT_ID'] and $_ENV['MAILTRAP_API_KEY'] are set and non-empty (e.g., using isset() and !== ''), and if not, emit a clear error and exit/throw so null isn't passed into new Config($apiKey). Update the block around $accountId, new Config(...), and (new MailtrapSendingClient(...))->webhooks(...) to perform these guards first and only construct Config and MailtrapSendingClient when the checks pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/sub-accounts/all.php`:
- Around line 9-11: The code reads $_ENV values directly and may fail when vars
are missing; before constructing Config and calling
MailtrapGeneralClient->organization(), validate that MAILTRAP_API_KEY and
MAILTRAP_ORGANIZATION_ID are present, fail fast with a clear error/exception if
they are not, and normalize MAILTRAP_ORGANIZATION_ID to an integer (e.g., via
intval() or (int) cast) when assigning $organizationId so the subsequent call to
MailtrapGeneralClient($config)->organization($organizationId)->subAccounts()
receives a validated integer ID.
---
Duplicate comments:
In `@examples/webhooks/all.php`:
- Around line 12-14: Add fail-fast checks for the required env vars before
constructing Config and MailtrapSendingClient: validate
$_ENV['MAILTRAP_ACCOUNT_ID'] and $_ENV['MAILTRAP_API_KEY'] are set and non-empty
(e.g., using isset() and !== ''), and if not, emit a clear error and exit/throw
so null isn't passed into new Config($apiKey). Update the block around
$accountId, new Config(...), and (new MailtrapSendingClient(...))->webhooks(...)
to perform these guards first and only construct Config and
MailtrapSendingClient when the checks pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9264dc17-1ce2-4eee-ae1d-142bac6e15b6
📒 Files selected for processing (18)
CHANGELOG.mdexamples/sub-accounts/all.phpexamples/webhooks/all.phpsrc/Api/General/ApiToken.phpsrc/Api/General/Organization.phpsrc/Api/General/Permission.phpsrc/DTO/Request/Permission/Permissions.phpsrc/DTO/Request/Webhook/CreateWebhook.phpsrc/DTO/Request/Webhook/UpdateWebhook.phpsrc/DTO/Request/Webhook/Webhook.phpsrc/DTO/Request/Webhook/WebhookInterface.phpsrc/MailtrapGeneralClient.phptests/Api/General/ApiTokenTest.phptests/Api/General/PermissionTest.phptests/Api/Organization/SubAccountTest.phptests/Api/Sending/WebhookTest.phptests/MailtrapGeneralClientTest.phptests/MailtrapTestCase.php
✅ Files skipped from review due to trivial changes (2)
- tests/MailtrapGeneralClientTest.php
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/Api/Organization/SubAccountTest.php
- src/DTO/Request/Webhook/UpdateWebhook.php
- tests/Api/General/ApiTokenTest.php
- tests/Api/Sending/WebhookTest.php
- src/MailtrapGeneralClient.php
| $organizationId = $_ENV['MAILTRAP_ORGANIZATION_ID']; | ||
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens | ||
| $subAccounts = (new MailtrapGeneralClient($config))->organization($organizationId)->subAccounts(); |
There was a problem hiding this comment.
Guard required env vars before client construction.
Direct $_ENV[...] reads here can fail noisily when vars are missing; add presence checks and normalize MAILTRAP_ORGANIZATION_ID to an integer.
Suggested patch
-$organizationId = $_ENV['MAILTRAP_ORGANIZATION_ID'];
-$config = new Config($_ENV['MAILTRAP_API_KEY']); `#your` API token from here https://mailtrap.io/api-tokens
+$apiKey = $_ENV['MAILTRAP_API_KEY'] ?? null;
+$organizationIdRaw = $_ENV['MAILTRAP_ORGANIZATION_ID'] ?? null;
+if (!$apiKey || !$organizationIdRaw || !ctype_digit((string) $organizationIdRaw)) {
+ throw new RuntimeException('Set MAILTRAP_API_KEY and numeric MAILTRAP_ORGANIZATION_ID in environment.');
+}
+$organizationId = (int) $organizationIdRaw;
+$config = new Config($apiKey); // your API token from https://mailtrap.io/api-tokens📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $organizationId = $_ENV['MAILTRAP_ORGANIZATION_ID']; | |
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens | |
| $subAccounts = (new MailtrapGeneralClient($config))->organization($organizationId)->subAccounts(); | |
| $apiKey = $_ENV['MAILTRAP_API_KEY'] ?? null; | |
| $organizationIdRaw = $_ENV['MAILTRAP_ORGANIZATION_ID'] ?? null; | |
| if (!$apiKey || !$organizationIdRaw || !ctype_digit((string) $organizationIdRaw)) { | |
| throw new RuntimeException('Set MAILTRAP_API_KEY and numeric MAILTRAP_ORGANIZATION_ID in environment.'); | |
| } | |
| $organizationId = (int) $organizationIdRaw; | |
| $config = new Config($apiKey); // your API token from https://mailtrap.io/api-tokens | |
| $subAccounts = (new MailtrapGeneralClient($config))->organization($organizationId)->subAccounts(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/sub-accounts/all.php` around lines 9 - 11, The code reads $_ENV
values directly and may fail when vars are missing; before constructing Config
and calling MailtrapGeneralClient->organization(), validate that
MAILTRAP_API_KEY and MAILTRAP_ORGANIZATION_ID are present, fail fast with a
clear error/exception if they are not, and normalize MAILTRAP_ORGANIZATION_ID to
an integer (e.g., via intval() or (int) cast) when assigning $organizationId so
the subsequent call to
MailtrapGeneralClient($config)->organization($organizationId)->subAccounts()
receives a validated integer ID.
| 'emailTemplates' => Api\General\EmailTemplate::class, | ||
| 'billing' => Api\General\Billing::class, | ||
| 'apiTokens' => Api\General\ApiToken::class, | ||
| 'organization' => Api\General\Organization::class, |
There was a problem hiding this comment.
Naming convention — singular vs plural
The organization key is the only entry in API_MAPPING that uses singular form — every other entry is plural (accounts, users, permissions, contacts, emailTemplates, billing, apiTokens). Users following the established convention will instinctively write ->organizations($id) and hit BadMethodCallException.
Could we rename for consistency?
* @method Api\General\Organization organizations(int $organizationId)
...
'organizations' => Api\General\Organization::class,The tests/MailtrapGeneralClientTest.php:32 match arm needs the same rename.
There was a problem hiding this comment.
My idea was that a token can't have access to multiple organizations, so I kept singular
| /** | ||
| * Class SubAccount | ||
| */ | ||
| class SubAccount extends AbstractApi implements OrganizationInterface |
There was a problem hiding this comment.
Orphaned Api\Organization\ namespace
SubAccount lives in Mailtrap\Api\Organization\ and implements an empty OrganizationInterface marker, but it's only reachable through Mailtrap\Api\General\Organization::subAccounts() (which itself implements GeneralInterface). We end up with two interface markers for one semantic group and a namespace with no top-level entry point.
Would it make sense to:
- Move this class to
src/Api/General/SubAccount.php - Replace
implements OrganizationInterfacewithimplements GeneralInterface - Delete the
src/Api/Organization/directory entirely (including the empty interface) - Move
tests/Api/Organization/SubAccountTest.phptotests/Api/General/SubAccountTest.php
This collapses everything organization-related under the same Api\General namespace where its entry point lives.
| { | ||
| $payload = $webhook->toArray(); | ||
| if (count($payload) === 0) { | ||
| throw new RuntimeException('At least one updatable field must be provided to update a webhook'); |
There was a problem hiding this comment.
Empty-payload guard belongs in the DTO
For CreateWebhook you moved validation into the DTO constructor — could we do the same here for symmetry? Right now UpdateWebhook::toArray() silently returns [] and the API class compensates, which means any future caller constructing the payload via the DTO loses the guarantee.
Suggested:
// UpdateWebhook::toArray()
if ($payload === []) {
throw new InvalidArgumentException('At least one updatable field must be provided to update a webhook');
}
return $payload;Note: I'd stick with $payload === [] rather than empty($payload) here — the new Permissions::toPayload() (src/DTO/Request/Permission/Permissions.php:48) already uses this style, so it keeps the PR internally consistent. empty() also has the historical loose-comparison footguns (empty('0'), empty('false')) that make readers pause even when the input is guaranteed to be an array.
Also suggest swapping RuntimeException → InvalidArgumentException — empty input is an argument-validation issue, consistent with EmailLogs::getMessage() (src/Api/Sending/EmailLogs.php:69).
After the move: drop lines 73-75 here and update testUpdateWebhookFailsWithEmptyPayload to call (new UpdateWebhook())->toArray() directly.
| * @param string|null $url | ||
| * @param bool|null $active | ||
| * @param string|null $payloadFormat One of Webhook::PAYLOAD_FORMAT_* | ||
| * @param string[]|null $eventTypes Subset of Webhook::EVENT_* |
There was a problem hiding this comment.
Document event_types PATCH semantics
Could you confirm whether the Mailtrap PATCH endpoint replaces the event_types list or merges with the existing one? Standard Rails-like PATCH behavior for arrays is full replacement, in which case callers wanting to add click would pass ['click'] and silently narrow the subscription to a single event — a footgun worth surfacing in the docblock.
If replace:
@param string[]|null $eventTypes Replaces the current event_types list entirely
(server-side replacement, not merge). Pass the full desired set.
A unit test asserting the full array round-trips into the request body would also help guard this.
| */ | ||
| class ApiToken extends AbstractApi implements GeneralInterface | ||
| { | ||
| public function __construct(ConfigInterface $config, private int $accountId) |
There was a problem hiding this comment.
Missing getAccountId() getter
ApiToken accepts int $accountId in the constructor but doesn't expose a public getter, breaking the convention used by Permission (src/Api/General/Permission.php:53-56), Stats (src/Api/Sending/Stats.php:151-154), Contact (src/Api/General/Contact.php:356-359). Note that the new Organization class in this PR already exposes getOrganizationId() — so it's inconsistent even within the new files of this PR.
Suggested:
public function getAccountId(): int { return $this->accountId; }Same thing applies to Webhook (no getAccountId()) and SubAccount (no getOrganizationId()).
| */ | ||
| class Webhook extends AbstractApi implements SendingInterface | ||
| { | ||
| public function __construct(ConfigInterface $config, private int $accountId) |
There was a problem hiding this comment.
Missing getAccountId() getter — same point as in ApiToken.php. Please add:
public function getAccountId(): int { return $this->accountId; }| */ | ||
| class SubAccount extends AbstractApi implements OrganizationInterface | ||
| { | ||
| public function __construct(ConfigInterface $config, private int $organizationId) |
There was a problem hiding this comment.
Missing getOrganizationId() getter — same convention point. Please add:
public function getOrganizationId(): int { return $this->organizationId; }| ->method('httpGet') | ||
| ->with(AbstractApi::DEFAULT_HOST . '/api/accounts/' . self::FAKE_ACCOUNT_ID . '/webhooks') | ||
| ->willReturn( | ||
| new Response(200, ['Content-Type' => 'application/json'], json_encode(['data' => [$this->getExpectedWebhookResponse()]])) |
There was a problem hiding this comment.
Inconsistent response envelope vs ApiTokenTest
Webhook fixtures wrap responses in {"data": {...}} (here and lines 69, 118, 207, 233), while tests/Api/General/ApiTokenTest.php fixtures (lines 48, 88, 140, 185) return the object directly with no wrapper. Since both test suites mock httpGet/httpPost, they're green either way — but this is implicitly asserting two different response shapes for two newly added endpoints of the same API.
If the assumed shape is wrong, production parsers in user code will hit KeyError despite green tests — classic mock fail mode. Could we verify against the live Mailtrap API (running examples/webhooks/all.php and examples/api-tokens/all.php against a real account) and align the fixtures?
| * | ||
| * @return ResponseInterface | ||
| */ | ||
| public function forward(int $messageId, string $email): ResponseInterface |
There was a problem hiding this comment.
Local email validation in forward()
forward() currently passes any string to the API and lets the server reject it with 422. A lightweight filter_var(..., FILTER_VALIDATE_EMAIL) guard saves a round-trip and gives users a more actionable error:
if (filter_var($email, FILTER_VALIDATE_EMAIL) === false) {
throw new InvalidArgumentException(sprintf('Invalid recipient email: "%s"', $email));
}Minor, but for an action endpoint with side effects it's worth catching locally.
4 participants
Motivation
Changes
Webhooks
API Tokens
Sub-Accounts
Sandbox Messages
Summary by CodeRabbit
Release Notes