[2.x] fix: allow boolean settings with truthy defaults to be disabled - #4782
Merged
imorland merged 2 commits intoAug 5, 2026
Merged
Conversation
9 tasks
Member
|
Thanks for the PR. Please see my comment on the issue you created: #4781 (comment) |
Pins what `setting(key, fallback)` seeds its stream with: a saved value is used, an absent one falls back to the default, and a saved empty string is kept rather than mistaken for absent. That last case is the bug — settings live in a string column, so a `false` comes back as `''`, and choosing the fallback on falsiness meant a setting whose default is truthy reverted to on with every reload. Also pins `'0'`, which was never affected: the string is truthy in JavaScript, unlike the number. Worth stating so the distinction survives.
Member
|
Thanks for tracking this down properly — the diagnosis is right, and the point about I've pushed tests onto your branch to go with the fix. They pin all four cases — saved value, absent value, saved empty string (the bug), and One note on that last one: |
imorland
approved these changes
Aug 5, 2026
imorland
added a commit
that referenced
this pull request
Aug 5, 2026
#4782 fixed this in `AdminPage` but the identical line in `SettingsModal` survived, so the bug still reached every settings *modal*: settings live in a string column, a saved `false` arrives as an empty string, and choosing the fallback on falsiness discarded it and reseeded the default. A boolean setting whose default is truthy could not be switched off — it came back on with every reload. `SettingsModal` is public API, imported by extensions as `flarum/admin/components/SettingsModal`, and core's custom header, footer and CSS modals extend it, as does flarum/audit. The existing test file now covers both classes side by side rather than only the one the original issue named, which is what would have caught this the first time. Its docblock is corrected too: it claimed a saved `'0'` was also discarded, but the string is truthy in JavaScript, unlike the number — the empty string is the only value a setting can hold that JavaScript reads as absent.
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
2 participants
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.
Fixes #4781
Changes proposed in this pull request:
Resolves a bug in
AdminPage.tsxwhere boolean settings (toggles) that have a truthy default value could not be turned off by the administrator.By switching from the logical OR operator (
||) to the nullish coalescing operator (??), the setting stream correctly recognizes""as a valid saved state. The fallback is now only applied when the setting is strictlyundefinedornull(e.g., during a fresh installation where the setting hasn't been saved to the database yet).Reviewers should focus on:
Screenshot
PixPin_2026-06-26_18-10-57.mp4
Necessity
Confirmed
composer test).Required changes: