feat(twig): add Craft-tolerant Twig AST parser + phpunit harness - #1
Open
Jensderond wants to merge 18 commits into
Open
feat(twig): add Craft-tolerant Twig AST parser + phpunit harness#1Jensderond wants to merge 18 commits into
Jensderond wants to merge 18 commits into
Conversation
…nbounded-all checks Claude-Session: https://claude.ai/code/session_01CNFGL89Pp8zKRn6ajL5rSg
The ForNode branch in TwigPerformanceAnalyzer::walk() pushed the loop's
frame onto loopStack before walking the loop's own children, including
seq (the source expression). This caused checkQueryInLoop() to falsely
flag the loop's source query as running inside the loop, even though it
executes once before iteration begins — spuriously flagging the most
common Craft Twig idiom, e.g.:
{% for entry in craft.entries.section('news').all() %}...{% endfor %}
Fix: walk seq in the outer loop context before pushing this loop's
frame, then push the frame and walk the remaining children. This is
still correct for nesting — a query used as a nested loop's source is
walked with the outer frame still on the stack, so it is still flagged.
Adds two regression tests asserting absence (top-level loop source not
flagged) and presence (nested loop source still flagged).
Claude-Session: https://claude.ai/code/session_01CNFGL89Pp8zKRn6ajL5rSg
- Add TwigPerformanceRule, a Rule<CollectedDataNode> that scans configured template paths and reports Finding objects from TwigPerformanceAnalyzer as PHPStan RuleErrors. Exposes a public scanTemplates() seam for unit testing without a full PHPStan kernel. - Wire craftTwigPerformance parameters/parametersSchema and the TwigTemplateParser / RelationFieldRegistry / TwigPerformanceAnalyzer / TwigPerformanceRule services into extension.neon, sharing single instances via service references. - Add nplusone.twig / clean.twig fixtures and TwigPerformanceRuleTest. Claude-Session: https://claude.ai/code/session_01CNFGL89Pp8zKRn6ajL5rSg
Declare twig/twig ^3.27 as a runtime dependency: src/Twig/* binds to Twig's node API but it was only present transitively via the craftcms/cms dev dependency. Bump craftcms/cms to ^5.10.8 (which ships twig ~3.27.0) so the dev environment matches the declared floor, and update the node-helper comment to reflect the verified Twig version. Document the result-cache gap: the Twig checks scan templates outside PHPStan's per-file analysis, so a warm result cache can serve stale Twig findings when only templates change. README now recommends a cold cache (CI) or clear-result-cache after editing templates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgpX7sHMRT3keuAe8XhkBA
…, ensure checks always run Three related fixes to how the Twig checks are delivered: - Shared traversal: extract TwigTemplateScanner + ScannedTemplate so both TwigActionInputRule and TwigPerformanceRule walk the configured template tree once and read/parse each .twig file at most once (lazily), instead of each running its own RecursiveDirectoryIterator. - Result-cache correctness: add TwigTemplateCacheMetaExtension (a ResultCacheMetaExtension) that hashes every discovered template's path + contents into PHPStan's result-cache key. Template edits, additions and removals now invalidate the cache and force re-analysis, so the out-of-band Twig scanning can no longer serve stale findings. - Reliable execution: add AnalysisMarkerCollector. The Twig rules piggyback on the CollectedDataNode hook, which PHPStan's AnalyserResultFinalizer skips entirely when no collector produced any data. Since the only other collector emits nothing unless an analysed file defines a yii\web\Controller subclass with action* methods, a project without such a controller in scope would silently skip every Twig check. This marker emits one datum per analysed file, keeping the CollectedDataNode rules reliably reachable. Rules, identifiers, tips and config toggles are unchanged. Tests cover the scanner (discovery, dedup, stable order, laziness), the cache meta hash (changes on edit/add/remove, stable key) and the marker collector. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgpX7sHMRT3keuAe8XhkBA
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PgpX7sHMRT3keuAe8XhkBA
The N+1 check analyses each template in isolation, so it assumed a loop
source with no visible .with() was un-eager-loaded. That produced false
positives on navigation partials (node.twig, footer.twig, careers nav)
where the elements arrive pre-eager-loaded from a parent layout.
- resolveEagerLoaded(): loop sources rooted at a variable not traceable
in this template (supplied across {% include %}/{% embed %}) now yield
null ("unknown"), which suppresses the N+1 check instead of assuming
none. Queries we can read (craft.*, .find()/.relatedTo(), {% set %}
vars, enclosing loop elements) still trust an empty set and flag.
- unwrapFallback(): see through the `craft.…all() ?? []` idiom
(NullCoalesceBinary on Twig >=3.16, ConditionalExpression on older).
- checkNestedRelationAll() made eager-aware for consistency.
.with(['children.children.children']) was stored as a single key, so the suppression check isset($eager['children']) never matched and the nesting never propagated into child loops — flagging eager-loaded relations at every level (navigation menu pattern). - isEagerLoaded(): a relation is covered when it is the head segment of any eager path, not only an exact key. - resolveEagerLoaded(): descend the enclosing loop's eager paths through the relation(s) accessed, so a deep path keeps covering each nesting level while relations beyond its depth still flag. An explicit .with() on the source overrides the inherited state. - descendEagerPaths()/relationSegments() helpers.
The 1.4.0 version bump edited composer.json without refreshing the lock file's content-hash, so a clean 'composer install' aborted with 'Your lock file does not contain a compatible set of packages'. The resolved dependency set was still valid; only the hash needed regenerating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01551h5FzhM7dza9reEzHMoK
Two loop-context defects in TwigPerformanceAnalyzer:
- checkChain() compared relation chains only against the innermost loop
frame, so an N+1 on an outer loop variable inside a nested loop was
never flagged (e.g. entry.author inside a loop over sizes). It now
resolves the chain root against every active frame via
elementLoopFrame(), which is additionally shadow-aware: an inner loop
rebinding the same variable name to plain literal values masks the
outer element binding instead of being skipped over.
- The {% else %} branch of a for loop runs at most once (only when the
sequence is empty), but it was walked with the loop frame still on the
stack, flagging queries there as per-iteration queries. It is now
walked after the frame is popped.
Also removes the unused isActiveElementLoopVar() helper.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01551h5FzhM7dza9reEzHMoK
…er sharing The README's result-cache section predated TwigTemplateCacheMetaExtension and still told users to run clear-result-cache manually after template edits; the meta extension makes that invalidation automatic. Also note the twig/twig ^3.27 floor against Craft's pinned Twig minor, and correct the scanner docblock: templates are read lazily at most once per consumer, not once globally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01551h5FzhM7dza9reEzHMoK
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
2 participants
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.
Claude-Session: https://claude.ai/code/session_01CNFGL89Pp8zKRn6ajL5rSg