Refactor: decompose Parse::parse() into ParseContext + per-state handlers - #71
Open
mmucklo wants to merge 4 commits into
Open
Refactor: decompose Parse::parse() into ParseContext + per-state handlers#71mmucklo wants to merge 4 commits into
mmucklo wants to merge 4 commits into
Conversation
Replace the ~24-key $emailAddress accumulator array threaded through parse() and its validation helpers with a typed ParseContext object. Property names mirror the former array keys so the change is a pure mechanical conversion with no behaviour change. A fresh ParseContext is created per parse() call and reset per address via resetAddress(); it is never stored on the Parse instance, preserving reentrancy across a localPartNormalizer callback. Type the addAddress() parameters (array/ParseContext/int) and drop the always-true isset() guard on the non-nullable domain property; refresh the PHPStan and Psalm baselines to drop the now-obsolete array-shape entries.
Decompose the ~772-line parse() state machine into a thin switch that dispatches to one handler per parser state, plus sub-handlers for the heavy STATE_ADDRESS branches (CFWS, '@', '.', atext, non-atext). parse() is now ~190 lines. Loop control (state/subState/commentNestLevel) and the hoisted input and config move onto ParseContext so the handlers read them without long parameter lists; behaviour, error codes and output are unchanged. Refresh the Psalm baseline for the state-machine narrowing false-positives that shift when the discriminant becomes a context property (PHPStan level 8 handles the mutation across calls and stays clean).
Fold the atext and period handling back inline into handleStateAddress so the dominant STATE_ADDRESS path makes a single method call per character instead of two. The larger, less-frequent branches (CFWS whitespace, '@', non-atext) stay in their own helpers. Behaviour is unchanged (full suite still green). Under opcache+JIT the batch-parsing benchmarks now run at or below the pre-refactor baseline.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71 +/- ##
============================================
- Coverage 96.01% 95.74% -0.27%
- Complexity 434 445 +11
============================================
Files 6 7 +1
Lines 1078 1105 +27
============================================
+ Hits 1035 1058 +23
- Misses 43 47 +4
🚀 New features to boost your workflow:
|
Post-review cleanup for the parse() decomposition.
- Rename the parse-state local (and every handler parameter) from
$emailAddress to $ctx: it holds a ParseContext, not an address, and
the old name shadowed the concept of the address being built. Pure
mechanical rename, no behavior change.
- Make ParseContext::resetAddress(int $state, int $subState) the single
source of truth for per-address reset. It now also sets state/subState
and zeroes commentNestLevel, which nothing reset before: an
unterminated comment could leak its nesting level into the next address
in a batch, self-healing only because '(' reassigns it to 1. Both the
initial setup and the per-address reset in parse() now go through it.
Roadmap: mark the parse() readability refactor delivered and record the
remaining non-blocking follow-ups (snake_case fields, structural split of
the context's three concerns, chars/len duplication, handleStateAddress).
108 tests / 7199 assertions, PHPStan and CS clean.
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.
Decomposes the ~772-line
Parse::parse()state machine. Pure behavior-preserving refactor — no parsing logic, conditions, ordering, error codes, or output shape changed.What changed
parse(): 772 → 193 lines — setup + a thinswitch ($ctx->state)dispatching to one handler per state, plus post-loop finalization.src/ParseContext.php— a per-parse object holding the ~24 accumulator fields + loop control + hoisted input/config as typed properties. Created fresh perparse()call and never stored on theParseinstance, so reentrancy is preserved (thelocalPartNormalizercallback can re-enterparse()).handleStateTrim,handleStateAddress,handleStateQuote,handleStateComment, …); the hot atext/period branches are inlined for performance.Independent verification (not just the author's report)
SEED=1/42/1337/99999; PHPStan level 8, Psalm, and CS all clean. Reentrancy confirmed with alocalPartNormalizerthat re-entersparse()— returns the correct result, no context corruption.master), Xdebug off. Refactored is faster — 1.88s vs 3.18s for 125k parses. (The sandbox's default config has Xdebug on + opcache-CLI off, which penalizes method calls; that instrumentation artifact accounts for any interpreted slowdown seen without it.)$ctx->state(incl. the defensivedefault:dead-code case), not real issues.Review notes
ParseContextkeeps the old array keys as snake_case property names (original_address,local_part_parsed, …) to keep the conversion mechanical and behavior-identical. Idiomatic camelCase could be a follow-up.