DEV: stop mutating inputs as a side effect

DEV: stop mutating inputs as a side effect

We had quite a few cases in core where inputs are being mutated as a side effect of calling a method.

This handles all the cases where specs caught this.

Mutating inputs makes code harder to reason about. Eg:

frog = "frog"
jump(frog)
puts frog
"fly" # ?????

This commit is part of a followup commit that adds # frozen_string_literal to all our specs.

diff --git a/lib/admin_user_index_query.rb b/lib/admin_user_index_query.rb
index 19df51a..6949832 100644
--- a/lib/admin_user_index_query.rb
+++ b/lib/admin_user_index_query.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require_dependency 'trust_level'
 
 class AdminUserIndexQuery
@@ -108,7 +110,7 @@ class AdminUserIndexQuery
 
     filter = params[:filter]
     if filter.present?
-      filter.strip!
+      filter = filter.strip
       if ip = IPAddr.new(filter) rescue nil
         @query.where('ip_address <<= :ip OR registration_ip_address <<= :ip', ip: ip.to_cidr_s)
       else
diff --git a/lib/discourse_tagging.rb b/lib/discourse_tagging.rb
index ffd8b14..4ddc887 100644
--- a/lib/discourse_tagging.rb
+++ b/lib/discourse_tagging.rb
@@ -111,8 +111,9 @@ module DiscourseTagging
 
     term = opts[:term]
     if term.present?
-      term.gsub!("_", "\\_")
-      term = clean_tag(term).downcase
+      term = term.gsub("_", "\\_")
+      clean_tag(term)
+      term.downcase!
       query = query.where('lower(tags.name) like ?', "%#{term}%")
     end
 
diff --git a/lib/email/message_builder.rb b/lib/email/message_builder.rb
index 66a9230..55b64cd 100644
--- a/lib/email/message_builder.rb
+++ b/lib/email/message_builder.rb
@@ -118,8 +118,13 @@ module Email
     end
 
     def body
-      body = @opts[:body]
-      body = I18n.t("#{@opts[:template]}.text_body_template", template_args).dup if @opts[:template]
+      body = nil
+
+      if @opts[:template]
+        body = I18n.t("#{@opts[:template]}.text_body_template", template_args).dup
+      else
+        body = @opts[:body].dup
+      end
 
       if @template_args[:unsubscribe_instructions].present?
         body << "\n"
diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb
index e62ca8a..bcca40f 100644
--- a/lib/site_setting_extension.rb
+++ b/lib/site_setting_extension.rb
@@ -382,7 +382,7 @@ module SiteSettingExtension
 
   def filter_value(name, value)
     if HOSTNAME_SETTINGS.include?(name)
-      value.split("|").map { |url| get_hostname(url) }.compact.uniq.join("|")
+      value.split("|").map { |url| url.strip!; get_hostname(url) }.compact.uniq.join("|")
     else
       value
     end
@@ -488,7 +488,6 @@ module SiteSettingExtension
   end
 
   def get_hostname(url)
-    url.strip!
 
     host = begin
       URI.parse(url)&.host
diff --git a/lib/text_cleaner.rb b/lib/text_cleaner.rb
index cdbccc1..fa136a4 100644
--- a/lib/text_cleaner.rb
+++ b/lib/text_cleaner.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 #
 # Clean up a text
 #
@@ -27,6 +29,8 @@ class TextCleaner
   end
 
   def self.clean(text, opts = {})
+    text = text.dup
+
     # Remove invalid byte sequences
     text.scrub!("")
     # Replace !!!!! with a single !
@@ -38,7 +42,7 @@ class TextCleaner
     # Capitalize first letter, but only when entire first word is lowercase
     first, rest = text.split(' ', 2)
     if first && opts[:capitalize_first_letter] && first == first.mb_chars.downcase
-      text = "#{first.mb_chars.capitalize}#{rest ? ' ' + rest : ''}"
+      text = +"#{first.mb_chars.capitalize}#{rest ? ' ' + rest : ''}"
     end
     # Remove unnecessary periods at the end
     text.sub!(/([^.])\.+(\s*)\z/, '\1\2') if opts[:remove_all_periods_from_the_end]
diff --git a/lib/topic_query.rb b/lib/topic_query.rb
index e7950d4..85e3739 100644
--- a/lib/topic_query.rb
+++ b/lib/topic_query.rb
@@ -685,17 +685,26 @@ class TopicQuery
     if SiteSetting.tagging_enabled
       result = result.preload(:tags)
 
-      if @options[:tags] && @options[:tags].size > 0
-        @options[:tags] = @options[:tags].split unless @options[:tags].respond_to?('each')
-        @options[:tags].each { |t| t.downcase! if t.is_a? String }
+      tags = @options[:tags]
+
+      if tags && tags.size > 0
+        tags = tags.split if String === tags
+
+        tags = tags.map do |t|
+          if String === t
+            t.downcase
+          else
+            t
+          end
+        end
 
         if @options[:match_all_tags]
           # ALL of the given tags:
-          tags_count = @options[:tags].length
-          @options[:tags] = Tag.where_name(@options[:tags]).pluck(:id) unless @options[:tags][0].is_a?(Integer)
+          tags_count = tags.length
+          tags = Tag.where_name(tags).pluck(:id) unless Integer === tags[0]
 
-          if tags_count == @options[:tags].length
-            @options[:tags].each_with_index do |tag, index|
+          if tags_count == tags.length
+            tags.each_with_index do |tag, index|
               sql_alias = ['t', index].join
               result = result.joins("INNER JOIN topic_tags #{sql_alias} ON #{sql_alias}.topic_id = topics.id AND #{sql_alias}.tag_id = #{tag}")
             end
@@ -705,12 +714,17 @@ class TopicQuery
         else
           # ANY of the given tags:
           result = result.joins(:tags)
-          if @options[:tags][0].is_a?(Integer)
-            result = result.where("tags.id in (?)", @options[:tags])
+          if Integer === tags[0]
+            result = result.where("tags.id in (?)", tags)
           else
-            result = result.where("lower(tags.name) in (?)", @options[:tags])
+            result = result.where("lower(tags.name) in (?)", tags)
           end
         end
+
+        # TODO: this is very side-effecty and should be changed
+        # It is done cause further up we expect normalized tags
+        @options[:tags] = tags
+
       elsif @options[:no_tags]
         # the following will do: ("topics"."id" NOT IN (SELECT DISTINCT "topic_tags"."topic_id" FROM "topic_tags"))
         result = result.where.not(id: TopicTag.distinct.pluck(:topic_id))
diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb
index d9c72ed..f414351 100644
--- a/lib/user_name_suggester.rb
+++ b/lib/user_name_suggester.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module UserNameSuggester
   GENERIC_NAMES = ['i', 'me', 'info', 'support', 'admin', 'webmaster', 'hello', 'mail', 'office', 'contact', 'team']
 
@@ -41,7 +43,7 @@ module UserNameSuggester
   end
 
   def self.sanitize_username(name)
-    name = name.to_s
+    name = name.to_s.dup
 
     if SiteSetting.unicode_usernames
       name.unicode_normalize!
diff --git a/lib/validators/reply_by_email_address_validator.rb b/lib/validators/reply_by_email_address_validator.rb
index a9110e0..02864be 100644
--- a/lib/validators/reply_by_email_address_validator.rb
+++ b/lib/validators/reply_by_email_address_validator.rb
@@ -1,15 +1,16 @@
+# frozen_string_literal: true
+
 class ReplyByEmailAddressValidator
   def initialize(opts = {})
     @opts = opts
   end
 
   def valid_value?(val)
-    val&.strip!
-
     return true  if val.blank?
     return false if !val.include?("@")
 
     value = val.dup
+    value.strip!
 
     if SiteSetting.find_related_post_with_key
       return false if !value.include?("%{reply_key}")
diff --git a/lib/wizard/step_updater.rb b/lib/wizard/step_updater.rb
index 5b1c88d..81d9ba0 100644
--- a/lib/wizard/step_updater.rb
+++ b/lib/wizard/step_updater.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Wizard
   class StepUpdater
     include ActiveModel::Model
@@ -29,7 +31,7 @@ class Wizard
     end
 
     def update_setting(id, value)
-      value.strip! if value.is_a?(String)
+      value = value.strip if value.is_a?(String)
 
       if !value.is_a?(Upload) && SiteSetting.type_supervisor.get_type(id) == :upload
         value = Upload.get_from_url(value) || ''
diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb
index 7593d0f..dc35161 100644
--- a/spec/components/site_setting_extension_spec.rb
+++ b/spec/components/site_setting_extension_spec.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 require_dependency 'site_setting_extension'
 require_dependency 'site_settings/local_process_provider'
@@ -776,8 +778,8 @@ describe SiteSettingExtension do
   describe "get_hostname" do
 

[... diff too long, it was truncated ...]

GitHub sha: 0a5a6dfd

1 Like