From 36e21cacc96dc6011f4fdc4eadbd1d3443a49e69 Mon Sep 17 00:00:00 2001 From: Philipp Date: Mon, 27 Mar 2023 00:17:40 +0200 Subject: [PATCH] Apply suggestions from code review Switch to `isWritable` Co-authored-by: Hypolite Petovan --- src/Core/Config/Capability/IManageConfigValues.php | 2 +- src/Core/Config/Model/DatabaseConfig.php | 4 ++-- src/Core/Config/Model/ReadOnlyFileConfig.php | 4 ++-- src/Module/Admin/Logs/Settings.php | 12 ++++++------ src/Module/Admin/Site.php | 10 +++++----- src/Module/Admin/Storage.php | 6 +++--- tests/src/Core/Config/ConfigTest.php | 4 ++-- view/templates/admin/storage.tpl | 4 ++-- view/theme/frio/templates/admin/storage.tpl | 6 +++--- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/Core/Config/Capability/IManageConfigValues.php b/src/Core/Config/Capability/IManageConfigValues.php index 93e0143b0..964087ca9 100644 --- a/src/Core/Config/Capability/IManageConfigValues.php +++ b/src/Core/Config/Capability/IManageConfigValues.php @@ -66,7 +66,7 @@ interface IManageConfigValues * * @return bool true, if set is disabled */ - public function isSetDisabled(string $cat, string $key): bool; + public function isWritable(string $cat, string $key): bool; /** * Sets a configuration value for system config diff --git a/src/Core/Config/Model/DatabaseConfig.php b/src/Core/Config/Model/DatabaseConfig.php index 058c90359..7b34eda97 100644 --- a/src/Core/Config/Model/DatabaseConfig.php +++ b/src/Core/Config/Model/DatabaseConfig.php @@ -81,9 +81,9 @@ class DatabaseConfig implements IManageConfigValues } /** {@inheritDoc} */ - public function isSetDisabled(string $cat, string $key): bool + public function isWritable(string $cat, string $key): bool { - return $this->cache->getSource($cat, $key) >= Cache::SOURCE_ENV; + return $this->cache->getSource($cat, $key) < Cache::SOURCE_ENV; } /** {@inheritDoc} */ diff --git a/src/Core/Config/Model/ReadOnlyFileConfig.php b/src/Core/Config/Model/ReadOnlyFileConfig.php index f0a25fcf4..cc84a6c0d 100644 --- a/src/Core/Config/Model/ReadOnlyFileConfig.php +++ b/src/Core/Config/Model/ReadOnlyFileConfig.php @@ -69,9 +69,9 @@ class ReadOnlyFileConfig implements IManageConfigValues } /** {@inheritDoc} */ - public function isSetDisabled(string $cat, string $key): bool + public function isWritable(string $cat, string $key): bool { - return $this->configCache->getSource($cat, $key) >= Cache::SOURCE_ENV; + return $this->configCache->getSource($cat, $key) < Cache::SOURCE_ENV; } /** {@inheritDoc} */ diff --git a/src/Module/Admin/Logs/Settings.php b/src/Module/Admin/Logs/Settings.php index 161207176..54f929f09 100644 --- a/src/Module/Admin/Logs/Settings.php +++ b/src/Module/Admin/Logs/Settings.php @@ -48,13 +48,13 @@ class Settings extends BaseAdmin return; } - if (!DI::config()->isSetDisabled('system', 'logfile')) { + if (DI::config()->isWritable('system', 'logfile')) { DI::config()->set('system', 'logfile', $logfile); } - if (!DI::config()->isSetDisabled('system', 'debugging')) { + if (DI::config()->isWritable('system', 'debugging')) { DI::config()->set('system', 'debugging', $debugging); } - if (!DI::config()->isSetDisabled('system', 'loglevel')) { + if (DI::config()->isWritable('system', 'loglevel')) { DI::config()->set('system', 'loglevel', $loglevel); } @@ -88,9 +88,9 @@ class Settings extends BaseAdmin '$clear' => DI::l10n()->t('Clear'), '$logname' => DI::config()->get('system', 'logfile'), // see /help/smarty3-templates#1_1 on any Friendica node - '$debugging' => ['debugging', DI::l10n()->t("Enable Debugging"), DI::config()->get('system', 'debugging'), DI::config()->isSetDisabled('system', 'debugging') ? DI::l10n()->t("Readonly because set by an environment variable") : '', DI::config()->isSetDisabled('system', 'debugging') ? 'disabled' : ''], - '$logfile' => ['logfile', DI::l10n()->t("Log file"), DI::config()->get('system', 'logfile'), DI::l10n()->t("Must be writable by web server. Relative to your Friendica top-level directory.") . (DI::config()->isSetDisabled('system', 'logfile') ? '
' . DI::l10n()->t("Readonly because set by an environment variable") : ''), "", DI::config()->isSetDisabled('system', 'logfile') ? 'disabled' : ''], - '$loglevel' => ['loglevel', DI::l10n()->t("Log level"), DI::config()->get('system', 'loglevel'), DI::config()->isSetDisabled('system', 'loglevel') ? DI::l10n()->t("Readonly because set by an environment variable") : '', $log_choices, DI::config()->isSetDisabled('system', 'loglevel') ? 'disabled' : ''], + '$debugging' => ['debugging', DI::l10n()->t('Enable Debugging'), DI::config()->get('system', 'debugging'), !DI::config()->isWritable('system', 'debugging') ? DI::l10n()->t('Read-only because it is set by an environment variable') : '', !DI::config()->isWritable('system', 'debugging') ? 'disabled' : ''], + '$logfile' => ['logfile', DI::l10n()->t('Log file'), DI::config()->get('system', 'logfile'), DI::l10n()->t('Must be writable by web server. Relative to your Friendica top-level directory.') . (!DI::config()->isWritable('system', 'logfile') ? '
' . DI::l10n()->t('Read-only because it is set by an environment variable') : ''), '', !DI::config()->isWritable('system', 'logfile') ? 'disabled' : ''], + '$loglevel' => ['loglevel', DI::l10n()->t("Log level"), DI::config()->get('system', 'loglevel'), !DI::config()->isWritable('system', 'loglevel') ? DI::l10n()->t('Read-only because it is set by an environment variable') : '', $log_choices, !DI::config()->isWritable('system', 'loglevel') ? 'disabled' : ''], '$form_security_token' => self::getFormSecurityToken("admin_logs"), '$phpheader' => DI::l10n()->t("PHP logging"), '$phphint' => DI::l10n()->t("To temporarily enable logging of PHP errors and warnings you can prepend the following to the index.php file of your installation. The filename set in the 'error_log' line is relative to the friendica top-level directory and must be writeable by the web server. The option '1' for 'log_errors' and 'display_errors' is to enable these options, set to '0' to disable them."), diff --git a/src/Module/Admin/Site.php b/src/Module/Admin/Site.php index 07aa79422..89f55d6f1 100644 --- a/src/Module/Admin/Site.php +++ b/src/Module/Admin/Site.php @@ -165,8 +165,8 @@ class Site extends BaseAdmin $transactionConfig->set('system', 'poco_discovery' , $poco_discovery); $transactionConfig->set('system', 'poco_local_search' , $poco_local_search); $transactionConfig->set('system', 'nodeinfo' , $nodeinfo); - if (!DI::config()->isSetDisabled('config', 'sitename')) { - $transactionConfig->set('config', 'sitename' , $sitename); + if (DI::config()->isWritable('config', 'sitename')) { + $transactionConfig->set('config', 'sitename', $sitename); } $transactionConfig->set('config', 'sender_email' , $sender_email); $transactionConfig->set('system', 'suppress_tags' , $suppress_tags); @@ -190,7 +190,7 @@ class Site extends BaseAdmin } else { $transactionConfig->set('config', 'info', $additional_info); } - if (!DI::config()->isSetDisabled('system', 'language')) { + if (DI::config()->isWritable('system', 'language')) { $transactionConfig->set('system', 'language', $language); } $transactionConfig->set('system', 'theme', $theme); @@ -417,7 +417,7 @@ class Site extends BaseAdmin '$relocate_cmd' => DI::l10n()->t('(Friendica directory)# bin/console relocate https://newdomain.com'), // name, label, value, help string, extra data... - '$sitename' => ['sitename', DI::l10n()->t('Site name'), DI::config()->get('config', 'sitename'), DI::config()->isSetDisabled('config', 'sitename') ? DI::l10n()->t("Readonly because set by an environment variable") : '', '', DI::config()->isSetDisabled('config', 'sitename') ? 'disabled' : ''], + '$sitename' => ['sitename', DI::l10n()->t('Site name'), DI::config()->get('config', 'sitename'), !DI::config()->isWritable('config', 'sitename') ? DI::l10n()->t('Read-only because it is set by an environment variable') : '', '', !DI::config()->isWritable('config', 'sitename') ? 'disabled' : ''], '$sender_email' => ['sender_email', DI::l10n()->t('Sender Email'), DI::config()->get('config', 'sender_email'), DI::l10n()->t('The email address your server shall use to send notification emails from.'), '', '', 'email'], '$system_actor_name' => ['system_actor_name', DI::l10n()->t('Name of the system actor'), User::getActorName(), DI::l10n()->t("Name of the internal system account that is used to perform ActivityPub requests. This must be an unused username. If set, this can't be changed again.")], '$banner' => ['banner', DI::l10n()->t('Banner/Logo'), $banner, ''], @@ -425,7 +425,7 @@ class Site extends BaseAdmin '$shortcut_icon' => ['shortcut_icon', DI::l10n()->t('Shortcut icon'), DI::config()->get('system', 'shortcut_icon'), DI::l10n()->t('Link to an icon that will be used for browsers.')], '$touch_icon' => ['touch_icon', DI::l10n()->t('Touch icon'), DI::config()->get('system', 'touch_icon'), DI::l10n()->t('Link to an icon that will be used for tablets and mobiles.')], '$additional_info' => ['additional_info', DI::l10n()->t('Additional Info'), $additional_info, DI::l10n()->t('For public servers: you can add additional information here that will be listed at %s/servers.', Search::getGlobalDirectory())], - '$language' => ['language', DI::l10n()->t('System language'), DI::config()->get('system', 'language'), DI::config()->isSetDisabled('system', 'language') ? DI::l10n()->t("Readonly because set by an environment variable") : '', $lang_choices, DI::config()->isSetDisabled('system', 'language') ? 'disabled' : ''], + '$language' => ['language', DI::l10n()->t('System language'), DI::config()->get('system', 'language'), !DI::config()->isWritable('system', 'language') ? DI::l10n()->t('Read-only because it is set by an environment variable") : '', $lang_choices, !DI::config()->isWritable('system', 'language') ? 'disabled' : ''], '$theme' => ['theme', DI::l10n()->t('System theme'), DI::config()->get('system', 'theme'), DI::l10n()->t('Default system theme - may be over-ridden by user profiles - Change default theme settings', DI::baseUrl() . '/admin/themes'), $theme_choices], '$theme_mobile' => ['theme_mobile', DI::l10n()->t('Mobile system theme'), DI::config()->get('system', 'mobile-theme', '---'), DI::l10n()->t('Theme for mobile devices'), $theme_choices_mobile], '$force_ssl' => ['force_ssl', DI::l10n()->t('Force SSL'), DI::config()->get('system', 'force_ssl'), DI::l10n()->t('Force all Non-SSL requests to SSL - Attention: on some systems it could lead to endless loops.')], diff --git a/src/Module/Admin/Storage.php b/src/Module/Admin/Storage.php index 1bb5bf9ab..04c200429 100644 --- a/src/Module/Admin/Storage.php +++ b/src/Module/Admin/Storage.php @@ -76,7 +76,7 @@ class Storage extends BaseAdmin } } - if (!empty($_POST['submit_save_set']) && !DI::config()->isSetDisabled('storage', 'name') ) { + if (!empty($_POST['submit_save_set']) && DI::config()->isWritable('storage', 'name') ) { try { $newstorage = DI::storageManager()->getWritableStorageByName($storagebackend); @@ -129,7 +129,7 @@ class Storage extends BaseAdmin 'prefix' => $storage_form_prefix, 'form' => $storage_form, 'active' => $current_storage_backend instanceof ICanWriteToStorage && $name === $current_storage_backend::getName(), - 'set_disabled' => DI::config()->isSetDisabled('storage', 'name'), + 'is_writable' => DI::config()->isWritable('storage', 'name'), ]; } @@ -146,7 +146,7 @@ class Storage extends BaseAdmin '$save_reload' => DI::l10n()->t('Save & Reload'), '$noconfig' => DI::l10n()->t('This backend doesn\'t have custom settings'), '$form_security_token' => self::getFormSecurityToken("admin_storage"), - '$storagebackend_ro_txt' => DI::config()->isSetDisabled('storage', 'name') ? DI::l10n()->t("Changing the current backend is prohibited because it is set by an environment variable") : '', + '$storagebackend_ro_txt' => !DI::config()->isWritable('storage', 'name') ? DI::l10n()->t('Changing the current backend is prohibited because it is set by an environment variable') : '', '$storagebackend' => $current_storage_backend instanceof ICanWriteToStorage ? $current_storage_backend::getName() : DI::l10n()->t('Database (legacy)'), '$availablestorageforms' => $available_storage_forms, ]); diff --git a/tests/src/Core/Config/ConfigTest.php b/tests/src/Core/Config/ConfigTest.php index 0c9d671ea..800f41a7e 100644 --- a/tests/src/Core/Config/ConfigTest.php +++ b/tests/src/Core/Config/ConfigTest.php @@ -629,9 +629,9 @@ class ConfigTest extends DatabaseTest foreach ($data as $category => $keyvalues) { foreach ($keyvalues as $key => $value) { if (!empty($assertDisabled[$category][$key])) { - static::assertTrue($config->isSetDisabled($category, $key), sprintf("%s.%s is not true", $category, $key)); + static::assertTrue($config->isWritable($category, $key), sprintf('%s.%s is not true', $category, $key)); } else { - static::assertFalse($config->isSetDisabled($category, $key), sprintf("%s.%s is not false", $category, $key)); + static::assertFalse($config->isWritable($category, $key), sprintf('%s.%s is not false', $category, $key)); } } } diff --git a/view/templates/admin/storage.tpl b/view/templates/admin/storage.tpl index f39e2aa96..787ec5604 100644 --- a/view/templates/admin/storage.tpl +++ b/view/templates/admin/storage.tpl @@ -20,14 +20,14 @@ {{if $storage.form}} - {{if ! $storage.set_disabled}} + {{if $is_writable}} {{if $storage.active}} {{else}} {{/if}} {{/if}} - {{elseif ! $storage.set_disabled}} + {{elseif $is_writable}}
{{/if}} diff --git a/view/theme/frio/templates/admin/storage.tpl b/view/theme/frio/templates/admin/storage.tpl index cdbe7bc34..c1bfe371b 100644 --- a/view/theme/frio/templates/admin/storage.tpl +++ b/view/theme/frio/templates/admin/storage.tpl @@ -6,7 +6,7 @@
{{$label_current}}: {{$storagebackend}} {{if $storagebackend_ro_txt}} -

{{$storagebackend_ro_txt nofilter}} +
{{$storagebackend_ro_txt nofilter}} {{/if}}

@@ -36,14 +36,14 @@
{{if $storage.form}} - {{if ! $storage.set_disabled}} + {{if $is_writable}} {{if $storage.active}} {{else}} {{/if}} {{/if}} - {{elseif ! $storage.set_disabled }} + {{elseif $is_writable}} {{/if}}