Inconsistent Setting Savings

[spencerfinnell]

Found some odd behavior while working on #6720

Bug Report

Expected behavior

When I set 0 to "Refund Window" it should save as 0 for unlimited.

Actual behavior

It saves as 30 (previous value).

Steps to reproduce the behavior

1. Visit Settings > General > Refunds
2. Enter 0 in Refund Window
3. Click save.

---

Expected behavior

When I set 0 to "Download File Limit" it should save as 0 for unlimited.

Actual behavior

It saves as blank.

Steps to reproduce the behavior

1. Visit Settings > Misc > File Downloads
2. Enter 0 in File Download Limit
3. Click save.

---

Master saves File Download Limit as blank when inserting a 0 as well. A couple instances to get the download limit global setting use edd_get_option( 'file_download_limit', 0 ) but most don't set a default. Not sure the expected behavior.

[spencerfinnell]

No number settings allow saving as 0. Something is likely checking for empty vs isset. This is present in master as well.

[spencerfinnell]

Pretty sure this has been broken for 3 years.

https://github.com/easydigitaldownloads/easy-digital-downloads/commit/dc3c10f438aa5391add70282c3a386931a784a71#diff-0fc4cfad82ef0e4a14720904d33c326cR1480

edd_get_option() should be taking the setting field's standard value as the default. The 0 string was being interpreted as false when checking again so the standard value was being used.

$edd_option = edd_get_option( $args['id'] );

if ( $edd_option ) {
	$value = $edd_option;
} else {
	$value = isset( $args['std'] ) ? $args['std'] : '';
}

Should be

$value = edd_get_option( $args['id'], $args['std'] );

[spencerfinnell]

I know it's a very risky thing to update but I believe the fixes in https://github.com/easydigitaldownloads/easy-digital-downloads/commit/c57a3fad0d1c8ba2e0b223af5272343d58ce4ba6 will help.

• When outputting the value of fields registered with the settings API avoid unneeded logic for determining the default value and instead use the $default argument in edd_get_option( $key, $default );
• When looking for a setting in edd_get_option() check if the key is set at all, and use that value if it is. Otherwise use the default. Previously checking for empty() would fail on things like blank strings for 0 strings.
• Allow empty setting values to be saved in settings. There is an option for allow_blank that is not used in any setting registrations and I am not sure of the original intention.
• edd_get_option() can be used like it is because https://github.com/easydigitaldownloads/easy-digital-downloads/blob/master/includes/admin/settings/register-settings.php#L191-L211 ensures all settings will contain a std value.

Didn't open a PR in case anyone wants to add on to it or discuss why the current behavior is the way it is.

[pippinsplugins]

The original intention of allow_blank was to permit a setting to be saved as an empty field, or, in another way, to enforce a field to always have a value by setting 'allow_blank' => false.

See https://github.com/easydigitaldownloads/easy-digital-downloads/issues/2808 and https://github.com/easydigitaldownloads/easy-digital-downloads/issues/4854

[spencerfinnell]

I didn’t see any settings registering that when I checked but I’ll look at Rich editors again.

[cklosowski]

Core doesn't have any that end up using it, but there are some specific cases where extensions needed to have this behavior exist. Free Downloads and Content Restriction are two that use it, in order to allow some of their rich editors to require some sort of messaging and fall back to the defaults.

[spencerfinnell]

The standard (default) value is set as an empty string though which allows it to be blank I think? I’ll test with the extensions and see what I find.

[spencerfinnell]

What's confusing to me is allow_blank is only affecting the output of these settings once they have already been saved as blank. So a blank setting is still being saved as far as I can tell? It would require saving the current settings section again in order to override the previously blanked value.

I think the check needs to be moved to the main sanitize function.

[spencerfinnell]

https://github.com/easydigitaldownloads/easy-digital-downloads/commit/ee383d30607ee74a1da8f7ef25f73b2a7a51adb3 Makes sure a blank input is never stored if the setting does not allow it.

[pippinsplugins]

Changes look sane but let's please get some good testing on it.

[spencerfinnell]

👍 Attached a PR for easier testing. Definitely realize it's a daunting one to introduce and since I think these problems have existed for a little while now there's no need to rush it in.

[SeanTOSCD]

I'm willing to give this some good testing but I'm curious what you all think is the best way to do it. I'll test all necessary scenarios, from fresh sites to updated sites to extension behavior.

If someone can help lay out a thorough testing scenario, we can get some solid rounds of testing on this. 🙏

[spencerfinnell]

@SDavisMedia Basically each field type needs to be saved/updated/edited and ensure the expected behavior is in both places:

1. The database.
2. The output of the value in the field.

One of the main issues previously was the field output did not reflect what was actually being saved in the database.

You can see a list of the field type callbacks that were changed in https://github.com/easydigitaldownloads/easy-digital-downloads/pull/6898/files

• [ ] Checkbox
• [ ] Multicheck
• [ ] Payment Icons
• [ ] Radio
• [ ] Gateways
• [ ] Select
• [ ] Text
• [ ] Rich Text
• [ ] Email
• [ ] Number
• [ ] Textarea
• [ ] Password
• [ ] Color
• [ ] Upload
• [ ] Color Select (button style I think?)
• [ ] Shop States
• [ ] License Key

[dgoldak]

@spencerfinnell when I checkout this issue EDD reverts back to 2.9. I assume we should test the fix against 3.0?

[cklosowski]

@dgoldak currently this issue is forked from master, not the release/3.0 branch. Since the settings saving isn't changing between the two versions, a failure would happen in both branches.

[pippinsplugins]

I would like to punt this out of 3.0. Adjusting the saving routine for settings is always really risky and, since this has been this way for more than 3 years, it's clearly not causing any major issues.

Let's address it after the 3.0 dust settles.