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.

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

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
 

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"

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

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]

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))

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!

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}")

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) || ''

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
 
     it "properly extracts the hostname" do
+      # consider testing this through a public interface, this tests implementation details
       expect(settings.send(:get_hostname, "discourse.org")).to eq("discourse.org")
-      expect(settings.send(:get_hostname, " discourse.org ")).to eq("discourse.org")
       expect(settings.send(:get_hostname, "@discourse.org")).to eq("discourse.org")
       expect(settings.send(:get_hostname, "https://discourse.org")).to eq("discourse.org")
     end