-
-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow resetting settings to default #3935
feat: allow resetting settings to default #3935
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close enough but not quite ;)
The problem we are trying to solve is that, when the admin saves a value (empty string for example) that will be used as the setting value, even if an extension developer adds a setting that must be filled.
So the idea is that the callback will be used in the SetSettingsController
class to determine which values to save and which to forget, because when an admin saves settings from the admin UI that's the controller that's used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested yet, But this looks good!
Now all we have to do (as per the conversation with Sasha on the issue) is to inverse the intent (and the name) of the forget
extender. So instead of saying forget this setting when the value equals this. It would become store this setting when the value is this.
For naming, maybe filter
would make sense, what do you think?
Sounds good to me! |
…lues # Conflicts: # framework/core/src/Extend/Settings.php # framework/core/tests/integration/extenders/SettingsTest.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry for the delay.
Documentation added: flarum/docs#478
Fixes #3934
Changes proposed in this pull request:
Reviewers should focus on:
Screenshot
QA
Necessity
Confirmed
composer test
).Required changes: