-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Allow mutators for Settings models and improve API #1080
base: develop
Are you sure you want to change the base?
Conversation
- Use "getAttribute" method to retrieve possibly mutated value before saving into settings array. - Ensure that default values populated by "initSettingsData" are populated into settings array if not yet persisted in the DB - Allow values to be retrieved as model properties at any time.
@@ -174,12 +176,14 @@ public function getSettingsValue($key, $default = null) | |||
/** | |||
* Set a single setting value, if allowed. | |||
*/ | |||
public function setSettingsValue($key, $value) | |||
public function setSettingsValue($key) |
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 change confused for me for a bit before I realized that is method is somewhat performing double duty. The first use-case is the public api of setSettingsValue($key, $value), but the second use-case (the one addressed by this PR and the other PRs) is to sync the model data to the internal fieldValues array whenever setAttributes() is called.
@bennothommo what do you think about changing it so that the model.setAttribute event listener is instead a protected method syncModelAttribute($key)
that then does $this->setSettingsValue($key, $this->model->getAttribute($key))
and leave this method unmodified?
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.
@LukeTowers I think we should come to a conclusion on what we're going to be doing about the actual event. If we either fix up the event or add the third parameter, then I'll have to change this anyway - fixing up the event would mean we don't need this PR at all (beyond maybe the additional afterModelFetch
calls to ensure everything works as properties).
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.
@bennothommo perhaps we can get on a call about this, I'm still not really understanding the problem with the event itself, would probably be easier to talk it through when you have a minute 😄
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.
Hi,
Let me know what and when to test. Thanks for your efforts.
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.
@goldmont is this still an issue for you?
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.
Hi,
I "solved" by using the Encryption trait.
Fixes #1047. Replaces #1071.
getAttribute
method in themodel.setAttribute
listener within theSettingsModel
behavior to retrieve the (possibly) mutated value before saving into settings array.initSettingsData
method in a Settings model are populated into settings array if not yet persisted in the DB, so that they are accessible via model properties.