-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Settings API: Separate default settings from fixtures #567
Comments
Can't fault any of this. It seems like the best way to me |
I want to get a very basic version of this setup ASAP:
Part 2 would then be:
|
Assign this one to me :) |
GOOD NEWS.I'm apparently not great at predicting time, because I've already done (a rough version of) the thing I said I might not finish by tomorrow. https://github.com/skattyadz/Ghost/compare/default-settings Tomorrow I'll test the sucker, and I might give it a structure more like:
EDIT: Although it's way cleaner, that structure doesn't give a lot of room for validations. Maybe have an object for each key containing the default value and applicable validations I can't work out whether "when a specific setting is requested, if it isn't found, we check to make sure it isn't specified in the JSON, if it is, we return the default value and load that into the DB" is actually needed if we're doing this on startup anyway? |
👍 This is excellent news 🎈 🍩 One change: populateDefaults should be the first thing that happens in init as everything else depends on settings.
That sounds sensible. With the system populate in place, modifying the structure shouldn't be a deal breaker though, we can always change it in future.
I was thinking that people might futz with their data and then anything which depends on a setting being present will break. It's definitely an enhancement for later though as there isn't even a delete method on the settings api so they would have to do some serious futzing! If you can test this with the one minor change of moving population up a bit further & get a PR in for just this bit that would be seriously awesome cos it means I can build in the new settings we need. Then we can start to look at validations next 😁 |
I've separated out the bit about loading an individual setting, might not need that, we'll see. Otherwise.. all done 👍 |
This is related to #482 (migrations being a PITA) and #360 (BIG FAT refactor).
Settings are kind of a special thing. They are not at all like the other models in the database - they have a clear set of pre-propulated rows (key, value pairs) and their presence is something the app needs to be able to rely upon.
Rather than using iterations of fixtures to update and add settings, we should have a clear place/way of defining what the required/default settings are, what their default values are, what their validation rules are, and whether or not they can be overwritten during an import (currentVersion should not be for example).
Whenever a setting is requested, if it doesn't exist in the cache/database, lets check the settings definition & add it if it is present.
The validation rules can be used in the validate() method on the settings model to check that a given value is permitted for specific keys.
Perhaps if this was shared, it would also be possible to use the validation rules to validate in the client.
Once this is done, settings can safely be removed from the fixtures.
The text was updated successfully, but these errors were encountered: