Avoid deprecated site setting logging in `SiteSetting.settings_hash`

Avoid deprecated site setting logging in SiteSetting.settings_hash.

From 1a57be32483af99b9b4d2c6f3ea6c6e230f68e7f Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Tue, 20 Nov 2018 11:59:38 +0800
Subject: [PATCH] Avoid deprecated site setting logging in
 `SiteSetting.settings_hash`.


diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb
index 07bd5c4..f7be775 100644
--- a/lib/site_setting_extension.rb
+++ b/lib/site_setting_extension.rb
@@ -177,9 +177,17 @@ module SiteSettingExtension
 
   def settings_hash
     result = {}
+    deprecated_settings = SiteSettings::DeprecatedSettings::SETTINGS.map { |s| s[0] }
+
     defaults.all.keys.each do |s|
-      result[s] = send(s).to_s
+      result[s] =
+        if deprecated_settings.include?(s.to_s)
+          send(s, warn: false).to_s
+        else
+          send(s).to_s
+        end
     end
+
     result
   end
 
diff --git a/lib/site_settings/deprecated_settings.rb b/lib/site_settings/deprecated_settings.rb
index 6d065dc..e1fd68a 100644
--- a/lib/site_settings/deprecated_settings.rb
+++ b/lib/site_settings/deprecated_settings.rb
@@ -1,7 +1,7 @@
 module SiteSettings; end
 
 module SiteSettings::DeprecatedSettings
-  DEPRECATED_SETTINGS = [
+  SETTINGS = [
     ['logo_url', 'logo', false, '2.4'],
     ['logo_small_url', 'logo_small', false, '2.4'],
     ['digest_logo_url', 'digest_logo', false, '2.4'],
@@ -15,7 +15,7 @@ module SiteSettings::DeprecatedSettings
   ]
 
   def setup_deprecated_methods
-    DEPRECATED_SETTINGS.each do |old_setting, new_setting, override, version|
+    SETTINGS.each do |old_setting, new_setting, override, version|
       unless override
         SiteSetting.singleton_class.public_send(
           :alias_method, :"_#{old_setting}", :"#{old_setting}"
diff --git a/spec/models/site_setting_spec.rb b/spec/models/site_setting_spec.rb
index 3942afe..6c6f556 100644
--- a/spec/models/site_setting_spec.rb
+++ b/spec/models/site_setting_spec.rb
@@ -163,10 +163,10 @@ describe SiteSetting do
 
     it 'should act as a proxy to the new methods' do
       begin
-        original_settings = SiteSettings::DeprecatedSettings::DEPRECATED_SETTINGS
-        SiteSettings::DeprecatedSettings::DEPRECATED_SETTINGS.clear
+        original_settings = SiteSettings::DeprecatedSettings::SETTINGS
+        SiteSettings::DeprecatedSettings::SETTINGS.clear
 
-        SiteSettings::DeprecatedSettings::DEPRECATED_SETTINGS.push([
+        SiteSettings::DeprecatedSettings::SETTINGS.push([
           'use_https', 'force_https', true, '0.0.1'
         ])
 
@@ -186,9 +186,9 @@ describe SiteSetting do
         expect(SiteSetting.force_https).to eq(false)
         expect(SiteSetting.force_https?).to eq(false)
       ensure
-        SiteSettings::DeprecatedSettings::DEPRECATED_SETTINGS.clear
+        SiteSettings::DeprecatedSettings::SETTINGS.clear
 
-        SiteSettings::DeprecatedSettings::DEPRECATED_SETTINGS.concat(
+        SiteSettings::DeprecatedSettings::SETTINGS.concat(
           original_settings
         )
       end

GitHub

1 Like

Technically we should use a Set here not an Array I know there are not too many of them but still…

1 Like

Hmm I think using an array is fine here. Even in the case where someone duplicates a deprecated site setting, the only downside is that Array#include? becomes slightly less inefficient.

Set though is such an easy change and this is doing an include? per setting

Done in

:slight_smile:

2 Likes