This repository was archived by the owner on Aug 23, 2026. It is now read-only.
Repository navigation
refactor(quality): Rector wave 6 — null-coalesce default gate, phantom TTL fallback removed (AID-542) - #193
Merged
Conversation
…m TTL fallback removed (AID-542)
Resolve the ApplyDefaultInsteadOfNullCoalesceRector split (AID-531) the
opposite way from mechanical application: fix the two flagged sites by
hand and keep the rule armed as the gate's guard.
The ticket assumed the classic ??→?: trap; the rule actually rewrites
`helper('key') ?? $default` into `helper('key', $default)`. That form
still has a non-equivalent corner — a key present with an explicit null
returns $default under ??, null under the parameter form — but both
flagged sites read `config('unblock.hq.ttl') ?? 7200`, where that
corner is unreachable by construction: config/unblock.php defines the
key as `(int) env('HQ_WHITELIST_TTL', 14000)`, so it always exists as
an int. Which exposed the real defect: the ?? 7200 was dead code AND a
phantom default — the code (and its "Default 2 hours" comment) claimed
7200 while the actual default is 14000, owned by the config file. Both
fallbacks are removed; the default now has a single owner.
The suite caught what the reachability analysis alone could not: one
test pinned the phantom fallback by fabricating the impossible state
(`config()->set('unblock.hq.ttl', null)` at runtime, then asserting
7200). "Not configured" really means the env var is absent, in which
case config resolves 14000 at load time — null never occurs. The test
was the fallback's only consumer, and it consumed a state that cannot
exist; it now pins the real contract (config-owned 14000 flowing
through to the mailable) with an explicit guard assertion.
The rule stays enabled: zero-diff after the hand fix, proven to bite
(a new `config('x') ?? $default` fails refactor:dry naming the file),
guarding against future phantom or duplicated defaults. If a genuinely
present-but-null key ever appears, skip that call site — not the rule.
Gates: check-full green, 1442 passed / 16 skipped (3585 assertions),
coverage 89.8%, refactor:dry (cleared cache) exit 0.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Resolves the
ApplyDefaultInsteadOfNullCoalesceRectorsplit from AID-531 — the opposite way from mechanical application: the two flagged sites are fixed by hand, and the rule stays armed as the gate's guard.What the rule actually does (ticket premise corrected)
Not the classic
??→?:trap: it rewriteshelper('key') ?? $defaultintohelper('key', $default). That form still has a non-equivalent corner — a key present with explicit null returns$defaultunder??, butnullunder the parameter form.What the review found at the two sites
Both read
config('unblock.hq.ttl') ?? 7200. The corner is unreachable by construction:config/unblock.phpdefines the key as(int) env('HQ_WHITELIST_TTL', 14000)— always present, always int. Which exposed the real defect:?? 7200was dead code, and// Default 2 hourscomment) claimed 7200 while the real default is 14000, owned by the config file.Both fallbacks removed → the default has a single owner. Sanity-checked:
config('unblock.hq.ttl')= 14000 (int).The suite caught what reachability analysis alone could not
One test pinned the phantom fallback by fabricating the impossible state:
config()->set('unblock.hq.ttl', null)at runtime, then asserting 7200. "Not configured" really means the env var is absent — in which case config resolves 14000 at load time; null never occurs. The test was the fallback's only consumer, and it consumed a state that cannot exist (the recorded lesson: a test that constructs the state directly is testing its own setup). It now pins the real contract: config-owned 14000 flowing through to the mailable, with an explicitexpect(config(...))->toBe(14000)guard.The rule stays enabled as a guard
refactor:dryexit 0, cleared cache)config('x') ?? $default→rc=2namingapp/ProbeCoalesce.phpconfig('x') ?? $dis always either dead or a duplicated default; if a genuinely present-but-null key ever appears, skip that call site — not the ruleGates
composer check-full→ exit 0, 1442 passed / 16 skipped (3585 assertions), coverage 89.8%.