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
    • Rank:
      34088

      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

          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 Škoda added a comment -

          Thanks, it sounds reasonable, please go ahead.

          Show
          Petr Škoda 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: