Empty settings don't return default values

[imorland]

Bug Report

This has been an annoyance forever, as an extention developer.

For example, we have a setting, a string let's say. We can provide a default value for that, great. However, if that setting is saved as simply an empty string '', I would expect the provided default to be returned. That's not currently the case.

At present, the logic for deciding if the default should be returned is based on is_null, which only works if that setting key does not exist on the table.

Expected Behavior
In the case where a setting key exists, but it has no value, the default (if provided) should be returned.

Environment

• Flarum version: any
• Website URL: n/a
• Webserver: any
• Hosting environment: any
• PHP version: any
• Browser: any

Possible Solution
I initially thought using empty(), but as @SychO9 pointed out, a string value of "0" would return true

[askvortsov1]

This is expected behavior, and has been broadened to include null for v1.2. The presence of any value in the settings table, whether that be null, empty string, "0", or anything else, indicates that a value has been set, and therefore, a default should not be used. This is made possible by #3056, as, on the level of defaults, we can now differentiate between null-value defaults and those that are not present, since defaults are no longer implemented via data migrations.

[davwheat]

One thing I fear as part of our deprecation of the add settings migration helper is that old migrations could break. Did we decide whether we will fully remove that with 2.0, or are we keeping it as a legacy compatibility feature?

[askvortsov1]

One thing I fear as part of our deprecation of the add settings migration helper is that old migrations could break. Did we decide whether we will fully remove that with 2.0, or are we keeping it as a legacy compatibility feature?

Old migrations can safely be removed and replaced with the relevant extenders, since they affect content not structure.

[imorland]

Take the following example

(new Extend\Settings())
        ->serializeToForum('someAttr', 'someKey', null, 'myDefault')

Would now have to become

    (new Extend\Settings())
        ->default('someKey', 'myDefault')
        ->serializeToForum('someAttr', 'someKey', null)

That feels the long way around? And would still exhibit the same results as the original problem?

[imorland]

Let's take this setting from byobu as our example case.

Now, we provide defaults, both in the frontend using the placeholder

{this.buildSettingComponent({
                type: 'string',
                setting: 'fof-byobu.icon-badge',
                label: app.translator.trans('fof-byobu.admin.settings.badge-icon'),
                help: (
                  <div>
                    <Badge icon={this.setting('fof-byobu.icon-badge').toJSON() || this.badgeDefault}></Badge>
                    {' '}
                    {helpText}
                  </div>
                ),
                placeholder: this.badgeDefault,
              })}

and in the extend.php

(new Extend\Settings())
        ->serializeToForum('byobu.icon-badge', 'fof-byobu.icon-badge', null, 'fas fa-map')

Now, that's fine. Where the problem comes into play is an admin specifies a different value in the admin panel, and saves it. At a later time, then decides to revert back to our default by removing their replacement value, leaving the text input blank.

This is when the issue I describe above hits. A workaround is to do something like

(new Extend\Settings())
        ->serializeToForum('byobu.icon-badge', 'fof-byobu.icon-badge', function ($value): string {
            return empty($value) ? 'fas fa-map' : $value;
        })

but it feels like this is only ever a workaround, therefore I'd like to see something a little more concrete in core for this problem

[davwheat]

This is expected behavior, and has been broadened to include null for v1.2.

Is this not a breaking change? We shouldn't start arbitrarily changing behaviour, especially something as critical and commonly-used as settings, which could break whole forums with unexpected behaviour changes in minor version bumps.

[SychO9]

There is no breaking change, if a setting is null on the database, you get null and not the default, which is the same behaviour as before.

this still holds basically:

At present, the logic for deciding if the default should be returned is based on is_null, which only works if that setting key does not exist on the table.

Which is the desired (and the non changed) behavior, any value set, is a value, and therefore should be returned instead of the default.

---

The way I think we should deal with the issue of unsetting settings so that the default be used again. Is for the developer to be able to determine which values lead to deleting the setting on save. Something like:

return [

    (new Extend\Settings)
        ->default('my_setting_key', 'flarum')
        ->deleteWhen('my_setting_key', function (mixed $value): bool {
            return $value === '';
        }),

]

[SychO9]

Problem

As an extension developer, I can register a default setting value. Sometimes I wish for the default value to be returned for certain values set by the admin.

Example

If you have a setting that contains a sentence, and the default is lorem ipsum. You may want the default to be returned when the value of the setting is either null or just an empty string ``.

Current behavior

The current behavior is that for any setting, the default value is only used when the setting key does not exist on the settings table (i.e the admin has never changed the value).

What needs to be done

An extension developer needs to be able to define when a setting value saved by the admin should lead to the setting being deleted instead (so that the default is used).

Something along the lines of (example):

return [

    (new Extend\Settings)
        ->default('my_setting_key', 'flarum')
        ->forget('my_setting_key', function (mixed $value): bool {
            return $value === '';
        }),

]

Relevant classes would be the Settings extender and the SetSettingsController.

[askvortsov1]

I wonder if it might make sense to instead augment the save settings endpoint to take a list of settings to clear, and make this a client side distinction? Feels a bit fragile (and work intensive) to have to hardcore for each setting which values constitute "clear", especially since there might be a world where "null" and "empty string" are valid values of a setting.

[SychO9]

Feels a bit fragile (and work intensive) to have to hardcore for each setting which values constitute "clear", especially since there might be a world where "null" and "empty string" are valid values of a setting.

But you would still have to make that distinction even if you move it to the frontend side? You would still need some-kind of callback for each setting to determine if it should be cleared or not. And in that case, i'm not entirely convinced the frontend side is better. Apart from the fact I do like the API allowing explicitly clearing settings.

[SychO9]

I think I also look at it from a data integrity standpoint. It's a form of sanitazion/validation of the data received. Should the settings be saved from a different client other than flarum's frontend. You would end up with unexpected setting values 🤔

We could also instead of a callback to forget. Define a callback to sanitize (reverse, same result)

[askvortsov1]

Agree with having a callback to sanitize, that's more general. Although I wonder if it's redundant combined with validation?

Another benefit of having an operation to unset settings is that it could be made explicit. So some settings could unset from the frontend if left blank, and others could have an explicit "x" button to clear them.

[SychO9]

Although I wonder if it's redundant combined with validation?

validation usually prevents you from saving invalid data, while sanitization cleans up data before saving, so if we added setting validation (which sounds like a good idea) it shouldn't conflicts with sanitization.

Another benefit of having an operation to unset settings is that it could be made explicit. So some settings could unset from the frontend if left blank, and others could have an explicit "x" button to clear them.

Yes, would definitely love have a clear settings controller, I've actually recently had a need for it and was surprised you can't clear setting values from the frontend unless you manually create the controller yourself.