Skip to content
Snippets Groups Projects
Closed Settings - Deprecate "serialize" flag
  • View options
  • Settings - Deprecate "serialize" flag

  • View options
  • Closed Issue created by totten

    Background: Basic settings

    Settings are "key-value" pairs. The "value" part allows type mixed (ie string or int or float or array or whatever).

    Support for mixed is consistent between Civi::settings() and $civicrm_setting.

    // Consistent access to a scalar
    Civi::setting()->set('my_string', 'abc');
    $civicrm_setting['domain']['my_string'] = 'abc';
    assert Civi::setting()->get('my_string') === 'abc';
    // Consistent access to an array
    Civi::setting()->set('my_array', [1,2,3]);
    $civicrm_setting['domain']['my_array'] = [1,2,3];
    assert Civi::setting()->get('my_string') === [1,2,3];

    Under the hood, values are stored in MySQL using serialize(). You can see this in both recent and ancient code:

    Background: serialize metadata

    There is an additional option in *.setting.php metadata called serialize. Superficially, this resembles serialize metadata for DAO fields.

    This option is defined for 8 settings: contact_view_options, contact_edit_options, advanced_search_options, user_dashboard_options, address_options, contact_autocomplete_options, contact_reference_options, and quicksearch_options.

    In all cases, the setting has a serialization format of SERIALIZE_SEPARATOR_BOOKEND.

    APIv4's Setting is a wrapper for Civi::settings(). Notably, it interprets the serialize metadata and applies an (additional/secondary) serialization.

    What's good about this?

    It makes it easier for REST/AJAX agents to read and write those 8 historical settings (the pre-existing settings with the annoying BOOKEND format).

    At the time of introduction, it probably had no effects on compatibility and required no upgrade steps.

    What's bad about this?

    It encourages people to add unnecessary serialization layers. If you are writing a new setting with structured data, then you'll see the serialize metadata and think: "I should add that!" Here are some docs that suggest JSON for a setting:

    https://docs.civicrm.org/dev/en/latest/framework/setting/#settings-definition

    But you shouldn't do that on a new setting! Here's why:

    • It adds complexity -- double-encoding. Every read and write of the data goes through both serialize() and some additional serialization method.
    • It adds complexity -- conflicted APIs. The setting can be viewed through various programmatic interfaces (e.g. local PHP array $civicrm_setting; local PHP method Civi::setting()->get(); e.g. remote REST method Setting.get). Some APIs present an array -- but others present an encoded string (like ^1^2^3^). There is no decent reason why $civicrm_setting, Civi::setting(), and Setting.get should present it differently.
    • It distracts from the real data-modeling problem of serialized fields. We generally do simple settings (strings/ints) because they're easier. Complex settings (like mailing_backend) are possible, but we don't do them as much - because the tooling/validation/etc is not as good. Intuitively, complex settings could work better if we added more metadata. But this is the wrong metadata. Switching mailing_backend between serialize/JSON/CSV/YAML/etc doesn't make this any better. What you need is a sharper description of the data inside of that serialized blob.
    • It becomes overall harder to change. Perhaps settings should be stored with json_encode() instead of serialize()? serialize() has additional kinds of vulnerabilities. JSON has better support in MySQL. Plot out the path for how to migrate the entirety of civicrm_setting.value -- and compare that to switching serialization on a setting-by-setting basis. It's easier to change if individual settings are not committed to specific serializers.

    What to do

    Simplest thing:

    • Keep the mechanism
    • Present it as deprecated
    Edited by totten

    Linked items ... 0

  • Activity

    • All activity
    • Comments only
    • History only
    • Newest first
    • Oldest first
    Loading Loading Loading Loading Loading Loading Loading Loading Loading Loading