Conversation
Benchmark ResultsComparison of Open to see the benchmark resultsNo benchmark changes above ±5%. Generated by phpbench against commit 032f9b4 |
a0040c6 to
138253d
Compare
138253d to
af2077e
Compare
af2077e to
032f9b4
Compare
|
Rebased on The cause was the sub-parser cache being cleared at the start of every top-level It was there for one reason: rules are mutable now, so a sub-parser built from a rule goes stale the moment someone calls public function getRule(string $rule): ?Rule
{
$this->cache = [];
// …
}
Measured after the change, four interleaved runs against
Faster on both, which is what I would expect: nested content no longer instantiates a fresh Also re-verified behaviour after the rebase: 377 tests green, and a personal dataset of real-world Markdown renders byte-identically to |
Rewritten to follow the direction in #13: the parser is the single rule registry, tokens pull from it, and a rule declares which tokens it applies to.
The API
IsRulesupplies the three methods, andParser::getRule()/Markdown::getRule()return a registered instance to reconfigure.The thirteen
forToken($this, [ … ])lists are gone. So are thenew BoldRule()calls they contained: nested content now runs the registered instances, which is what makes removal, reconfiguration and addition propagate at all.The one place I deviate
The proposal has each rule hold an explicit
$supportedTokensallowlist. I measured what that costs on the current code: thirteen tokens, thirteen rules, 98 edges. Transposing the matrix does not shrink it —TextRulewould list 13 tokens,LinkRuleandSocialHandleRule12,BoldRuleandItalicRule11.That matters because the matrix is what broke in the first place. #47 existed because those lists drifted: fifteen valid CommonMark/GFM combinations rendered as literal text, purely because a rule was added to some lists and not the others. An allowlist that every rule has to keep complete is the same failure mode, re-signed.
So
$tokenSupportis an override map rather than an allowlist, and the default answer comes from the context the rule is written for:RuleContext::INLINE— runs inside every token that holds inline content.RuleContext::BLOCK— runs on the document, and on the containers that opt in.The whole matrix is then 16 declarations instead of 98, and all sixteen are real:
addTokenSupport()andremoveTokenSupport()are still the escape hatch — they are how those sixteen are written, so the extension path and the built-in path are the same path.This also answers the requirement the allowlist version misses. An extension that adds a block rule almost always adds its token with it. Under an allowlist, its content parses nothing until the extension calls
addTokenSupport(MyToken::class)on every registered inline rule — including the ones a different extension registers later. Under a context default, a custom token gets the inline rules with nothing declared at all;RuleRegistryTest::a_custom_token_gets_the_registered_inline_rulesis that case.What this costs
Rulegains three methods.use IsRule;is the one-line migration; the custom rules inMarkdownTestshow it.ParagraphRuleandRawRulelosereadonly. Token support is mutable state, and a readonly class cannot hold it.new Parser(rules: [new HeadingRule()])now really means that rule only. Nested content used to arrive with a list the token rebuilt, so an isolated parser silently got inline parsing it never asked for. It no longer does, which is most of the test diff: the isolated rule tests now register the rules they depend on, and the cases that asserted''for "no rule matched" assert the literal text instead, becauseTextRuleis registered and that is what a parser does. Say the word if you would rather keep the old convenience — I would rather surface it than paper over it.getRule()drops the sub-parser cache. Rules are mutable and shared, so a sub-parser built from a rule stops being valid the moment someone reconfigures it — this is the "this should be properly cached" note in your sketch.getRule()is the only way to reach a registered instance, so invalidating there closes the window exactly when it opens, at no cost on the parsing path. Without it,getRule(…)->addTokenSupport(…)silently does nothing after the first parse.Evidence
RuleRegistryTestcovers the three requirements from Make tokens and rules more configurable #13 plus the custom-token case and the stop-char leak below.main. This refactor changes no output.mainon both fixtures: −5.5% small, −2.4% large, averaged over four interleaved runs. The earlier +9.7% the bot reported on01-smallwas the per-document cache clear; it is gone.One bug this had to fix
Stop chars were stored on the rule (
$rule->stopChars .= …). That was harmless while every rule set had its own instances. Once sets share the registered instance, one set's stop chars leak into the next and**bold**stops parsing on the second call.activateRules()now clones aNeedsStopCharsrule when the set's stop chars differ from the ones it carries, so the registry instance keeps its own and each set gets a copy.