Details
-
Type:
Improvement
-
Status:
Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 1.7
-
Fix Version/s: 1.7
-
Component/s: Administration
-
Labels:None
-
Affected Branches:MOODLE_17_STABLE
-
Fixed Branches:MOODLE_17_STABLE
Description
All settings are stored in database in serialized form. Default config value should be serialized too. But when submitting the form with complex settings are data are in different form - also arrays. I think I found several problems in your implementation (and fix some of them), but the code is not easy to read and understand.
Proposal:
1/ add method serialize/deserialize to class admin_setting - with deafult implemenation just returning data as text
2/ always store defaults in serialized form (only strings - the same as in database)
2/ make get_setting() to return deserialized form (arrays, strings, etc)
3/ decide if write_setting() should accept only deserialized, serialized, or both forms
4/ fix all uses of write_setting() accorning to 3/
The resulting code should be easier to read, most bugs should be fixed during the conversion, it would be easier to add new complex types.
Issue Links
| This issue will help resolve: | ||||
| MDL-6577 | Upgrade settings broken in admin interface |
|
|
|
I don't think defaults should be in serialized form, but I do agree they need to be more consistent.
Tell me your thoughts on this:
1/ write_setting always takes a deserialized form (i.e. whatever is sent in $_POST, which is sometimes a plain string for admin_setting_configtext, and other times an array for admin_setting_configmultiselect)
2/ defaultsetting is stored in the deserialized form so that write_setting(defaultsetting) will always work
3/ get_setting is changed so that it returns data in a deserialized form, that way write_setting(get_setting()) produces a valid result, and also so that we can do $currentsetting = (get_setting() ? get_setting() : defaultsetting) to get a value to use in output_html (
MDL-6577)Let me know if this sounds alright. If it does, I'll go ahead an implement these changes and the fix for
MDL-6577.MDL-6577) Let me know if this sounds alright. If it does, I'll go ahead an implement these changes and the fix forMDL-6577.