Conversation
leonid-shevtsov
approved these changes
Mar 13, 2026
mklocek
approved these changes
Mar 16, 2026
📝 WalkthroughWalkthroughAdds a new Sending Stats API with endpoints for aggregated metrics and groupings (by domain, category, email service provider, and date); integrates it into the client, provides an example, and adds tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as MailtrapSendingClient
participant Stats as Stats API
participant HTTP as HTTP Layer
participant Server as API Server
User->>Client: stats(accountId) -> get Stats instance
Client->>Stats: __construct(config, accountId)
Stats-->>Client: Stats instance
User->>Stats: get(startDate, endDate, filters)
Stats->>Stats: buildQueryParams(filters)
Stats->>HTTP: httpGet(url, query)
HTTP->>Server: GET /sending/{accountId}/stats?params
Server-->>HTTP: JSON response
HTTP-->>Stats: PSR-7 Response
Stats->>Stats: handleResponse(response)
Stats-->>User: ResponseInterface
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Api/Sending/StatsTest.php (1)
55-55: Assert the PSR-7 contract instead of Nyholm’s concrete class.
StatsreturnsResponseInterface; these checks will start failing on any valid PSR-7 implementation swap even though the behavior is unchanged. Checking the interface keeps the tests aligned with the public contract.🔧 Contract-level assertion
+use Psr\Http\Message\ResponseInterface; ... - $this->assertInstanceOf(Response::class, $response); + $this->assertInstanceOf(ResponseInterface::class, $response);Repeat the same change for the other response assertions in this file.
Also applies to: 92-92, 130-130, 164-164, 195-195, 226-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Api/Sending/StatsTest.php` at line 55, Replace concrete Nyholm response assertions with the PSR-7 contract: change assertions like $this->assertInstanceOf(Response::class, $response) to assert the interface Psr\Http\Message\ResponseInterface instead, and apply the same replacement for the other occurrences in this test (the assertions at the other mentioned locations). Ensure the tests import or reference Psr\Http\Message\ResponseInterface so the assertions check the public contract rather than Nyholm's concrete Response class.src/Api/Sending/Stats.php (1)
24-29: Add concrete element types to these filter docblocks.All five public methods expose multiple plain
arrayparams, so IDEs and static analysis cannot tell thatsendingDomainIdsis anint[]while the other filters arestring[]. Tightening the PHPDoc will make this new SDK surface easier to call correctly.✍️ Example of the docblock tightening
- * `@param` array $sendingDomainIds - * `@param` array $sendingStreams - * `@param` array $categories - * `@param` array $emailServiceProviders + * `@param` int[] $sendingDomainIds + * `@param` string[] $sendingStreams + * `@param` string[] $categories + * `@param` string[] $emailServiceProvidersAlso applies to: 50-55, 76-81, 102-107, 128-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Api/Sending/Stats.php` around lines 24 - 29, Update the PHPDoc for the public methods in src/Api/Sending/Stats.php to use concrete array element types so static analysis and IDEs know the expected types; change `@param` array $sendingDomainIds to `@param` int[] $sendingDomainIds and change `@param` array $sendingStreams, `@param` array $categories, `@param` array $emailServiceProviders (and any other plain array filters in the same methods such as at lines referenced: 50-55, 76-81, 102-107, 128-133) to `@param` string[] $sendingStreams, `@param` string[] $categories, `@param` string[] $emailServiceProviders respectively, keeping existing parameter names and descriptions intact in each method's docblock.
🤖 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/sending/stats.php`:
- Around line 7-10: The example's bootstrap uses the wrong vendor path and
unreliable $_ENV; update the require of the autoloader (the existing require
__DIR__ ... line) to point to the repository-root vendor/autoload.php, replace
$_ENV usage with getenv() for MAILTRAP_ACCOUNT_ID and MAILTRAP_API_KEY, add a
guard that exits or throws a clear error when either value is missing, and
ensure $accountId is cast to int before constructing Config (reference variables
$accountId and the Config constructor to locate the fix).
---
Nitpick comments:
In `@src/Api/Sending/Stats.php`:
- Around line 24-29: Update the PHPDoc for the public methods in
src/Api/Sending/Stats.php to use concrete array element types so static analysis
and IDEs know the expected types; change `@param` array $sendingDomainIds to
`@param` int[] $sendingDomainIds and change `@param` array $sendingStreams, `@param`
array $categories, `@param` array $emailServiceProviders (and any other plain
array filters in the same methods such as at lines referenced: 50-55, 76-81,
102-107, 128-133) to `@param` string[] $sendingStreams, `@param` string[]
$categories, `@param` string[] $emailServiceProviders respectively, keeping
existing parameter names and descriptions intact in each method's docblock.
In `@tests/Api/Sending/StatsTest.php`:
- Line 55: Replace concrete Nyholm response assertions with the PSR-7 contract:
change assertions like $this->assertInstanceOf(Response::class, $response) to
assert the interface Psr\Http\Message\ResponseInterface instead, and apply the
same replacement for the other occurrences in this test (the assertions at the
other mentioned locations). Ensure the tests import or reference
Psr\Http\Message\ResponseInterface so the assertions check the public contract
rather than Nyholm's concrete Response class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c51cbc7-ce19-4272-958d-5bf6da40b03c
📒 Files selected for processing (6)
CHANGELOG.mdexamples/sending/stats.phpsrc/Api/Sending/Stats.phpsrc/MailtrapSendingClient.phptests/Api/Sending/StatsTest.phptests/MailtrapSendingClientTest.php
examples/sending/stats.php
Outdated
Comment on lines
+7
to
+10
| require __DIR__ . '/../vendor/autoload.php'; | ||
|
|
||
| $accountId = (int) $_ENV['MAILTRAP_ACCOUNT_ID']; | ||
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the overall repository structure
git ls-files | head -30Repository: mailtrap/mailtrap-php
Length of output: 813
🏁 Script executed:
# Specifically look for vendor directory and examples structure
find . -maxdepth 3 -type d -name vendor -o -type d -name examples | head -20Repository: mailtrap/mailtrap-php
Length of output: 75
🏁 Script executed:
# Check where vendor/autoload.php actually exists
find . -name autoload.php -type f 2>/dev/null | head -10Repository: mailtrap/mailtrap-php
Length of output: 47
🏁 Script executed:
# List the examples directory structure
fd --max-depth 3 . examples/ 2>/dev/null | head -40Repository: mailtrap/mailtrap-php
Length of output: 1132
🏁 Script executed:
# Check if there are other example files that bootstrap, to see the pattern used
rg "require.*autoload" examples/ --type php -B2 -A2Repository: mailtrap/mailtrap-php
Length of output: 7832
Fix the example bootstrap before shipping it.
__DIR__ . '/../vendor/autoload.php' resolves to examples/vendor/autoload.php, but the vendor directory is at the repository root. The same block also assumes $_ENV is populated, which is unreliable in CLI; getenv() plus an explicit guard is safer for runnable examples.
🛠️ Minimal bootstrap fix
-require __DIR__ . '/../vendor/autoload.php';
+require dirname(__DIR__, 2) . '/vendor/autoload.php';
-$accountId = (int) $_ENV['MAILTRAP_ACCOUNT_ID'];
-$config = new Config($_ENV['MAILTRAP_API_KEY']); `#your` API token from here https://mailtrap.io/api-tokens
+$accountId = filter_var(getenv('MAILTRAP_ACCOUNT_ID'), FILTER_VALIDATE_INT);
+$apiKey = getenv('MAILTRAP_API_KEY');
+
+if (false === $accountId || false === $apiKey) {
+ throw new \RuntimeException('Set MAILTRAP_ACCOUNT_ID and MAILTRAP_API_KEY before running this example.');
+}
+
+$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.
Suggested change
| require __DIR__ . '/../vendor/autoload.php'; | |
| $accountId = (int) $_ENV['MAILTRAP_ACCOUNT_ID']; | |
| $config = new Config($_ENV['MAILTRAP_API_KEY']); #your API token from here https://mailtrap.io/api-tokens | |
| require dirname(__DIR__, 2) . '/vendor/autoload.php'; | |
| $accountId = filter_var(getenv('MAILTRAP_ACCOUNT_ID'), FILTER_VALIDATE_INT); | |
| $apiKey = getenv('MAILTRAP_API_KEY'); | |
| if (false === $accountId || false === $apiKey) { | |
| throw new \RuntimeException('Set MAILTRAP_ACCOUNT_ID and MAILTRAP_API_KEY before running this example.'); | |
| } | |
| $config = new Config($apiKey); // your API token from https://mailtrap.io/api-tokens |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/sending/stats.php` around lines 7 - 10, The example's bootstrap uses
the wrong vendor path and unreliable $_ENV; update the require of the autoloader
(the existing require __DIR__ ... line) to point to the repository-root
vendor/autoload.php, replace $_ENV usage with getenv() for MAILTRAP_ACCOUNT_ID
and MAILTRAP_API_KEY, add a guard that exits or throws a clear error when either
value is missing, and ensure $accountId is cast to int before constructing
Config (reference variables $accountId and the Config constructor to locate the
fix).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Api/Sending/Stats.php (1)
59-149: Consider extracting common GET endpoint plumbing.
byDomain,byCategory,byEmailServiceProvider, andbyDaterepeat the same call pattern with only a path suffix change. A small private helper would reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Api/Sending/Stats.php` around lines 59 - 149, The four methods byDomain, byCategory, byEmailServiceProvider and byDate duplicate identical GET plumbing; add a small private helper (e.g., private function getStatsBy(string $suffix, string $startDate, string $endDate, array $sendingDomainIds = [], array $sendingStreams = [], array $categories = [], array $emailServiceProviders = []): ResponseInterface) that calls $this->handleResponse($this->httpGet($this->getBasePath() . $suffix, $this->buildQueryParams(...))) and then update each public method to call that helper with '/domains', '/categories', '/email_service_providers', and '/date' respectively, reusing buildQueryParams, getBasePath, httpGet and handleResponse.tests/Api/Sending/StatsTest.php (1)
115-229: Add filter-forwarding tests for grouped endpoints.Only
testGetWithFilters()verifies optional filter arrays are passed tohttpGet. Add filtered variants for grouped methods too, so query mapping regressions are caught early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Api/Sending/StatsTest.php` around lines 115 - 229, Add new test methods that mirror the existing grouped tests but include an additional filter array parameter and assert it is passed to httpGet; e.g. create testByDomainWithFilters, testByCategoryWithFilters, testByEmailServiceProviderWithFilters, and testByDateWithFilters that call Stats::byDomain/byCategory/byEmailServiceProvider/byDate with a third $filters argument, set the mock expectation on httpGet (same AbstractApi::DEFAULT_HOST paths) to receive ['start_date'=>'2026-01-01','end_date'=>'2026-01-31','filters'=> $filters] (or however the client maps filters into the query), return the same JSON Response, convert with ResponseHelper::toArray and assert the response shape/values as in the existing tests; follow the existing pattern of $this->stats->expects($this->once())->method('httpGet')->with(...)->willReturn(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Api/Sending/Stats.php`:
- Around line 16-19: Validate the injected accountId in the Stats::__construct
and throw an InvalidArgumentException if it's not a positive integer (<= 0) so
misuse is caught immediately; add the check at the start of the constructor
(before proceeding with parent::__construct) and use a clear error message
mentioning the invalid $accountId and the Stats class.
---
Nitpick comments:
In `@src/Api/Sending/Stats.php`:
- Around line 59-149: The four methods byDomain, byCategory,
byEmailServiceProvider and byDate duplicate identical GET plumbing; add a small
private helper (e.g., private function getStatsBy(string $suffix, string
$startDate, string $endDate, array $sendingDomainIds = [], array $sendingStreams
= [], array $categories = [], array $emailServiceProviders = []):
ResponseInterface) that calls
$this->handleResponse($this->httpGet($this->getBasePath() . $suffix,
$this->buildQueryParams(...))) and then update each public method to call that
helper with '/domains', '/categories', '/email_service_providers', and '/date'
respectively, reusing buildQueryParams, getBasePath, httpGet and handleResponse.
In `@tests/Api/Sending/StatsTest.php`:
- Around line 115-229: Add new test methods that mirror the existing grouped
tests but include an additional filter array parameter and assert it is passed
to httpGet; e.g. create testByDomainWithFilters, testByCategoryWithFilters,
testByEmailServiceProviderWithFilters, and testByDateWithFilters that call
Stats::byDomain/byCategory/byEmailServiceProvider/byDate with a third $filters
argument, set the mock expectation on httpGet (same AbstractApi::DEFAULT_HOST
paths) to receive
['start_date'=>'2026-01-01','end_date'=>'2026-01-31','filters'=> $filters] (or
however the client maps filters into the query), return the same JSON Response,
convert with ResponseHelper::toArray and assert the response shape/values as in
the existing tests; follow the existing pattern of
$this->stats->expects($this->once())->method('httpGet')->with(...)->willReturn(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2766544d-76af-47b9-bfc8-c9ae086b5632
📒 Files selected for processing (6)
CHANGELOG.mdexamples/sending/stats.phpsrc/Api/Sending/Stats.phpsrc/MailtrapSendingClient.phptests/Api/Sending/StatsTest.phptests/MailtrapSendingClientTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- src/MailtrapSendingClient.php
- CHANGELOG.md
- tests/MailtrapSendingClientTest.php
- examples/sending/stats.php
Comment on lines
+16
to
+19
| public function __construct(ConfigInterface $config, private int $accountId) | ||
| { | ||
| parent::__construct($config); | ||
| } |
There was a problem hiding this comment.
Fail fast for invalid account IDs.
A non-positive account ID currently passes through and only fails at request time. Validate it in the constructor to catch misuse earlier.
Proposed fix
public function __construct(ConfigInterface $config, private int $accountId)
{
+ if ($accountId <= 0) {
+ throw new \InvalidArgumentException('Account ID must be greater than 0.');
+ }
+
parent::__construct($config);
}📝 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.
Suggested change
| public function __construct(ConfigInterface $config, private int $accountId) | |
| { | |
| parent::__construct($config); | |
| } | |
| public function __construct(ConfigInterface $config, private int $accountId) | |
| { | |
| if ($accountId <= 0) { | |
| throw new \InvalidArgumentException('Account ID must be greater than 0.'); | |
| } | |
| parent::__construct($config); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Api/Sending/Stats.php` around lines 16 - 19, Validate the injected
accountId in the Stats::__construct and throw an InvalidArgumentException if
it's not a positive integer (<= 0) so misuse is caught immediately; add the
check at the start of the constructor (before proceeding with
parent::__construct) and use a clear error message mentioning the invalid
$accountId and the Stats class.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
/api/accounts/{account_id}/stats) to the PHP SDK, enabling users to retrieve aggregated email sending statistics.Changes
How to test
Examples
Summary by CodeRabbit
New Features
Documentation
Tests
Chore