fix(schema): correct 7 more Class (b) restart flags — v0.47.0 - #36
Conversation
Audited every remaining "restart": false key after tmdb.api_key was corrected
in 0.46.0, against one criterion: is the value captured into a DI singleton's
constructor, or read once at container/route build time? Seven were, and are
now "restart": true with the requirement disclosed in each helpText.
* lastfm.api_key / .shared_secret / .enabled -- overlaid at route-build time
(once per worker) by Application::applyLastfmOverrides(), then frozen into
LastfmApi's constructor-promoted readonly properties. That docblock ALREADY
asserted these keys carry "restart": true; the schema disagreed with it.
* matching.noise_suffixes -- factory() injected as a constructorParameter into
MediaScanner at two wiring sites. Its comment: "computed once at construction".
* metadata.provider_priority / .genres_mode -- captured into the PriorityConfig
factory, injected as constructorParameter('globalPriority'). Its comment:
"resolved ONCE when first built (per worker cycle, not per request)".
* port-forward.port_forwarding.upnp_enabled -- read at container-build time in
NetworkServicesProvider::register().
The upnp key needed more than a flag flip: the provider computed the value into
a local that was never passed to any definition, and PortForwardService had no
UPnP switch at all, so an override changed nothing even across a restart. The
companion phlix-server change wires it; "restart": true is accurate only with
that change. Flipping the flag alone would have swapped one false promise for
another -- compare hwaccel.probe_timeout, deleted in 0.26.0 rather than
relabelled.
23 keys were verified CORRECT as "restart": false (each resolves through
SettingsRepository at use time) and are untouched. 8 more have no use-time
consumer found at all and are left alone pending a consumer trace rather than
guessed at in either direction.
No key added or removed; property count unchanged at 72 ("restart": true 34->41).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage ∅ diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ ∅ diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0248d7d) 1016 829 81.59% Head commit (f748964) 1016 (+0) 829 (+0) 81.59% (+0.00%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#36) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #36 +/- ##
=========================================
Coverage 81.29% 81.29%
Complexity 414 414
=========================================
Files 50 50
Lines 1160 1160
=========================================
Hits 943 943
Misses 217 217
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Follow-up to #35. That PR corrected
tmdb.api_key; this one closes out the audit it triggered.Criterion
For every remaining
restart: falsekey: is the value captured into a DI singleton's constructor, or read once at container / route build time? If so it cannot apply live, regardless ofSettingsRepository::getEffective()doing an uncachedSELECTon every call. Seven keys qualify.lastfm.api_key,lastfm.shared_secret,lastfm.enabledApplication::applyLastfmOverrides(), then frozen intoLastfmApi's constructor-promoted readonly properties. That docblock already asserted these keys carry"restart": true— the schema simply disagreed with the code.matching.noise_suffixesfactory()injected asconstructorParameter('noiseSuffixes', …)intoMediaScannerat two wiring sites. Its own comment: "the value is computed once at construction."metadata.provider_priority,metadata.genres_modePriorityConfigfactory, injected asconstructorParameter('globalPriority', …). Its own comment: "resolved ONCE when first built (per worker cycle, not per request)."port-forward.port_forwarding.upnp_enabledNetworkServicesProvider::register(). See the caveat below.Each also gains a
helpTextsentence disclosing the requirement, matching howtmdb.api_keyand theprocess.<worker>.enabledkeys disclose theirs.⚠
port-forward.port_forwarding.upnp_enabledneeded more than a flag flipMy first pass classified this as plain Class (b). Re-verifying before changing it showed that was wrong — the key is not merely restart-required, it is inert:
NetworkServicesProvider::register()computes$upnpEnabledat line 73 and the variable appears nowhere else in the file — it is passed to no definition.PortForwardService::__construct()has no UPnP switch at all, andautoConfigure()calls$this->upnp->discoverGateway()unconditionally.So an admin setting it to
falsegot nothing — even across a restart. Flipping the flag alone would have swapped one false promise for another, exactly what this schema did withhwaccel.probe_timeoutbefore it was deleted in v0.26.0.The companion phlix-server PR wires it (
PortForwardService::$upnpEnabledgates UPnP discovery and falls through to NAT-PMP, rather than disabling forwarding wholesale — that is whatautoalready does).restart: trueis accurate only in combination with that change; the two should land together.Note the existing
InertSettingRepairsTestis not wrong about this — it states its own scope honestly: "This proves the value resolves and overlays; it does NOT by itself proveNetworkServicesProviderconsumes that path." It never claimed the effect, and the effect was missing.Left alone, deliberately
restart: false— each resolves throughSettingsRepositoryat use time (per request / per call): theauth.*policy classes,EncodeSettings,ArtworkDownloadPolicy(whose docblock explicitly requiresrestart: false),DlnaAllowlistMiddleware,CastingEnabledMiddleware,TraktOperatorConfig(re-read per request), and others.transcoding.tone_mapping_mode,transcoding.prefer_hdr_output,newsletter.enabled,newsletter.send_hour,relay.reconnect_delay,relay.ping_interval,webhooks.enabled,stats.enabled. Their flags may be wrong in either direction, or they may be inert likeupnp_enabledwas. Left untouched pending a consumer trace rather than guessed at.Scope
No key added or removed — property count unchanged at 72 (
restart: true34 → 41). The diff is exactly 14 lines changed across 7 keys (restart+helpTexteach), verified withgit diff | grep -vc '"restart"|"helpText"'→ 0 unintended lines.Verification
./vendor/bin/phpunitcomposer stancomposer cs🤖 Generated with Claude Code