smtp: implement the client, the message builder and both transports - #130
Merged
Conversation
Eleven specifications the client has to satisfy, taken verbatim from the RFC Editor, plus notes from Go net/smtp, emersion/go-smtp, Symfony Mailer and PHPMailer -- the last of which packages/messaging wraps today, so its behaviour is the compatibility target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Records the three decisions taken on scope: submission only, both a native and a Swoole transport, and a message builder with MIME so packages/messaging can eventually drop PHPMailer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the placeholder with the design in docs/design.md. The envelope is its own type and the client never parses what it sends, so a blind recipient can reach RCPT TO without reaching a header. A send returns which recipients were accepted and which were not, with the reply that says whether a refusal is worth retrying -- the fact PHPMailer drops and packages/messaging cannot currently express. Encryption is an enum rather than Symfony's two booleans, and the capability map is rebuilt after STARTTLS along with the read buffer, so bytes that arrived in the clear are never trusted afterwards. 96 unit tests drive a scripted transport; 10 more run against Mailpit, which is where the awkward truths live -- it advertises SIZE 0 and annotates stored messages with its own Bcc field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThe PR replaces the SMTP package placeholder with a complete message-submission client, MIME message builder, authentication mechanisms, and native and Swoole transports.
Confidence Score: 5/5The PR appears safe to merge. The previously reported SMTPUTF8, interrupted-DATA, final-reply recovery, and LOGIN reconnect failures are resolved in the current code, and no blocking failure remains. Important Files Changed
Reviews (8): Last reviewed commit: "smtp: stop asking the kernel for a port ..." | Re-trigger Greptile |
A connection interrupted part way through DATA was left marked ready while the server was still reading message data, so the next send wrote MAIL FROM as the body of the last one. That connection is now discarded; a refusal after the terminating dot still keeps it, since the server is back in command state by then. Reply-To is a header and never a path, so a non-ASCII one appeared in no RCPT TO and SMTPUTF8 went undeclared. The envelope now carries the flag Envelope::fromMessage reads off the message. LOGIN counts challenges and an authenticator outlives its connection, so a reconnect answered the first prompt with the password. initial() is specified as the once-per-exchange reset point. Found by Greptile on #130. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audited against the package-design skill, which the first pass had not been written against. Three gaps, each a real one: phpstan was inheriting the root level 5 rather than pinning level max, and rector was not extending the root with the stable prepared sets. At max the analysis found 29 errors -- nullable socket error codes, unbounded read lengths, and Swoole's untyped errCode and errMsg going straight into messages. All fixed by narrowing at the seam, none by an ignore. Value objects are now final readonly. That is what surfaced the real design fault: Login could not be readonly because it counted challenges, which is the mutable per-exchange state the checklist says to grep for -- and the same state Greptile reported as a reconnect bug. The client now passes the step, so the mechanism holds nothing and the bug is no longer expressible. The e2e helpers narrow through PHPUnit assertions instead of casts, which caught setUp reading an empty DELETE body as an object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An interface audit, looking for booleans that should be enums and shallow modules in Ousterhout's sense. Three findings, all the same shape: a type naming states it did not have, or unable to name states it did. Reply carried isPositive, isTransient and isPermanent -- three booleans spanning eight combinations for a set of four, where 354 answered false to all three. RFC 5321 already names the four classes; Outcome does too. Tls carried a verifyPeer boolean that switched off the issuer check and the hostname check together. Verification splits them, and the e2e suite now runs SelfSigned instead of discarding a check it could keep. Mime\Encoding was seven static one-liners with the composition left to callers, and that shallowness was hiding two protocol violations: nothing folded header lines, so thirty recipients produced a 1482 octet To field against a 998 limit, and nothing split encoded words, so an accented subject produced a 200 character word against a limit of 75. Mime\Header owns encode, split and fold; the longest line is now 78. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things. The state rule after DATA was too narrow. A 4yz or 5yz on the reply to the terminating dot is an answer and the session continues, but a socket failure or an unparseable reply is not -- the stream is dead or no longer aligned, and a client left marked ready sends its next MAIL FROM into that. The rule is about the kind of failure rather than where it happened, so every command now goes through one exchange() that keeps the connection only for a transaction failure. The greeting read goes through it too. Then the coverage gaps, found by auditing which symbols no test mentions. Transport\Native had none at all: it is now driven against a real socket on an ephemeral port, including the failures a healthy server never produces -- hang-ups, use before connect, use after close, a refused connection. The streaming attachment path had none either, which is where a block size that is not a multiple of three would corrupt every byte after the first block while a small file looked fine. The reply reader, the one part fed by input it does not control, is now pinned as bounded on over-long lines, too many continuations, a code that changes part way through, and a hang-up mid-reply. 151 unit tests, up from 105. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A second Mailpit reaches two paths the first cannot: TLS from the first byte, and a recipient ceiling low enough to draw a 452. That transient per-recipient refusal is the case the Result shape exists for, and until now only the permanent one had ever been seen from a real server. The suite splits by question. ClientTest covers the session, including several messages on one connection and a second session on a closed client -- the shape of the LOGIN state bug, now with a server to prove it. MessageTest covers what a parser reads back, which is the only way to settle folding and encoded words: a folded accented subject returns whole, twenty-five recipients come back in order with their names, and attachment bytes are fetched through the API and compared, so a base64 boundary handled wrongly cannot pass by looking plausible. 26 end-to-end tests, up from 10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The one production class no tier touched. It now covers the same ground as the stream transport against the same kind of listener -- writes, reads, a payload past one write, hang-ups, use before connect and after close, a refused connection -- plus the buffering it alone does, since it takes whole reads off the socket and hands them out in slices. Every body runs inside a scheduler, carrying failures back out rather than letting Swoole turn an assertion into a fatal. Holding both transports to the same promises is what found the place they do not keep them. Swoole checks a certificate name with X509_check_host(), which reads the DNS entries and not the address ones, so an IP literal cannot pass verification however the certificate is written; streams check both. PHP cannot change that, so the transport now refuses the combination up front and says which knob fixes it, rather than letting the handshake fail with SSL verify failed and nothing else. Two end-to-end tests run the whole client over it against a real server, which is the only way to reach Swoole's own handshake: one upgrading through STARTTLS with enableSSL, one encrypted from the socket up. 165 unit and 28 end-to-end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
testConnectingToNobodyFails took an ephemeral port, released it, and dialled it. On Linux loopback that can pair with the source port the kernel has just handed out and connect to itself, so the connection succeeds and the test fails -- which is what CI saw for the coroutine transport. The stream transport had the same test and had only been lucky. Port 1 instead: outside the ephemeral range, so there is nothing to pair with and nothing listening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Fills in
packages/smtp, which #129 landed as a placeholder. Three commits: the specifications and a survey of prior art, the design written against them, then the implementation.Scope is submission — hand a message to a configured server with authentication and TLS. Resolving MX records and delivering to strangers is a different job with a different failure model, and is out of scope.
What is here
23 classes:
Client,Envelope,Message,Address,Attachment,Reply,Result,Capabilities,Encryption,Tls,Mime\{Encoding,Part},Transport\{Transport,Native,Swoole},Auth\{Authenticator,Plain,Login,XOAuth2}and five exceptions. Sixteen RFCs are vendored underdocs/rfc, in the path.vale.inialready exempts.Where it departs from the four implementations surveyed
docs/prior-art.mdrecords the survey — Gonet/smtp,emersion/go-smtp, Symfony Mailer, and PHPMailer, whichpackages/messagingwraps today.A send is not a boolean. RFC 5321 lets a server refuse some recipients and accept others while the message still reaches the rest.
Resultcarriesaccepted,rejectedas address toReply, and the queue identifier.TransactionExceptionis thrown only when every recipient is refused, and itsReplyanswers the question a queue-backed sender actually has:isTransient()means put it back,isPermanent()means do not. PHPMailer sets oneErrorInfostring, whichmessagingcopies into every per-recipient result, so today that distinction cannot be made at all.The envelope is its own type and the client never parses what it sends.
Bccaddresses reachRCPT TOand no header. Only PHPMailer merges the two, which is why itsSMTPclass cannot be used on its own.Encryption is an enum, not Symfony's
autoTlsandrequireTls— two booleans whose four combinations include one that means nothing.No keep-alive, restart threshold or ping threshold. A
Clientis one connection; pooling is whatutopia-php/poolsis for.Two subtleties, both silent when wrong
The read buffer is cleared with the capability map after
STARTTLS. RFC 3207 section 4.2 requires forgetting what was said before the handshake, and that has to include bytes already sitting unread — they arrived in the clear and could have been injected ahead of the upgrade. The test that proves it first looked like a failure: clearing the buffer discarded the rest of the scripted replies, which is exactly right, so the double now models a server that stays quiet until the handshake finishes and the assertion checks the capabilities came from the reply read afterwards.Dot-stuffing is a generator holding two pieces of state, not a
str_replace: whether the next byte opens a line, and a trailing\rwhose\nhas not arrived yet. Without the second, a chunk boundary falling inside a CRLF reads as a bare CR and the stuffing misses — which truncates the message at that point, and only for messages large enough to split there. Eleven cases inStuffingTest.php, plus an end-to-end check that a message containing a lone.line survives.Testing
composer testis 85 unit tests driving a scripted in-memory transport: no sockets, no containers.composer test:e2eis 10 more against Mailpit in compose, on host ports 11026 and 18026.The end-to-end tier earned its place by contradicting two assumptions:
Bcc,Return-PathandReceived, so "no blind recipient reaches a header" can only be asserted about the bytes we wrote. The test asserts both halves — the server did see the blind recipient, and we did not put it in a header.SIZE 0, which exercises the rule that a declared zero means no fixed maximum rather than a zero-byte limit.The server presents a committed self-signed pair from
tests/fixtures/certs, following thenatsprecedent. Generating one in the container entrypoint was the first attempt and does not work: the image has noopenssland no/data, and depending onapk addat test time would be flaky in CI.MP_SMTP_ALLOWED_RECIPIENTSgives the suite an address the server genuinely refuses, which is what makes partial acceptance testable against real replies.Deliberately not done
PIPELININGis parsed but unused. It saves a round trip per recipient and moves error attribution from "the reply to this command" to "the third reply in this group". The reply reader is shaped so it can be added.dnsversion 2 redesign — the surface is small enough to decorate.messagingmigration. Dropping PHPMailer fromAdapter\Email\SMTPneeds a releasedsmtpfirst, since CI tests changed packages against Packagist and a consumer cannot adopt an unreleased sibling in the same pull request.Checks
bin/monorepo check smtp— Pint, PHPStan and Rector passbin/monorepo test smtp— 85 unit, then compose up and 10 end-to-endbin/monorepo validate— passesvale README.md docs packages— 0 errors🤖 Generated with Claude Code