[7.x] Fix: restore StatementsAnalyzer's dropped $depth/$root_scope properties#11900
Open
alies-dev wants to merge 1 commit into
Open
[7.x] Fix: restore StatementsAnalyzer's dropped $depth/$root_scope properties#11900alies-dev wants to merge 1 commit into
StatementsAnalyzer's dropped $depth/$root_scope properties#11900alies-dev wants to merge 1 commit into
Conversation
Merge commit 290eaec (6.x into master) resolved a conflict in StatementsAnalyzer's property/constructor block by taking 6.x's version wholesale, while the separate analyze() method kept master's version wholesale. The two halves no longer matched: - private int $depth = 0 was dropped, but $this->depth++/-- in analyze() remained, causing an uncaught RuntimeException on PHP 8.2+ (Psalm's ErrorHandler converts the dynamic-property deprecation into a crash). - The $root_scope constructor-promoted property was dropped too, so $this->root_scope always read as an undefined property. - The constructor logic that populates $this->taint_flow_graph and $this->variable_use_graph from the shared codebase graph was replaced by 6.x's simpler version, which never assigns either property. This silently disabled unused-variable detection and taint tracking wherever the crash above didn't already stop execution (e.g. under PHPUnit, which doesn't install that ErrorHandler). Restores all three to match the pre-merge master constructor (commit 5c1c31c) exactly, alongside the type_variable_tracker logic 6.x legitimately added. Full test suite goes from 374 failing/erroring tests to 72 (all pre-existing/unrelated), and self-analysis from 6139 issues to 12 (all pre-existing/unrelated). Fixes vimeo#11899
alies-dev
changed the title
StatementsAnalyzer's dropped $depth/$root_scope properties
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.
Context
Merge 290eaec resolved a conflict in
StatementsAnalyzer.phpby taking 6.x's constructor wholesale while keeping master'sanalyze()wholesale. That droppedprivate int $depth = 0;, the$root_scopeconstructor property, and the constructor logic populating$taint_flow_graph/$variable_use_graph— not just$depthas #11899 describes. Result: PHP 8.2+ crashes (dynamic-property deprecation, converted to an uncaughtRuntimeExceptionby Psalm's ownErrorHandler), and even without the crash (e.g. under PHPUnit) unused-variable/taint detection silently no-ops since$variable_use_graphstays null. Confirmed pre-existing via upstream CI on290eaec2: every unit-test chunk fails across PHP 8.1-8.5.Solution
Restores all three to match the pre-merge master constructor (
5c1c31c62), interleaved with 6.x's latertype_variable_trackeraddition. All 6new StatementsAnalyzer(...)call sites already pass the 3rd argument PHP was silently dropping.Validated: crash gone; full suite 374 failing/erroring tests → 72 (all pre-existing, unrelated); self-analysis 6139 issues → 12 (all pre-existing, unrelated).
Fixes #11899