DEV: Avoid $ globals (#15453)

Also:
* Remove an unused method (#fill_email)
* Replace a method that was used just once (#generate_username) with `SecureRandom.alphanumeric`
* Remove an obsolete dev puma `tmp/restart` file logic

app/controllers/forums_controller.rb

@@ -17,11 +17,6 @@ class ForumsController < ActionController::Base
       end
     end
 
-    if $shutdown # rubocop:disable Style/GlobalVars
-      render plain: "shutting down", status: (params[:shutdown_ok] ? 200 : 500)
-    else
-      render plain: "ok"
-    end
+    render plain: "ok"
   end
-
 end

app/services/destroy_task.rb

@@ -1,8 +1,7 @@
 # frozen_string_literal: true
 
 class DestroyTask
-
-  def initialize(io = $stdout)
+  def initialize(io = STDOUT)
     @io = io
   end
 

bin/unicorn

@@ -80,14 +80,14 @@ if ARGV.include?("--help")
   exit
 end
 
-# this dev_mode hackery enables, the following to be used to restart unicorn:
+# this dev_mode hackery enables the following to be used to restart unicorn:
 #
 # pkill -USR2 -f 'ruby bin/unicorn'
 #
 # This is handy if you want to bind a key to restarting unicorn in dev
 
 if dev_mode
-  $unicorn_dev_supervisor_pid = Process.pid # rubocop:disable Style/GlobalVars
+  UNICORN_DEV_SUPERVISOR_PID = Process.pid
 
   restart = true
   while restart

config/application.rb

@@ -291,9 +291,12 @@ module Discourse
     require 'logster/redis_store'
     # Use redis for our cache
     config.cache_store = DiscourseRedis.new_redis_store
-    $redis = DiscourseRedis.new # rubocop:disable Style/GlobalVars
+    Discourse.redis = DiscourseRedis.new
     Logster.store = Logster::RedisStore.new(DiscourseRedis.new)
 
+    # Deprecated
+    $redis = Discourse.redis # rubocop:disable Style/GlobalVars
+
     # we configure rack cache on demand in an initializer
     # our setup does not use rack cache and instead defers to nginx
     config.action_dispatch.rack_cache = nil

config/initializers/000-development_reload_warnings.rb

@@ -12,7 +12,7 @@ if Rails.env.development? && !Rails.configuration.cache_classes && Discourse.run
   ]
 
   Listen.to(*paths, only: /\.rb$/) do |modified, added, removed|
-    supervisor_pid = $unicorn_dev_supervisor_pid # rubocop:disable Style/GlobalVars
+    supervisor_pid = UNICORN_DEV_SUPERVISOR_PID
     auto_restart = supervisor_pid && ENV["AUTO_RESTART"] != "0"
 
     files = modified + added + removed

config/initializers/100-watch_for_restart.rb

@@ -1,44 +0,0 @@
-# frozen_string_literal: true
-
-# this is a trivial graceful restart on touch of tmp/restart.
-#
-# It simply drains all the requests (waits up to 4 seconds) and issues a HUP
-#  if you need a more sophisticated cycling restart for multiple thins it will need to be written
-#
-# This works fine for Discourse.org cause we host our app across multiple machines, if you hosting
-#  on a single machine you have a trickier problem at hand as you need to cycle the processes in order
-#
-
-# VIM users rejoice, if you add this to your .vimrc CTRL-a will restart puma:
-# nmap <C-a> <Esc>:!touch tmp/restart<CR><CR>
-
-Thread.new do
-  file = "#{Rails.root}/tmp/restart"
-  old_time = File.ctime(file).to_i if File.exist? file
-  wait_seconds = 4
-
-  if Rails.env.development? && $PROGRAM_NAME =~ /puma/
-    require 'listen'
-
-    time = nil
-
-    begin
-      listener = Listen.to("#{Rails.root}/tmp", only: /restart/) do
-
-        time = File.ctime(file).to_i if File.exist? file
-
-        if old_time != time
-          Rails.logger.info "attempting to reload #{$$} #{$PROGRAM_NAME} in #{wait_seconds} seconds"
-          $shutdown = true # rubocop:disable Style/GlobalVars
-          sleep wait_seconds
-          Rails.logger.info "restarting #{$$}"
-          Process.kill("USR2", $$)
-        end
-      end
-      listener.start
-      sleep
-    rescue => e
-      puts "Failed to watch for restart, this probably means the old postgres directory is in tmp, remove it #{e}"
-    end
-  end
-end

config/unicorn.conf.rb

@@ -26,7 +26,7 @@ end
 pid (ENV["UNICORN_PID_PATH"] || "#{discourse_path}/tmp/pids/unicorn.pid")
 
 if ENV["RAILS_ENV"] != "production"
-  logger Logger.new($stdout)
+  logger Logger.new(STDOUT)
   # we want a longer timeout in dev cause first request can be really slow
   timeout (ENV["UNICORN_TIMEOUT"] && ENV["UNICORN_TIMEOUT"].to_i || 60)
 else

lib/discourse.rb

@@ -1,5 +1,4 @@
 # frozen_string_literal: true
-# rubocop:disable Style/GlobalVars
 
 require 'cache'
 require 'open3'
@@ -653,36 +652,32 @@ module Discourse
   end
 
   def self.git_version
-    $git_version ||=
-      begin
-        git_cmd = 'git rev-parse HEAD'
-        self.try_git(git_cmd, Discourse::VERSION::STRING)
-      end # rubocop:disable Style/GlobalVars
+    @git_version ||= begin
+      git_cmd = 'git rev-parse HEAD'
+      self.try_git(git_cmd, Discourse::VERSION::STRING)
+    end
   end
 
   def self.git_branch
-    $git_branch ||=
-      begin
-        git_cmd = 'git rev-parse --abbrev-ref HEAD'
-        self.try_git(git_cmd, 'unknown')
-      end
+    @git_branch ||= begin
+      git_cmd = 'git rev-parse --abbrev-ref HEAD'
+      self.try_git(git_cmd, 'unknown')
+    end
   end
 
   def self.full_version
-    $full_version ||=
-      begin
-        git_cmd = 'git describe --dirty --match "v[0-9]*"'
-        self.try_git(git_cmd, 'unknown')
-      end
+    @full_version ||= begin
+      git_cmd = 'git describe --dirty --match "v[0-9]*"'
+      self.try_git(git_cmd, 'unknown')
+    end
   end
 
   def self.last_commit_date
-    $last_commit_date ||=
-      begin
-        git_cmd = 'git log -1 --format="%ct"'
-        seconds = self.try_git(git_cmd, nil)
-        seconds.nil? ? nil : DateTime.strptime(seconds, '%s')
-      end
+    @last_commit_date ||= begin
+      git_cmd = 'git log -1 --format="%ct"'
+      seconds = self.try_git(git_cmd, nil)
+      seconds.nil? ? nil : DateTime.strptime(seconds, '%s')
+    end
   end
 
   def self.try_git(git_cmd, default_value)
@@ -1001,9 +996,7 @@ module Discourse
     @preloaded_rails = true
   end
 
-  def self.redis
-    $redis
-  end
+  mattr_accessor :redis
 
   def self.is_parallel_test?
     ENV['RAILS_ENV'] == "test" && ENV['TEST_ENV_NUMBER']
@@ -1031,5 +1024,3 @@ module Discourse
     Rails.env.development? || ENV["ALLOW_DEV_POPULATE"] == "1"
   end
 end
-
-# rubocop:enable Style/GlobalVars

lib/tasks/themes.rake

@@ -145,7 +145,7 @@ task "themes:isolated_test" => :environment do |t, args|
 
   redis = TemporaryRedis.new
   redis.start
-  $redis = redis.instance # rubocop:disable Style/GlobalVars
+  Discourse.redis = redis.instance
   db = TemporaryDb.new
   db.start
   db.migrate

spec/models/user_option_spec.rb

@@ -125,7 +125,7 @@ describe UserOption do
             end
 
             after do
-              $redis.flushdb
+              Discourse.redis.flushdb
             end
 
             it "should have a reason for the first visit" do

spec/rails_helper.rb

@@ -14,21 +14,15 @@ end
 
 require 'rubygems'
 require 'rbtrace'
-
 require 'pry'
 require 'pry-byebug'
 require 'pry-rails'
-
-# Loading more in this block will cause your tests to run faster. However,
-# if you change any configuration or code from libraries loaded here, you'll
-# need to restart spork for it take effect.
 require 'fabrication'
 require 'mocha/api'
 require 'certified'
 require 'webmock/rspec'
 
 class RspecErrorTracker
-
   def self.last_exception=(ex)
     @ex = ex
   end

spec/requests/forums_controller_spec.rb

@@ -19,24 +19,6 @@ RSpec.describe ForumsController do
     end
   end
 
-  describe "during shutdown" do
-    before(:each) do
-      $shutdown = true
-    end
-    after(:each) do
-      $shutdown = nil
-    end
-
-    it "returns a 500 response" do
-      get "/srv/status"
-      expect(response.status).to eq(500)
-    end
-    it "returns a 200 response when given shutdown_ok" do
-      get "/srv/status?shutdown_ok=1"
-      expect(response.status).to eq(200)
-    end
-  end
-
   describe "cluster parameter" do
     it "returns a 500 response if the cluster is not configured" do
       get "/srv/status?cluster=abc"

spec/requests/users_controller_spec.rb

@@ -1610,13 +1610,13 @@ describe UsersController do
 
     context 'is too long' do
       before do
-        get "/u/check_username.json", params: { username: generate_username(User.username_length.last + 1) }
+        get "/u/check_username.json", params: { username: SecureRandom.alphanumeric(SiteSetting.max_username_length.to_i + 1) }
       end
       include_examples 'checking an invalid username'
 
       it 'should return the "too long" message' do
         expect(response.status).to eq(200)
-        expect(response.parsed_body['errors']).to include(I18n.t(:'user.username.long', max: User.username_length.end))
+        expect(response.parsed_body['errors']).to include(I18n.t(:'user.username.long', max: SiteSetting.max_username_length))
       end
     end
 

spec/support/helpers.rb

@@ -58,11 +58,6 @@ module Helpers
     post
   end
 
-  def generate_username(length = 10)
-    range = [*'a'..'z']
-    Array.new(length) { range.sample }.join
-  end
-
   def stub_guardian(user)
     guardian = Guardian.new(user)
     yield(guardian) if block_given?
@@ -82,14 +77,6 @@ module Helpers
     expect(result).to eq(true)
   end
 
-  def fill_email(mail, from, to, body = nil, subject = nil, cc = nil)
-    result = mail.gsub("FROM", from).gsub("TO", to)
-    result.gsub!(/Hey.*/m, body)  if body
-    result.sub!(/We .*/, subject) if subject
-    result.sub!("CC", cc.presence || "")
-    result
-  end
-
   def email(email_name)
     fixture_file("emails/#{email_name}.eml")
   end