Open
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (38)
📝 WalkthroughWalkthroughAdds Email Logs API support (list and get message) to the SDK, registers it on the sending client, introduces DTOs and filters for listing, updates many example scripts' autoload paths, and adds tests for the new API and query-parameter normalization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant SendingClient as MailtrapSendingClient
participant EmailLogs as EmailLogs API
participant HTTP as HTTP Handler
participant API as Mailtrap API Host
Client->>SendingClient: emailLogs(accountId)
SendingClient->>EmailLogs: instantiate(config, accountId)
rect rgba(100,150,200,0.5)
Note over Client,API: List email logs (filters, pagination)
Client->>EmailLogs: getList(filters, searchAfter)
EmailLogs->>EmailLogs: serialize filters -> query params
EmailLogs->>HTTP: httpGet("/api/accounts/{id}/email_logs", params)
HTTP->>API: GET /api/accounts/{id}/email_logs
API-->>HTTP: HTTP response
HTTP-->>EmailLogs: response body
EmailLogs->>EmailLogs: handleResponse()
EmailLogs-->>Client: ResponseInterface
end
rect rgba(150,100,200,0.5)
Note over Client,API: Get single message by ID
Client->>EmailLogs: getMessage(messageId)
EmailLogs->>HTTP: httpGet("/api/accounts/{id}/email_logs/{messageId}")
HTTP->>API: GET /api/accounts/{id}/email_logs/{messageId}
API-->>HTTP: HTTP response
HTTP-->>EmailLogs: response body
EmailLogs->>EmailLogs: handleResponse()
EmailLogs-->>Client: ResponseInterface
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Api/Sending/EmailLogs.php (1)
65-72: Consider adding empty string validation forsendingMessageId.If an empty string is passed, the resulting URL would be malformed (ending with
/email_logs/). While the API would return an error, early validation provides a clearer developer experience.🛡️ Optional defensive validation
public function getMessage(string $sendingMessageId): ResponseInterface { + if ($sendingMessageId === '') { + throw new \InvalidArgumentException('sendingMessageId cannot be empty'); + } + return $this->handleResponse( $this->httpGet( sprintf('%s/%s', $this->getBasePath(), $sendingMessageId) ) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Api/Sending/EmailLogs.php` around lines 65 - 72, The getMessage method should validate the sendingMessageId parameter to prevent an empty string building a malformed URL; add a check at the start of getMessage (e.g., if trim($sendingMessageId) === '') and throw an InvalidArgumentException (or other existing app-specific exception) with a clear message before calling httpGet/handleResponse so callers get immediate feedback instead of a downstream HTTP error.tests/Api/Sending/EmailLogsTest.php (1)
106-131: Consider moving reflection-based test to AbstractApi test class.This test verifies
normalizeArrayParamsbehavior, which is a method inAbstractApi. While it works here, testing inherited protected/private methods through reflection in a child class test can make it harder to maintain. Consider whether this test belongs in anAbstractApiTestclass instead, or document why it's specifically needed here for EmailLogs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Api/Sending/EmailLogsTest.php` around lines 106 - 131, The test testGetListQueryParamsUseBracketNotationForMultipleValues is exercising AbstractApi::normalizeArrayParams via reflection from the EmailLogs test class; move this reflection-based test into a dedicated AbstractApiTest (or a common parent test) to keep unit tests centered on the class that owns the logic, or alternatively add a clarifying comment in the EmailLogs test explaining why normalizeArrayParams must be validated here; update/remove the reflection usage from EmailLogsTest and create a new test method in AbstractApiTest that calls normalizeArrayParams via reflection to assert bracket notation behavior, referencing the same behavior and assertions currently present in testGetListQueryParamsUseBracketNotationForMultipleValues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Api/Sending/EmailLogs.php`:
- Around line 65-72: The getMessage method should validate the sendingMessageId
parameter to prevent an empty string building a malformed URL; add a check at
the start of getMessage (e.g., if trim($sendingMessageId) === '') and throw an
InvalidArgumentException (or other existing app-specific exception) with a clear
message before calling httpGet/handleResponse so callers get immediate feedback
instead of a downstream HTTP error.
In `@tests/Api/Sending/EmailLogsTest.php`:
- Around line 106-131: The test
testGetListQueryParamsUseBracketNotationForMultipleValues is exercising
AbstractApi::normalizeArrayParams via reflection from the EmailLogs test class;
move this reflection-based test into a dedicated AbstractApiTest (or a common
parent test) to keep unit tests centered on the class that owns the logic, or
alternatively add a clarifying comment in the EmailLogs test explaining why
normalizeArrayParams must be validated here; update/remove the reflection usage
from EmailLogsTest and create a new test method in AbstractApiTest that calls
normalizeArrayParams via reflection to assert bracket notation behavior,
referencing the same behavior and assertions currently present in
testGetListQueryParamsUseBracketNotationForMultipleValues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a48a200-c8d6-4ff8-a733-cd3af6175da1
📒 Files selected for processing (33)
README.mdexamples/batch/bulk.phpexamples/batch/bulk_template.phpexamples/batch/sandbox.phpexamples/batch/sandbox_template.phpexamples/batch/transactional.phpexamples/batch/transactional_template.phpexamples/bulk/bulk.phpexamples/bulk/bulk_template.phpexamples/config/all.phpexamples/contact-fields/all.phpexamples/contact-lists/all.phpexamples/contacts/all.phpexamples/general/accounts.phpexamples/general/billing.phpexamples/general/permissions.phpexamples/general/users.phpexamples/sending/all.phpexamples/sending/email-logs.phpexamples/sending/minimal.phpexamples/sending/suppressions.phpexamples/sending/template.phpexamples/templates/all.phpexamples/testing/attachments.phpexamples/testing/inboxes.phpexamples/testing/messages.phpexamples/testing/projects.phpexamples/testing/send-mail.phpexamples/testing/template.phpsrc/Api/Sending/EmailLogs.phpsrc/MailtrapSendingClient.phptests/Api/Sending/EmailLogsTest.phptests/MailtrapSendingClientTest.php
mklocek
force-pushed
the
email-logs-api
branch
3 times, most recently
from
df34d92 to
514bd21
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/Api/Sending/EmailLogsTest.php (1)
117-120: Avoid tautological assertions on local fixtures.At Line 117-120 and Line 145-147, assertions validate
$expectedParams(locally constructed data) rather than runtime output. Since->with($basePath, $expectedParams)already checks request params, these can be removed or replaced with assertions on parsed response content.Also applies to: 145-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Api/Sending/EmailLogsTest.php` around lines 117 - 120, The test contains tautological assertions against the locally constructed $expectedParams (in EmailLogsTest, the variables around the ->with($basePath, $expectedParams) expectation), e.g. checking $expectedParams['filters']['sent_after'] and subject operator/value; remove these redundant assertions and instead assert runtime behavior: either remove lines asserting $expectedParams fields or replace them with assertions on the parsed response or on the mock client request that is actually executed (the object passed into ->with(...)). Update the assertions at both locations (around the existing ->with($basePath, $expectedParams) usage and the similar block at lines ~145-147) to verify the response or the real request payload rather than the local fixture.src/DTO/Request/EmailLogs/EmailLogsListFilters.php (1)
66-71: Fail fast on unknown criterion keys inwithCriterion().Currently, invalid keys are silently ignored in
toArray(), which can mask typos and send broader-than-expected queries. Validate inwithCriterion()and throw early.💡 Proposed fix
use Mailtrap\DTO\Request\RequestInterface; +use Mailtrap\Exception\InvalidArgumentException; @@ public function withCriterion(string $name, FilterCriterion $criterion): self { + if (!\in_array($name, self::VALID_CRITERIA_KEYS, true)) { + throw new InvalidArgumentException(sprintf('Unknown email logs criterion: %s', $name)); + } + $criteria = $this->criteria; $criteria[$name] = $criterion; return new self($this->sentAfter, $this->sentBefore, $criteria); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DTO/Request/EmailLogs/EmailLogsListFilters.php` around lines 66 - 71, The withCriterion method in EmailLogsListFilters should fail fast for unknown criterion keys: before adding $name to $criteria, validate $name against the set of allowed criterion keys used by toArray (e.g. a class-level allowed keys array or the keys expected in toArray), and if $name is not recognised throw an InvalidArgumentException with a clear message; update withCriterion to perform this check (referencing EmailLogsListFilters::withCriterion, FilterCriterion, and toArray) so typos are rejected early.
🤖 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/EmailLogs.php`:
- Around line 45-47: Trim the $searchAfter input before validating and adding it
to query params: update the logic around $searchAfter (the variable used to set
$params['search_after']) to call trim() on the value and only set
$params['search_after'] when the trimmed value is not an empty string (i.e., not
null and not whitespace). This ensures whitespace-only cursors are ignored and
only meaningful cursors are sent downstream.
- Around line 67-76: The getMessage method currently only checks for empty
sendingMessageId; update it to validate that sendingMessageId matches the
expected UUID pattern and throw InvalidArgumentException if not, then URL-encode
the validated sendingMessageId before composing the path passed to httpGet (use
the existing getBasePath(), httpGet(), and handleResponse() calls) to prevent
reserved/path characters from producing malformed endpoints.
---
Nitpick comments:
In `@src/DTO/Request/EmailLogs/EmailLogsListFilters.php`:
- Around line 66-71: The withCriterion method in EmailLogsListFilters should
fail fast for unknown criterion keys: before adding $name to $criteria, validate
$name against the set of allowed criterion keys used by toArray (e.g. a
class-level allowed keys array or the keys expected in toArray), and if $name is
not recognised throw an InvalidArgumentException with a clear message; update
withCriterion to perform this check (referencing
EmailLogsListFilters::withCriterion, FilterCriterion, and toArray) so typos are
rejected early.
In `@tests/Api/Sending/EmailLogsTest.php`:
- Around line 117-120: The test contains tautological assertions against the
locally constructed $expectedParams (in EmailLogsTest, the variables around the
->with($basePath, $expectedParams) expectation), e.g. checking
$expectedParams['filters']['sent_after'] and subject operator/value; remove
these redundant assertions and instead assert runtime behavior: either remove
lines asserting $expectedParams fields or replace them with assertions on the
parsed response or on the mock client request that is actually executed (the
object passed into ->with(...)). Update the assertions at both locations (around
the existing ->with($basePath, $expectedParams) usage and the similar block at
lines ~145-147) to verify the response or the real request payload rather than
the local fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eda3a315-719d-4bed-956a-ae74afbfc384
📒 Files selected for processing (38)
README.mdexamples/batch/bulk.phpexamples/batch/bulk_template.phpexamples/batch/sandbox.phpexamples/batch/sandbox_template.phpexamples/batch/transactional.phpexamples/batch/transactional_template.phpexamples/bulk/bulk.phpexamples/bulk/bulk_template.phpexamples/config/all.phpexamples/contact-fields/all.phpexamples/contact-lists/all.phpexamples/contacts/all.phpexamples/general/accounts.phpexamples/general/billing.phpexamples/general/permissions.phpexamples/general/users.phpexamples/sending/all.phpexamples/sending/email-logs.phpexamples/sending/minimal.phpexamples/sending/suppressions.phpexamples/sending/template.phpexamples/templates/all.phpexamples/testing/attachments.phpexamples/testing/inboxes.phpexamples/testing/messages.phpexamples/testing/projects.phpexamples/testing/send-mail.phpexamples/testing/template.phpsrc/Api/Sending/EmailLogs.phpsrc/DTO/Request/EmailLogs/EmailLogsFilterOperator.phpsrc/DTO/Request/EmailLogs/EmailLogsFilterValue.phpsrc/DTO/Request/EmailLogs/EmailLogsListFilters.phpsrc/DTO/Request/EmailLogs/FilterCriterion.phpsrc/MailtrapSendingClient.phptests/Api/AbstractApiTest.phptests/Api/Sending/EmailLogsTest.phptests/MailtrapSendingClientTest.php
🚧 Files skipped from review as they are similar to previous changes (21)
- examples/testing/inboxes.php
- examples/testing/send-mail.php
- examples/general/accounts.php
- examples/batch/bulk.php
- examples/batch/transactional_template.php
- examples/sending/email-logs.php
- examples/general/billing.php
- examples/contact-fields/all.php
- examples/contact-lists/all.php
- examples/templates/all.php
- examples/testing/attachments.php
- examples/testing/projects.php
- examples/contacts/all.php
- examples/bulk/bulk_template.php
- examples/general/permissions.php
- examples/sending/suppressions.php
- examples/batch/bulk_template.php
- examples/testing/messages.php
- examples/config/all.php
- examples/sending/minimal.php
- examples/batch/sandbox.php
piobeny
approved these changes
Mar 17, 2026
The README says to run examples from the project root, e.g. php examples/sending/minimal.php. Scripts under examples/<subdir>/ (e.g. examples/batch/, examples/sending/) used: require __DIR__ . '/../vendor/autoload.php'; That resolves to examples/vendor/autoload.php, while vendor/ actually lives at the project root, so the path was incorrect. From examples/<subdir>/, you need to go up two levels to reach the project root, then into vendor/: require __DIR__ . '/../../vendor/autoload.php';
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
2 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.
Changes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores