# FirebaseNotificationQuery Decomposition — Follow-Up Work Produced by the `refactor/firebasenotificationquery-decomposition` branch (2026-08-26). That branch was **behaviour-neutral by design**: 24 of 28 method bodies are token-for-token identical to the originals, and the other 4 differ by exactly one token pair each (`(int)` casts). Everything below was found while refactoring and deliberately left alone. The final whole-branch review found **zero Critical and zero Important defects**. Nothing here blocks anything; one item needs a product decision rather than a code change. --- ## Needs a decision, not a fix ### F1. `OnlyDownloadMorning` runs daily and silently does nothing. `OnlyDownloadCampaignService.php:35` — `runOnlyDownloadMorning()` has an **entirely commented-out body**. But `FirebaseCampaignRegistry.php:65` still registers `OnlyDownloadMorning` as a live key, and it is in `FirebaseNotificationController`'s allow-list. So the 03:55 UTC cron fires, does nothing, logs *"Executed OnlyDownloadMorning successfully"*, and exits 0. Anyone reading the logs sees a healthy campaign. Pre-existing and untouched by this work. The fix is a product call: **restore the body, or deregister the key.** It should not be resolved by an engineer guessing which was intended. --- ## Code follow-ups, in the order they're worth doing ### F2. Delete three now-unreachable `is_numeric()` guards, and the four redundant casts, together. `DayUnlockNotificationQuery.php:36`, `FinalScriptNotificationQuery.php:172`, `ReEngagementNotificationQuery.php:40` and `:97`. Task 12 gave these methods native `int` parameters. Combined with `declare(strict_types=1)`, a non-int argument now raises a `TypeError` *before the body runs*, so `is_numeric($programId)` is unconditionally true and the `InvalidArgumentException` branch is dead. They're currently suppressed by PHPStan baseline entries. The same commit should drop the four `(int) $programId` casts at the `ActiveDayService::getDefaultActiveDay()` call sites (`ReEngagementNotificationQuery.php:44, :101, :169`, `DayUnlockNotificationQuery.php:40`), which the native `int` parameters made redundant. Both were deferred deliberately: deleting them is a *body* edit, and this branch's entire reviewability rested on "the only body deltas are four casts." Doing them together, after the branch lands, is one small clean commit. **One semantic delta to be aware of while doing it:** the guard used to throw `InvalidArgumentException` (an `\Exception`); the native type now throws `TypeError` (an `\Error`). `FirebaseNotificationController.php:74` catches `Exception`, not `Throwable`, so such a failure would escape as a framework 500 rather than the JSON error body. Practically unreachable — the only values passed are a program PK and an `int`-typed closure parameter — but if you touch this area, catching `Throwable` there is the safer shape. ### F3. Rename two stale properties. `DayUnlockCampaignService.php:32` and `UnlockUserLTPProcessor.php:22` still call the injected property `$firebaseNotificationQuery`. The other seven consumers use `$users`, and the class that name refers to no longer exists. Harmless at runtime; misleading to anyone grepping for it. ### F4. Three query bodies are byte-identical and now live in two files. `FinalScriptNotificationQuery.php:56`, `:84` and `MilestoneNotificationQuery.php:91` generate identical SQL — confirmed by a fixture hash collision. The duplication is pre-existing and was correctly preserved verbatim, but the split moved it across files, so a future fix to one will silently miss the other two. Worth consolidating deliberately, or at least cross-referencing in the docblocks. --- ## Strengthening the golden master `tests/Unit/Http/Queries/Firebase/FirebaseNotificationQuerySqlSnapshotTest.php` is a sound oracle — deterministic, discriminating (every multi-case method produces *n* distinct SQL of *n* cases), and it can fail. Four narrow blind spots, in rough value order: 1. **Nothing asserts `$firebaseSqlOwners` covers every public method on the nine classes**, so a newly added query method gets no protection automatically. A test that reflects over the nine classes and fails on an unmapped public method would close this permanently. 2. **The three identical-SQL methods in F4 are indistinguishable to the oracle.** Covered here by token-level body comparison instead, but the fixture alone cannot tell them apart. 3. **The test asserts fixture ⊇ dataset, never dataset ⊇ fixture** — deleting a case would silently orphan its fixture entry rather than failing. 4. **`DB::pretend()` plus a swallowed `Throwable` means a method that builds correct SQL and then throws still passes.** Inherent to the technique, and the file's docblock discloses it. --- ## A correction to the earlier analysis I reported that this code has **no HTTP surface at all**. That was wrong. `routes/api.php:159` exposes: ``` GET /api/test/Firebase/{commandType} → FirebaseNotificationController::runCommand ``` It does not change any conclusion — the route sits behind `secure.cron`, returns 403 in production, allow-lists the 36 registry names, and passes only a command name to `Artisan::call`, so no request-derived value reaches any query parameter. But "cron-only" was inaccurate, and the accurate statement is "cron-driven, with one guarded diagnostic endpoint." --- ## One claim to state accurately The branch's proof is **"no behaviour change that can affect results"**, not "provably zero behaviour change". One binding type shifted: with `int $programId`, `->where('a.iProgramID', $programId)` now binds `PDO::PARAM_INT` where PDO may previously have supplied `"1"`. Benign against MySQL `INT` columns and invisible to the fixture, which always passed an int — recorded so nobody over-claims.