-
-
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
Add Settings Extender #2452
Add Settings Extender #2452
Conversation
859d192
to
400b102
Compare
12b4eeb
to
13d6708
Compare
13d6708
to
d179ce3
Compare
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.
A few more nitpicks, but other than that, I really like this one!
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.
So I've been thinking more about this, and I'm no longer as sure as I was before. Sorry for the confusion and back-and-forth. The thing is, we already have a default settings mechanism: adding those values to the DB via a migration, which feels a bit faster. This new mechanism is more convenient, and makes sense, but I'd like to have a little bit more discussion, if only for the sake of devil's advocacy, before we implement it. How would you feel about splitting the default settings portion of this out into another PR (so this one only adds serialization), and revisiting the default part for beta 16? Sorry again.
🤦 I completely forgot about migrations. Also while updating all bundled extensions I didn't use the
I don't even think we need to consider it at all unless we really feel the need to do something like this in the future. The question is, are we fine with having the whole settings extender class with just the
Never be sorry when it comes to what's best for the project :D, I can dance. |
Yeah I think that's good, seeing as we really only want to serialize them to one serializer. If different use cases emerge, we can always reconsider later, but for now I think this is a good solution. |
Part of #1891
Changes proposed in this pull request:
Add an extender to expose settings to the frontend through the ForumSerializer.
Reviewers should focus on:
Naming:
Confirmed
composer test
).