Conversation
4.0 groundwork. The polymorphic array-returning parse() is deprecated (removed in 5.0); the typed methods are now the entry points. - Extract the state-machine core into a private parseInternal(); parse() becomes a thin @deprecated shim over it, and parseSingle/parseMultiple/ parseStream call parseInternal directly (no longer routed through the deprecated method). parse() output is byte-identical. - Rewrite the testspec runner onto parseMultiple()->toArray() / parseSingle()->toArray() — validates the typed path and drops the suite's dependency on parse(). 236 assertions unchanged. - Docs: README (Basic Usage, ParseOptions examples, Other Examples), cookbook, UPGRADE, and ARCHITECTURE now use the single-purpose methods; parse() is presented only as the deprecated legacy shim. - Roadmap: parse() deprecation recorded in the ledger; removal moved to a new v5.0 section. CHANGELOG [Unreleased] Deprecated entry added. 110 tests / 7214 assertions, PHPStan L8, Psalm, CS all green.
Completes the 3.9 deprecation: local-part validation is folded back into a private ParseContext-based validateLocalPart() (dropping the array-shaped BC shim and its psalm-suppress), and validateDomainName() is now private too. Both took the parser's internal accumulator and were never a supported extension point; validation is customized through ParseOptions. BREAKING for any subclass that overrode them. Removed the now-moot BC-override test; CHANGELOG Removed entry and roadmap/ledger updated. 109 tests / 7187 assertions, PHPStan L8, Psalm, CS all green.
…tInstance - Promote the 5 ParseOptions state fields (bannedChars, separators, useWhitespaceAsSeparator, lengthLimits, allowedWhitespace) to public readonly, assigned once in the constructor. The getX() accessors remain. - Remove the 7 @deprecated mutating setters (deprecated since v3.0). No call sites; configure via the constructor or withX() builders. - Deprecate Parse::getInstance() (removed in 5.0). The static singleton carries process-global state and is pinned to the LEGACY preset, so it silently applies permissive defaults; modern PHP uses explicit instantiation / DI. Docs and tests switched to new Parse(). - Replace the deprecated-setters test with one covering the readonly properties + withX() builders. Prune 7 now-stale psalm baseline entries. - CHANGELOG/ROADMAP/ledger updated. 109 tests / 7165 assertions, PHPStan L8, Psalm, CS all green.
Aligns the internal accumulator with the codebase's camelCase convention. Mechanical rename of ~20 fields across Parse.php and ParseContext.php (property declarations + all $ctx->/$this-> accesses). The public snake_case output-array keys are string literals in addAddress() and are untouched, so parse() / toArray() output is byte-identical. 109 tests / 7183 assertions (testspec output unchanged), PHPStan L8, Psalm, CS all green.
Encodes the context's three concerns structurally: the input snapshot (chars/len/multiple/emails) and hoisted config (separators/bannedChars/ useWhitespaceAsSeparator/allowedWhitespace) are now public readonly constructor-promoted properties, so a state handler can no longer mutate config; only the per-address accumulator stays mutable. parseInternal() now builds chars/len/config before constructing the context and passes them in (they were previously assigned after `new`). $chars/$len are still kept as loop locals for the hot counter. 109 tests / 7186 assertions, PHPStan L8, Psalm, CS all green.
Introduces src/ParserState.php (backed int enum, values matching the former Parse::STATE_* constants) and types ParseContext::$state/$subState as ParserState. The parser can no longer hold an out-of-range state, and the old "subState 0 default is not a valid start" caveat is gone — an un-initialized enum property is a type error, not a silent wrong value. Mechanical: 75 self::STATE_X -> ParserState::X, the dispatch switch and the in_array() state check now use enum cases, and the one state-interpolating log line uses ->name. Behavior-preserving. Perf: enum === is identity comparison (≈ int), so no hot-loop regression is expected; verified by the CI "Benchmarks (vs base)" job (≤1.5x base). Local phpbench is unreliable here (Xdebug + opcache.enable_cli=0 time it out). 109 tests / 7208 assertions, PHPStan L8, Psalm, CS all green.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72 +/- ##
============================================
+ Coverage 95.84% 96.42% +0.57%
+ Complexity 446 438 -8
============================================
Files 7 7
Lines 1108 1091 -17
============================================
- Hits 1062 1052 -10
+ Misses 46 39 -7
🚀 New features to boost your workflow:
|
parse() has no internal callers now (the typed methods use parseInternal() directly), so Psalm's findUnusedCode flags the public deprecated shim. It's still called by external code until its 5.0 removal. (Local runs passed on a stale cache; CI runs fresh.)
UPGRADE.md: new v3.x -> v4.0 section — breaking changes (removed ParseOptions setters with a setter -> withX() migration table; validateLocalPart/ validateDomainName now private), the parse()/getInstance() deprecations, the readonly state fields, and a no-action note for the internal changes (ParserState enum, ParseContext modernization). ROADMAP.md: 4.0 is now explicitly lean (breaking cleanup + internal modernization only). Moved the deferred features out: DNS/MX -> v4.1, confusable-against-target-list -> v4.2 (both additive), and RFC 6854 group syntax -> v5.0 (breaking: new group-node result shape).
…keystone Encodes the strategy connecting three longer-horizon goals — PHP framework integration, localized error messages, and ports to other languages — which all converge on decoupling error identity (ParseErrorCode + named parameters) from presentation (a rendered, localizable string). - New "Strategic direction (North Star)" section. - v4.1 is now the structured-errors keystone: messageParameters on ParsedEmailAddress + a swappable MessageProvider interface (sketched), additive so invalid_reason stays English by default; and making testspec.yml code+params-normative for ports. - DNS/MX -> v4.2, confusable-target -> v4.3. RFC 6854 groups reclassified as additive (Option B: flat emailAddresses + a groups() view) -> 4.3, off 5.0.
Moving the suite off the deprecated parse()/getInstance() and removing the setter test left a few methods with no caller. Cover them, and add reachable branch tests: - getInstance() — deprecated but public until 5.0; assert the shared instance. - ParseOptions::getMaxLocalPartLength() and the 5 fluent builders the toggle test was missing (withAllowObsRoute, withTrimSingleAddressWhitespace, withStrictMultiWhitespace, withRejectTrailingDot, withDetectConfusableDomain). - Quoted UTF-8 local part under rfc5321 -> Utf8NotAllowedInLocalPart (validateLocalPart's UTF-8 gate). - Quoted local part re-quoted after a normalizer rewrites it. Every method in Parse and ParseOptions is now covered. Overall lines 95.49% -> 96.18%, methods 87.78% -> 91.11%.
The "coverage decreased" was measurement noise, not a real regression: PropertyTest fuzzes from a time-based seed by default, so the coverage job's line count (and the Codecov delta) jittered run-to-run — adding tests could even show a lower number. Two seeded coverage runs of identical code differed by ~0.6%. - Pin SEED=12345 for the coverage job so the measured number is deterministic and comparable (this seed lands at 96.18%, above the prior ~95.84% base). The 8.1-8.6 matrix jobs stay unseeded so they keep fuzzing. - Add two behavioral branch tests (mixed quoted/unquoted display name; NFC normalization of an unquoted local part under rfc6531). Coverage is now reproducible; overall ~96.18% lines, every Parse/ParseOptions method covered. 114 tests / 7234 assertions, PHPStan L8, Psalm, CS green.
Push coverage after the deterministic-seed fix: - Add behavioral edge tests (mid-string quoted display-name word, etc.). - Mark the two provably-unreachable defensive blocks @codeCoverageIgnore: the switch `default:` case (impossible now that $ctx->state is a ParserState enum with every case handled) and the ParserConfusion branch (a 500k-input fuzz confirmed it's dead). These are excluded from the denominator rather than faked with contrived internal-state tests. Overall line coverage 96.18% -> 97.13% (deterministic, SEED=12345). PHPStan L8, Psalm, CS green.
mmucklo
marked this pull request as ready for review
August 29, 2026 03:47
Adds an explicit test for validateIpGlobalRange behavior via IP-literal domains (private IPv4 -> IpNotInGlobalRange, link-local IPv6 -> Ipv6NotInGlobalRange, global IPv4 accepted). Behavioral coverage of the global-range path that no test asserted directly. Project coverage 96.95% (up from the ~95.84% base); patch 95.17% (>70% target). Remaining uncovered changed-lines are version-conditional (PHP 8.1 IP-range fallback) or defensive branches preempted by earlier checks.
…age seed Staff+ review follow-ups (PR #72), all low-severity: 1. Deprecate the five ParseOptions pass-through getters (getBannedChars, getSeparators, getUseWhitespaceAsSeparator, getLengthLimits, getAllowedWhitespace) now that the fields are public readonly — they duplicate the properties. Read the property; removed in 5.0. Internal call sites repointed to the properties; @psalm-suppress PossiblyUnusedMethod on the now-caller-less public accessors. getMax*Length() stay (they read into $lengthLimits). CHANGELOG/ROADMAP/UPGRADE updated. 2. Fix stale comment: the per-parse ParseContext "keeps the parser reentrant", not "keeps parse() reentrant" — reentrancy lives in parseInternal() now. 3. Coverage seed: switch the pinned SEED 12345 -> 1 and reword the comment. The value is now chosen for reproducibility only, not to maximize the number; SEED=1 still lands at 96.53% (above the 95.84% base) purely from the real tests + @codeCoverageIgnore, so no metric-gaming. 116 tests / 7222 assertions, PHPStan L8, Psalm (fresh), CS all green.
Staff+ review #2 follow-up. When the spec fixtures were rerouted off the deprecated parse() onto the typed API's ->toArray(), two gaps opened: parse() (the raw-array BC contract, live until 5.0) lost its broad coverage, and the fixtures no longer directly assert that ->toArray() matches parse()'s raw array — so a future key drop/rename in toArray() could diverge unnoticed. Add an explicit equivalence test across single/multiple x valid/invalid, name-addr, IP-literal, and comment cases. It restores parse() coverage in both modes and fails the moment the two public shapes diverge. No correctness bugs found in the review; this closes the one genuine gap. 117 tests / 7202 assertions, PHPStan L8, Psalm, CS green.
…ethod noise Principal-review follow-ups (PR #72). A. The 4.0 immutability/DI thesis was self-contradicting: ParseOptions was made immutable and getInstance() deprecated as global-mutable-state, yet Parse still let you swap its whole config on a live instance. - Parse now implements Psr\Log\LoggerAwareInterface; setLogger() returns void (was fluent) — standard PSR-3 logger injection. BREAKING for chained calls. - Deprecate Parse::setOptions() (config should be constructor-injected / immutable); removed in 5.0, at which point $options/$logger go readonly. - CHANGELOG/UPGRADE document both. B. Stop fighting Psalm one method at a time: findUnusedCode flagged every public/BC method with no internal caller (the parse() shim, the 5 deprecated getters, ...) as PossiblyUnusedMethod. Suppress PossiblyUnusedMethod (and MissingOverrideAttribute — #[\Override] is 8.3+, we target 8.1+) at the config level; remove the 6 inline @psalm-suppress annotations. This pruned the psalm baseline from 47 entries to 0. C. Roadmap: setOptions removal + immutable Parse in v5.0; and two long-term god-object items (the 27-param ParseOptions constructor; the ~1,700-LOC Parse class) recorded in the backlog. 117 tests / 7260 assertions, PHPStan L8, Psalm (fresh, empty baseline), CS green.
Adds rector/upgrade-4.0.php plus three custom rules that auto-fix the
mechanical parts of the 4.0 upgrade (opt-in — the consumer runs it and
reviews the diff; nothing runs on composer update):
- GetInstanceToNewParseRector: Parse::getInstance() -> new Parse()
- ParseOptionsGetterToPropertyRector: $o->getBannedChars() -> $o->bannedChars
(+ separators/useWhitespaceAsSeparator/
lengthLimits/allowedWhitespace; getMax*
length helpers left alone)
- ParseOptionsSetterToWithRector: $o->setBannedChars($v) ->
$o = $o->withBannedChars($v) (+ 3 more),
only when the receiver is directly
assignable
Semantic migrations (parse() array->object, setMax*Length, setOptions,
chained setLogger) are intentionally left to the documented manual steps.
Verified end-to-end: rector applies all three rules correctly to a fixture and
leaves the excluded cases untouched. rector/rector added as a dev dep; the
rules are linted (cs-fixer) and type-checked (PHPStan) in CI. UPGRADE.md gains
an "Automated migration (Rector)" section; CHANGELOG Added entry.
117 tests / 7225 assertions, PHPStan L8, Psalm, CS all green.
Completes the deprecate -> warn -> auto-fix story. - Every deprecated public method (parse(), getInstance(), setOptions(), and the five ParseOptions pass-through getters) now calls trigger_deprecation() from symfony/deprecation-contracts, so callers get an E_USER_DEPRECATED at runtime — not just a docblock. symfony/phpunit-bridge and similar tools aggregate these into a call-site report; the shipped Rector config then auto-fixes most. - Add symfony/deprecation-contracts to require (a tiny, standard, zero-dep package; the ecosystem-idiomatic way to signal deprecations). - phpunit.xml: convertDeprecationsToExceptions="false" so the suite's own intentional deprecated-API tests emit notices without failing. - New test asserts each deprecated method fires a notice naming it. - composer.json: add "support" (issues/source/docs) so Packagist surfaces the UPGRADE guide. (Composer has no native auto-run/warn-on-update hook; runtime deprecations are the correct signal.) 118 tests / 7229 assertions, PHPStan L8, Psalm, CS all green.
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.
4.0 is a breaking-modernization release. Same parser, tighter surface: the API is modernized and the internals are typed, but a valid address parses identically — output is byte-identical, verified by the full spec corpus.
High level — what's merging
Everything sorts into four buckets. Only the first two touch callers.
flowchart LR B["🔴 BREAKING · 2<br/>7 setters removed<br/>validators → private"]:::brk D["🟠 DEPRECATED · 2<br/>parse()<br/>getInstance()"]:::dep I["🔵 INTERNAL · 3<br/>ParserState enum<br/>snake → camelCase<br/>readonly config"]:::int X["🟢 DOCS · 3<br/>UPGRADE guide<br/>ROADMAP<br/>CHANGELOG"]:::saf B --> R D --> R I --> R X --> R R["v4.0<br/>0 behavior changes<br/>net −39 LOC"]:::rel classDef brk fill:#f8e3e2,stroke:#cf3b3b,color:#7a1f1f classDef dep fill:#f6ecd6,stroke:#a9721a,color:#5a3d0e classDef int fill:#e2ecfb,stroke:#2f6bd6,color:#1a3c73 classDef saf fill:#e0f0e7,stroke:#2c8253,color:#164a2e classDef rel fill:#e7e4fb,stroke:#5b4fe3,color:#2c2470,stroke-width:2pxMid level — the public surface, before → after
parseSingle/parseMultiple/parseStreamParseOptions::withX()buildersparse(string, bool $multiple)@deprecated(→5.0)getInstance()@deprecated(→5.0)ParseOptions::setX()validateLocalPart()/validateDomainName()protectedprivateSTATE_*(12 consts)enum ParserState: intParseOptionsstate fieldspublic readonlyThe two breaking rows are the whole upgrade. Migration is mechanical — full table in the new
v3.x → v4.0section ofUPGRADE.md:Low level — the numbers GitHub's diff buries
GitHub shows +787 / −722 (1,509 lines touched) — it reads like a rewrite. It isn't:
snake_case → camelCaseSTATE_* → ParserState::Per-file:
Parse.php792 ·ParseContext.php164 ·ParseTest.php162 ·ParseOptions.php106 ·ParserState.php37 (new) ·PropertyTest.php10.Proof it's safe
testspec.ymlcorpus asserts exact output; the runner was re-pointed atparseMultiple()->toArray()and every case still matches → byte-identical.ParserStateenum lands within ±2% of master, well under the 1.5× gate (enum===is identity comparison).withX(), nothing changes; otherwise ~2 minutes of find-and-replace.Notable / worth a look
parse()— a strict improvement that shipped for free with the deprecation.parse()is a shim over a new privateparseInternal()core —parseSingle/parseMultiple/parseStreamcall the core directly, so the deprecated method has no internal callers.ParseOptionsproperty isreadonly; you can't keep the mutating setters and readonly, which is why removal (not just deprecation) is the right call.ROADMAP.md(incl. the new North Star on error identity vs. presentation).Draft until the UPGRADE guide gets a final read and a
4.0.0-beta1tag is cut to soak.