Moodle
  1. Moodle
  2. MDL-6440

Add serialize/deserialize to admin_settings class

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Vincenzo K. Marcovecchio added a comment -

            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.

            Show
            Vincenzo K. Marcovecchio added a comment - 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 .
            Hide
            Petr Skoda added a comment -

            Thanks, it sounds reasonable, please go ahead.

            Show
            Petr Skoda added a comment - Thanks, it sounds reasonable, please go ahead.
            Hide
            Vincenzo K. Marcovecchio added a comment -

            See latest updates to lib/adminlib.php

            Changes I recommended for consistency have been made.

            Show
            Vincenzo K. Marcovecchio added a comment - See latest updates to lib/adminlib.php Changes I recommended for consistency have been made.
            Hide
            Michael Blake added a comment -

            Vince, Please complete "Fix Version' for this bug. Thanks.

            Show
            Michael Blake added a comment - Vince, Please complete "Fix Version' for this bug. Thanks.

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: