-
-
Notifications
You must be signed in to change notification settings - Fork 835
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: Default Settings Repository #3127
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.
Radical idea, but alongside this, I think we should consider deprecating the default
arguments to get settings. Otherwise, those arguments take on a weird hybrid role between truly being a setting default, and representing "if setting is null, do X". The latter isn't nonsensical, but it can also be done explicitly.
On the other hand, maybe there could be theoretical use cases for overriding the default on individual call sites, although I can't really think of any. But keeping defaults to one place would definitely simplify things.
This reverts commit 6aea45f
Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
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.
Looking over this again, there's still one thing I'm unsure about.
There's a difference between a setting not being set (in which case default
should be used), and a setting having the value of null
(in which case null
should be returned). Ideally, our system should be able to distinguish between the two, but because of the current implementation with nullable defaults, the difference between the two is encapsulated within get
and not available to the caller. In this PR, you pass the configured default value through the repo layers, which solves the problem, but depends on the default
argument that we are hoping to remove.
For v2, when we do remove the argument, we could throw an exception in DatabaseSettingsRepository
if the setting is not found. This doesn't depend on the default arg, so it fits well with our plans. It would be a breaking change though, which is why we can't do it now.
Good point! hadn't thought of that, and agree with the plan for 2.0 |
[ci skip] [skip ci]
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.
This is really nice now!
Fixes #3056
Needed for #3011
Changes proposed in this pull request:
default
settings extender to register setting default values.addSettings
migration helper.Reviewers should focus on:
I don't think we're missing anything about us switching to this approach ?
Necessity
Confirmed
composer test
).