Complete Mail correctness, current parity, and package boundaries - #470
Merged
Conversation
Intercept every side-effecting MailFake delivery entry point so tests cannot accidentally reach a real transport or queue. Consume explicit mailer selection exactly once across success and failure while preserving recipients, queue names, enum identifiers, and real-mailer validation behavior. Correct MailFake assertion callback types and NotificationFake callable handling, port the current upstream fake coverage, and record the remaining EventFake parity work separately.
Apply explicit queue names before dispatch, pass the owning queue factory through delayed delivery, and support enum queue identifiers across optional and named-queue entry points without weakening their nullability contracts. Render message content before callbacks so direct sends and mailable envelope callbacks observe and may replace the final body. Complete the no-attachment assertion, preserve legacy storage MIME behavior, and revalidate the narrowed callback contract through Console scheduled output.
Restrict remote attachments to HTTP and HTTPS while restoring current single-label URL support at the shared Str boundary. Resolve storage disks once, preserve explicit MIME types, and let unknown MIME fall back without violating Symfony string contracts. Keep the intentionally narrow Filesystem contract instead of introducing adapter capability machinery, document native resource shapes, and cover URL, storage, pooled-adapter, and attachment target behavior with focused and integration regressions.
Read a non-empty X-SES-TENANT-NAME header from the Symfony message and forward it as the SES v2 TenantName option without mutating shared transport configuration. Cover present, absent, and empty tenant headers while retaining Hypervel intentional SES v2-only transport support.
Build on-demand transports only from their supplied configuration, replace stale container access and suppressions, and retain the Laravel protected provider extension point with accurate documentation. Declare the Mail split package actual direct and optional dependencies, point facade metadata at the concrete forwarded mailer surface, regenerate the facade, and pin package and facade invariants with focused tests.
Move the affected Mail tests onto framework-owned test bases, add truthful native method types, and retain current upstream mailable, Markdown, data, header, assertion, and attachable-message coverage. Give image and Markdown fixtures worker-safe temporary-directory ownership with exception-safe cleanup, eliminating committed-tree writes and redundant per-test unlink calls.
Add the repository-required native void return types to the remaining Mail transport tests without changing their transport ordering, failure, resend, or logging assertions. This completes the touched Mail test surface consistently while preserving the existing behavioral coverage.
Apply truthful native types throughout the Mail integration suite while retaining the shared environment-owning base case and every rendering, locale, queue, Markdown, encoding, and sent-message scenario. The storage attachment integration remains with the attachment-boundary commit because it directly proves the corrected adapter and resolver behavior.
Document HTTP-only remote attachments, no-attachment assertions, enum queue identifiers, SES v2 tenant headers, and the stable configuration boundary exposed by pooled mailers in the canonical Mail guide. Keep package READMEs minimal: record intentional SES v2-only support and existing cloud-storage guidance for Mail, and restore the HTTP badge, canonical Requests guide link, approved differences, and final provenance ordering.
Add the signed-off Mail design with its verified findings, rejected concerns, implementation boundaries, regression strategy, performance analysis, and Laravel-facing result. Record every Mail and shared finding in the audit ledger, amend completed Contracts, Support, Filesystem, Console, and HTTP work units, add only the six genuine cross-package dependencies, and mark Mail complete in the package checklist.
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request audits and updates Mail behavior across contracts, delivery, attachments, queues, transports, fakes, package metadata, documentation, and tests. It also adds cross-package validation and coroutine-safe test fixtures. ChangesMail contracts and delivery
Validation and package integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Greptile SummaryThis PR completes Mail package parity and correctness work across delivery, queue routing, attachment handling, test fakes, transport configuration, package metadata, and documentation.
Confidence Score: 5/5The PR appears safe to merge because no eligible new or outstanding blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/mail/src/Mailer.php | Updates callback ordering and enum-capable queue routing while passing the configured queue factory through immediate and delayed mailable dispatch. |
| src/support/src/Testing/Fakes/MailFake.php | Expands fake-owned mail entry points, one-shot mailer selection, queue metadata preservation, and recipient recording. |
| src/mail/src/Attachment.php | Restricts URL attachments to HTTP(S), resolves storage disks once, and treats unknown MIME metadata as an inference fallback. |
| src/mail/src/Mailable.php | Improves storage attachment MIME handling and adds an assertion that verifies all attachment collections are empty. |
| src/mail/src/MailManager.php | Uses typed container resolution, isolates on-demand transport construction from named-mailer presentation settings, and retains pooled transport behavior. |
| src/mail/src/Transport/SesV2Transport.php | Maps a non-empty SES tenant header into the SES v2 TenantName request option. |
| src/support/src/Str.php | Restores URL recognition for single-label hosts while retaining caller-supplied protocol restrictions. |
| src/mail/composer.json | Declares directly used Symfony packages, removes unrelated hard dependencies, and documents optional integration requirements. |
| src/contracts/src/Mail/MailQueue.php | Widens optional queue identifiers to support strings and unit enums consistently with the queue subsystem. |
| src/support/src/Testing/Fakes/NotificationFake.php | Corrects callable handling in notification fake assertions with corresponding test coverage. |
Reviews (2): Last reviewed commit: "fix(mail): preserve exact SES tenant nam..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/support/src/Str.php (1)
515-529: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDocument the host regex adaptation.
Str::isUrl()diverges from the Symfony/Laravel host expression: the single-level branch remains ASCII-only, and the global\.?accepts a trailing dot. If this is intentional, update the header comment to record the deliberate adaptation, not “derived from Symfony 5.0.7”.🤖 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 `@src/support/src/Str.php` around lines 515 - 529, The header comment for Str::isUrl() should document the deliberate host-regex adaptations: the single-level hostname branch is ASCII-only and the optional global dot permits a trailing dot. Replace the misleading “derived from Symfony 5.0.7” wording while leaving the regex behavior unchanged.Source: Coding guidelines
🤖 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 `@src/mail/src/MailManager.php`:
- Around line 145-147: Remove or rewrite the comment preceding the mailer
construction in MailManager so it accurately describes the behavior implemented
by the surrounding code; do not claim that a container instance is assigned to
Mailer when this method only creates the mailer and optionally configures its
queue.
In `@src/mail/src/Transport/SesV2Transport.php`:
- Around line 40-42: Update the tenant handling around tenantName() to preserve
the non-empty value "0": use an explicit null check in the assignment condition
so it is added to SES V2 options, and change the tenantName() return logic to
treat only an empty string as absent rather than using truthiness. Add coverage
for an X-SES-TENANT-NAME value of "0".
In `@src/support/src/Testing/Fakes/NotificationFake.php`:
- Line 56: Update the Notification facade’s assertSentTo callback parameter type
to match NotificationFake::assertSentTo, using callable|int|null and removing
string from the union so numeric strings are handled by the fake as counts.
In `@tests/Integration/Mail/SendingMarkdownMailTest.php`:
- Line 159: Remove the unused $mailable assignment in the
MarkdownEmbedImageMailable test and pass the new mailable instance directly to
Mail::to(...)->send().
In `@tests/Support/SupportTestingNotificationFakeTest.php`:
- Around line 252-262: Move UserStub and LocalizedUserStub into the
Hypervel\Tests\Support\SupportTestingNotificationFakeTest namespace, updating
all references and hardcoded class-name strings. Also move MailableStub,
QueueableMailableStub, LocalizedRecipientStub, and FailingQueueMailableStub into
Hypervel\Tests\Support\SupportTestingMailFakeTest, updating their references and
assertion strings; leave MailFakeMailerName and MailFakeQueueName unchanged.
Affected sites: tests/Support/SupportTestingNotificationFakeTest.php:252-262 and
tests/Support/SupportTestingMailFakeTest.php:646-707.
---
Nitpick comments:
In `@src/support/src/Str.php`:
- Around line 515-529: The header comment for Str::isUrl() should document the
deliberate host-regex adaptations: the single-level hostname branch is
ASCII-only and the optional global dot permits a trailing dot. Replace the
misleading “derived from Symfony 5.0.7” wording while leaving the regex behavior
unchanged.
🪄 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 Plus
Run ID: 26618074-04b0-4e24-b015-8a595abfeadc
📒 Files selected for processing (56)
docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-08-03-1909-mail-correctness-current-parity-and-package-boundaries.mddocs/todo.mdsrc/boost/docs/mail.mdsrc/contracts/src/Mail/MailQueue.phpsrc/contracts/src/Mail/Mailer.phpsrc/filesystem/src/ServeFile.phpsrc/http/README.mdsrc/mail/README.mdsrc/mail/composer.jsonsrc/mail/src/Attachment.phpsrc/mail/src/MailManager.phpsrc/mail/src/MailServiceProvider.phpsrc/mail/src/Mailable.phpsrc/mail/src/Mailer.phpsrc/mail/src/Message.phpsrc/mail/src/TextMessage.phpsrc/mail/src/Transport/SesV2Transport.phpsrc/support/src/Facades/Mail.phpsrc/support/src/Str.phpsrc/support/src/Testing/Fakes/MailFake.phpsrc/support/src/Testing/Fakes/NotificationFake.phptests/Console/Scheduling/EventTest.phptests/Integration/Mail/AttachingFromStorageTest.phptests/Integration/Mail/MailableTestCase.phptests/Integration/Mail/MailableWithSecuredEncodingTest.phptests/Integration/Mail/MailableWithoutSecuredEncodingTest.phptests/Integration/Mail/MarkdownParserTest.phptests/Integration/Mail/RenderingMailWithLocaleTest.phptests/Integration/Mail/SendingMailWithLocaleTest.phptests/Integration/Mail/SendingMarkdownMailTest.phptests/Integration/Mail/SendingQueuedMailTest.phptests/Integration/Mail/SentMessageMailTest.phptests/Mail/AttachableTest.phptests/Mail/AttachmentTest.phptests/Mail/MailFailoverTransportTest.phptests/Mail/MailLogTransportTest.phptests/Mail/MailMailableAssertionsTest.phptests/Mail/MailMailableDataTest.phptests/Mail/MailMailableHeadersTest.phptests/Mail/MailMailableTest.phptests/Mail/MailMailerTest.phptests/Mail/MailManagerTest.phptests/Mail/MailMarkdownTest.phptests/Mail/MailMessageTest.phptests/Mail/MailResendTransportTest.phptests/Mail/MailRoundRobinTransportTest.phptests/Mail/MailSesV2TransportTest.phptests/Mail/MailableAlternativeSyntaxTest.phptests/Mail/MailableQueuedTest.phptests/Mail/MarkdownCoroutineSafetyTest.phptests/Mail/PackageMetadataTest.phptests/Support/SupportStrTest.phptests/Support/SupportTestingMailFakeTest.phptests/Support/SupportTestingNotificationFakeTest.php
Treat only an absent or empty X-SES-TENANT-NAME header as unset so valid numeric tenant names such as "0" reach the SES v2 request unchanged. Add a counterfactual regression for the numeric case. Regenerate the Notification facade from the corrected fake signature, remove a stale MailManager comment and an unused integration-test assignment, and keep the audit records aligned with the evidence.
Collaborator
Author
|
@coderabbitai Thanks for the review. The follow-up is in Changed:
No change:
Focused Mail, integration Mail, Notification fake, and facade-lint checks pass. The full |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-ext-diff --format=fuller --stat a2d2f35f9
git show --no-ext-diff --format= -- \
a2d2f35f9 -- \
src/mail/src/Transport/SesV2Transport.php \
tests/Mail/MailSesV2TransportTest.php \
src/mail/src/MailManager.php \
src/support/src/Facades/Notification.php \
tests/Integration/Mail/SendingMarkdownMailTest.php
printf '\n--- Follow-up commit patch ---\n'
git show --no-ext-diff --format= a2d2f35f9Length of output: 37003
The SES v2 transport now preserves The Notification facade metadata now matches I accept the retained Support test namespaces, the Laravel-identical 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
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
1 participant
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.
Summary
This completes the Mail package correctness and current-parity work. It fixes queue routing, message callback ordering, attachment boundaries, SES v2 tenant forwarding, fake behavior, facade metadata, and split-package dependencies. It also ports the relevant current Laravel tests and keeps Hypervel's long-lived worker and pooled-transport behavior explicit.
For more details, see:
docs/plans/2026-08-03-1909-mail-correctness-current-parity-and-package-boundaries.mdWhat changed
Delivery and queues
queueOnandlaterOnaliases.assertHasNoAttachmentsassertion.Attachments
Str::isUrl, including internal service URLs such ashttp://mail/....falseinto a string boundary.Fakes and facade metadata
MailFakeown every side-effecting mail entry point so a test cannot accidentally reach a real transport or queue.Transports and package boundaries
X-SES-TENANT-NAMEheaders as the SES v2TenantNameoption.TransportPoolProxyand document when disabling pooling is appropriate for concrete transport inspection.Tests and documentation
Compatibility and performance
Supported Laravel Mail APIs, named arguments, protected extension points, callback shapes, queue identifiers, and facade forwarding are preserved or restored. Callback metadata now uses the actual supported
Closure|stringboundary instead ofmixed; values outside that union were never valid Mail behavior.No request-scoped registry, lock, retry, cache, context state, or compatibility wrapper is introduced. Message callback ordering moves existing work without adding work. Storage attachments perform fewer container/disk resolutions and skip unnecessary metadata I/O. Other changes are constant-time checks at existing operation boundaries or test/documentation-only changes.
Validation
composer fixgate, including formatting, both PHPStan configurations, the parallel component suite, Testbench package mode, and dogfood.Summary by CodeRabbit
New Features
X-SES-TENANT-NAMEheader.Bug Fixes
Documentation