Attach the deprecation version to the deprecating call - #277
Conversation
`export_uses()` wrote `deprecation_version` to `$out['functions'][0]`, the first recorded function use, instead of the `_deprecated_*()` call that carries the version. That only looked correct when the deprecating call happened to be the first call in the scope, as it is in a deprecated file; whenever another call precedes it, the version was attached to that unrelated call and the deprecating call got none. Build the use record first and attach the version to it before appending. Found during the review of #262 and extracted here as a standalone change.
sirreal
left a comment
There was a problem hiding this comment.
Note
This is an agentic review, generated by Claude Code at the repository owner's request.
Ready to land.
Verified
-
CI green on 7.4 and 8.4; mergeable, clean, based on
master. -
The bug is real. On
master,export_uses()appends the use record and then writes$out[ $type ][0]['deprecation_version'] = $version;— an unconditional index-0 write, unrelated to which record triggered it. Any function whose_deprecated_*call is not the first recorded call exports the version on the wrong entry, and with two deprecating calls in one body the last one silently overwrites the first, still on index 0. -
The fix is minimal and behavior-preserving in the common case. Building
$used, attaching the version to it, and appending once produces byte-identical output when the deprecating call is first: same keys, same order (name,line,end_line,deprecation_version). The$version = nullcase still sets the key, as before. No positional indexing left. PHP 7.4-clean. -
The tests are genuinely RED on master, not tautologies. I checked the assertion helper:
Export_UnitTestCase::entity_uses()matches onlineand then does a fullassertEquals( $used, $exported_used )— not a subset check. So both new tests fail onmaster:test_deprecating_call_has_version— the_deprecated_functionrecord (line 5) has nodeprecation_versionon master.test_preceding_call_has_no_version— thedo_something_firstrecord (line 4) carries an extradeprecation_versionkey on master, so the exact-equality assertion fails.
The second one is the important one; it is what pins "the version does not leak onto an unrelated record", and it only works because the helper is exact. Worth remembering that
entity_uses()is exact, sinceassertEntityContains()right above it is a subset check — easy to mix up when writing the next test. -
Fixture line numbers match the
.inc(call on line 4, deprecation on line 5).
Worth noting
- This changes exported JSON for every function in the corpus whose deprecating call is not the first recorded call. That is the point, but once #284 lands it would be worth re-running this branch through the corpus-diff job and pasting the hunk count, so the intended change is enumerated rather than asserted.
- #281 fixes the importer half of the same story (finding the
_deprecated_filerecord anywhere in the uses list). #281 is non-regressing before this lands and fully correct after — I checked that claim against both diffs and it holds. Either order works; this one first makes #281 immediately effective.
export_uses()writesdeprecation_versionto$out[ $type ][0]— the first recorded function use — instead of the_deprecated_function()record it belongs to. Any function whose deprecated-call is not the first call in the body exports the version on the wrong record:exports
uses.functions[0] = { name: 'do_something_first', …, deprecation_version: '6.1.0' }while the_deprecated_functionrecord gets none. It only looks correct in existing fixtures because the deprecated-call happens to come first.Build the record locally, attach the version to it, and append it once — no positional indexing. The test was added first and fails on master with the version on the wrong entry.
Found during the review of #262 and extracted here as a standalone change.
🤖 Generated with Claude Code