[BUG] Configuration getOrDefault returns default values for false settings

[simonklink]

We use the domain sync cronjob:
modules/registrars/openprovider/cron/DomainSync.php

And this uses the process_domains method where it loads the configuration settings
https://github.com/openprovider/Openprovider-WHMCS-domains/blob/master/modules/registrars/openprovider/src/DomainSync.php#L149

However this method contains a bug
https://github.com/openprovider/Openprovider-WHMCS-domains/blob/master/modules/registrars/openprovider/src/Configuration.php#L51

public static function getOrDefault($key, $defaultValue = false)
{
    self::init();
    $value = self::$config[$key];
    if (!$value) {
        return $defaultValue;
    }

    return $value;
}

If the setting is false the default value is returned because of these lines:

if (!$value) {
    return $defaultValue;
}

So our configuration contained false for syncExpiryDate
But the default value in the process_domains code is true.

$setting['syncExpiryDate'] = Configuration::getOrDefault('syncExpiryDate', true);

So now all our expirydates are updated which we didn't want.
So the correct code should be something like:

public static function getOrDefault($key, $defaultValue = false)
{
    self::init();
    return array_key_exists($key, self::$config) ? self::$config[$key] : $defaultValue;
}

[sapillai]

Hi @simonklink,

Openprovider cron sync feature is deprecated since version 5.3 of the Openprovider domain module, and is not recommended for versions WHMCS 8+. We suggest that you use the WHMCS native domain sync and do not use the Openprovider custom sync for WHMCS 8 and higher.

This information is available in the readme.

[simonklink]

Hi @sapillai,

Thats ok, but the Configuration::getOrDefault is used on more locations than only in the deprecated domain sync.

So the bug also applies to all the other Configuration::getOrDefault that provide a default value and have a false setting.

[sapillai]

Hi @simonklink,

Thats ok, but the Configuration::getOrDefault is used on more locations than only in the deprecated domain sync.
So the bug also applies to all the other Configuration::getOrDefault that provide a default value and have a false setting.

I have re-opened this issue and will have our developers check this.