DEV: Add support for Rails 6

Minor fixes to add Rails 6 support to Discourse, we now will boot
with RAILS_MASTER=1, all specs pass

Only one tiny deprecation left

Largest change was the way ActiveModel:Errors changed interface a
bit but there is a simple backwards compat way of working it

app/controllers/extra_locales_controller.rb

@@ -3,7 +3,11 @@
 class ExtraLocalesController < ApplicationController
 
   layout :false
-  skip_before_action :check_xhr, :preload_json, :redirect_to_login_if_required
+
+  skip_before_action :check_xhr,
+    :preload_json,
+    :redirect_to_login_if_required,
+    :verify_authenticity_token
 
   def show
     bundle = params[:bundle]

app/controllers/uploads_controller.rb

@@ -152,7 +152,7 @@ class UploadsController < ApplicationController
       upload.update_columns(retain_hours: retain_hours) if retain_hours > 0
     end
 
-    upload.errors.empty? ? upload : { errors: upload.errors.values.flatten }
+    upload.errors.empty? ? upload : { errors: upload.errors.to_hash.values.flatten }
   ensure
     tempfile&.close!
   end

app/mailers/user_notifications.rb

@@ -584,7 +584,7 @@ class UserNotifications < ActionMailer::Base
       )
 
       unless translation_override_exists
-        html = UserNotificationRenderer.new(Rails.configuration.paths["app/views"]).render(
+        html = UserNotificationRenderer.with_view_paths(Rails.configuration.paths["app/views"]).render(
           template: 'email/invite',
           format: :html,
           locals: { message: PrettyText.cook(message, sanitize: false).html_safe,
@@ -611,7 +611,8 @@ class UserNotifications < ActionMailer::Base
       end
 
       unless translation_override_exists
-        html = UserNotificationRenderer.new(Rails.configuration.paths["app/views"]).render(
+
+        html = UserNotificationRenderer.with_view_paths(Rails.configuration.paths["app/views"]).render(
           template: 'email/notification',
           format: :html,
           locals: { context_posts: context_posts,

app/models/group.rb

@@ -99,7 +99,7 @@ class Group < ActiveRecord::Base
   validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values }
 
   scope :visible_groups, Proc.new { |user, order, opts|
-    groups = Group.order(order || "name ASC")
+    groups = self.order(order || "name ASC")
 
     if !opts || !opts[:include_everyone]
       groups = groups.where("groups.id > 0")

app/models/user.rb

@@ -988,7 +988,7 @@ class User < ActiveRecord::Base
   end
 
   def secure_category_ids
-    cats = self.admin? ? Category.where(read_restricted: true) : secure_categories.references(:categories)
+    cats = self.admin? ? Category.unscoped.where(read_restricted: true) : secure_categories.references(:categories)
     cats.pluck('categories.id').sort
   end
 

app/views/layouts/_head.html.erb

@@ -5,7 +5,7 @@
 <%- end %>
 <% site_apple_touch_icon_url = SiteSetting.site_apple_touch_icon_url %>
 <%- if site_apple_touch_icon_url.present? %>
-<link rel="apple-touch-icon" type="image/png" href="<%= UrlHelper.absolute(site_apple_touch_icon_url) %>">
+<link rel="apple-touch-icon" type="image/png" href="<%= ::UrlHelper.absolute(site_apple_touch_icon_url) %>">
 <%- end %>
 <meta name="theme-color" content="#<%= ColorScheme.hex_for_name('header_background', scheme_id) %>">
 <% if mobile_view? %>

app/views/metadata/opensearch.xml.erb

@@ -4,7 +4,7 @@
   <Description>Search for posts on <%= SiteSetting.title %></Description>
   <Tags>discourse forum</Tags>
   <% site_favicon_url = SiteSetting.site_favicon_url %>
-  <% absolute_site_favicon_url = UrlHelper.absolute(site_favicon_url) %>
+  <% absolute_site_favicon_url = ::UrlHelper.absolute(site_favicon_url) %>
   <% if site_favicon_url =~ /\.ico$/ -%>
     <Image height="16" width="16" type="image/vnd.microsoft.icon"><%= absolute_site_favicon_url %></Image>
   <%- else -%>

app/views/users/password_reset.html.erb

@@ -4,7 +4,7 @@
       <%= @error %>
     </div>
   <%end%>
-  <% if @user.present? and @user.errors.any? %>
+  <% if @user.present? and @user.errors.present? %>
     <div class='alert alert-error'>
       <% @user.errors.full_messages.each do |msg| %>
         <li><%= msg %></li>

lib/discourse_tagging.rb

@@ -27,13 +27,13 @@ module DiscourseTagging
         staff_tags = new_tag_names & all_staff_tags
         staff_tags += new_tag_names & hidden_tags
         if staff_tags.present?
-          topic.errors[:base] << I18n.t("tags.staff_tag_disallowed", tag: staff_tags.join(" "))
+          topic.errors.add(:base, I18n.t("tags.staff_tag_disallowed", tag: staff_tags.join(" ")))
           return false
         end
 
         staff_tags = removed_tag_names & all_staff_tags
         if staff_tags.present?
-          topic.errors[:base] << I18n.t("tags.staff_tag_remove_disallowed", tag: staff_tags.join(" "))
+          topic.errors.add(:base, I18n.t("tags.staff_tag_remove_disallowed", tag: staff_tags.join(" ")))
           return false
         end
 
@@ -77,7 +77,7 @@ module DiscourseTagging
 
         # validate minimum required tags for a category
         if !guardian.is_staff? && category && category.minimum_required_tags > 0 && tags.length < category.minimum_required_tags
-          topic.errors[:base] << I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)
+          topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags))
           return false
         end
 
@@ -85,7 +85,7 @@ module DiscourseTagging
       else
         # validate minimum required tags for a category
         if !guardian.is_staff? && category && category.minimum_required_tags > 0
-          topic.errors[:base] << I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)
+          topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags))
           return false
         end
 

lib/email/receiver.rb

@@ -1112,7 +1112,9 @@ module Email
         raise TooShortPost
       end
 
-      raise InvalidPost, errors.join("\n") if result.errors.any?
+      if result.errors.present?
+        raise InvalidPost, errors.join("\n")
+      end
 
       if result.post
         @incoming_email.update_columns(topic_id: result.post.topic_id, post_id: result.post.id)

lib/freedom_patches/active_record_attribute_methods.rb

@@ -10,9 +10,19 @@
 module ActiveRecord
   module AttributeMethods
     module ClassMethods
+      # this is for Rails 5
       def enforce_raw_sql_whitelist(*args)
         return
       end
+
+      BLANK_ARRAY = [].freeze
+
+      # this patch just allows everyting in Rails 6
+      def disallow_raw_sql!(args, permit: nil)
+        # we may consider moving to https://github.com/rails/rails/pull/33330
+        # once all frozen string hints are in place
+        return BLANK_ARRAY
+      end
     end
   end
 end

lib/freedom_patches/rails6.rb

@@ -0,0 +1,9 @@
+# this is a quick backport of a new method introduced in Rails 6
+# to be removed after we upgrade to Rails 6
+if ! defined? ActionView::Base.with_view_paths
+  class ActionView::Base
+    class << self
+      alias with_view_paths new
+    end
+  end
+end

lib/has_errors.rb

@@ -25,7 +25,7 @@ module HasErrors
   end
 
   def add_error(msg)
-    errors[:base] << msg unless errors[:base].include?(msg)
+    errors.add(:base, msg) unless errors[:base].include?(msg)
   end
 
   def add_errors_from(obj)

lib/new_post_manager.rb

@@ -124,14 +124,14 @@ class NewPostManager
 
     if post.errors[:raw].present?
       result = NewPostResult.new(:created_post, false)
-      result.errors[:base] << post.errors[:raw]
+      result.errors.add(:base, post.errors[:raw])
       return result
     elsif manager.args[:topic_id]
       topic = Topic.unscoped.where(id: manager.args[:topic_id]).first
 
       unless manager.user.guardian.can_create_post_on_topic?(topic)
         result = NewPostResult.new(:created_post, false)
-        result.errors[:base] << I18n.t(:topic_not_found)
+        result.errors.add(:base, I18n.t(:topic_not_found))
         return result
       end
     elsif manager.args[:category]
@@ -139,7 +139,7 @@ class NewPostManager
 
       unless manager.user.guardian.can_create_topic_on_category?(category)
         result = NewPostResult.new(:created_post, false)
-        result.errors[:base] << I18n.t("js.errors.reasons.forbidden")
+        result.errors.add(:base, I18n.t("js.errors.reasons.forbidden"))
         return result
       end
     end
@@ -172,7 +172,7 @@ class NewPostManager
   def perform
     if !self.class.exempt_user?(@user) && matches = WordWatcher.new("#{@args[:title]} #{@args[:raw]}").should_block?
       result = NewPostResult.new(:created_post, false)
-      result.errors[:base] << I18n.t('contains_blocked_words', word: matches[0])
+      result.errors.add(:base, I18n.t('contains_blocked_words', word: matches[0]))
       return result
     end
 

lib/new_post_result.rb

@@ -27,7 +27,7 @@ class NewPostResult
     if arr.empty?
       @success = true
     else
-      arr.each { |e| errors[:base] << e unless errors[:base].include?(e) }
+      arr.each { |e| errors.add(:base, e) unless errors[:base].include?(e) }
     end
   end
 

lib/post_creator.rb

@@ -91,7 +91,7 @@ class PostCreator
     @post = nil
 
     if @user.suspended? && !skip_validations?
-      errors[:base] << I18n.t(:user_is_suspended)
+      errors.add(:base, I18n.t(:user_is_suspended))
       return false
     end
 
@@ -102,8 +102,9 @@ class PostCreator
       max_allowed_message_recipients = SiteSetting.max_allowed_message_recipients
 
       if names.length > max_allowed_message_recipients
-        errors[:base] << I18n.t(:max_pm_recepients,
-          recipients_limit: max_allowed_message_recipients
+        errors.add(
+          :base,
+          I18n.t(:max_pm_recepients, recipients_limit: max_allowed_message_recipients)
         )
 
         return false
@@ -124,7 +125,7 @@ class PostCreator
         ", user_ids: users.keys)
         .pluck(:id).each do |m|
 
-        errors[:base] << I18n.t(:not_accepting_pms, username: users[m])
+        errors.add(:base, I18n.t(:not_accepting_pms, username: users[m]))
       end
 
       return false if errors[:base].present?
@@ -136,7 +137,7 @@ class PostCreator
     else
       @topic = Topic.find_by(id: @opts[:topic_id])
       unless @topic.present? && (@opts[:skip_guardian] || guardian.can_create?(Post, @topic))
-        errors[:base] << I18n.t(:topic_not_found)
+        errors.add(:base, I18n.t(:topic_not_found))
         return false
       end
     end
@@ -147,7 +148,7 @@ class PostCreator
 
     if @post.has_host_spam?
       @spam = true
-      errors[:base] << I18n.t(:spamming_host)
+      errors.add(:base, I18n.t(:spamming_host))
       return false
     end
 

lib/post_revisor.rb

@@ -562,7 +562,7 @@ class PostRevisor
       category.update_column(:description, new_description)
       @category_changed = category
     else
-      @post.errors[:base] << I18n.t("category.errors.description_incomplete")
+      @post.errors.add(:base, I18n.t("category.errors.description_incomplete"))
     end
   end
 

lib/topic_creator.rb

@@ -160,7 +160,7 @@ class TopicCreator
         # Validate minimum required tags for a category
         category = find_category
         if category.present? && category.minimum_required_tags > 0
-          topic.errors[:base] << I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags)
+          topic.errors.add(:base, I18n.t("tags.minimum_required_tags", count: category.minimum_required_tags))
           rollback_from_errors!(topic)
         end
       end

lib/validators/post_validator.rb

@@ -59,7 +59,7 @@ class Validators::PostValidator < ActiveModel::Validator
 
   def watched_words(post)
     if !post.acting_user&.staff? && !post.acting_user&.staged && matches = WordWatcher.new(post.raw).should_block?
-      post.errors[:base] << I18n.t('contains_blocked_words', word: matches[0])
+      post.errors.add(:base, I18n.t('contains_blocked_words', word: matches[0]))
     end
   end
 

lib/validators/upload_url_validator.rb

@@ -8,7 +8,7 @@ class UploadUrlValidator < ActiveModel::EachValidator
         end
 
       unless uri && Upload.exists?(url: value)
-        record.errors[attribute] << (options[:message] || I18n.t('errors.messages.invalid'))
+        record.errors.add(attribute, options[:message] || I18n.t('errors.messages.invalid'))
       end
     end
   end

lib/validators/url_validator.rb

@@ -15,7 +15,7 @@ class UrlValidator < ActiveModel::EachValidator
         end
 
       unless valid
-        record.errors[attribute] << (options[:message] || I18n.t('errors.messages.invalid'))
+        record.errors.add(attribute, options[:message] || I18n.t('errors.messages.invalid'))
       end
     end
   end

spec/components/concern/has_custom_fields_spec.rb

@@ -22,10 +22,10 @@ describe HasCustomFields do
       DB.exec("drop table custom_fields_test_items")
       DB.exec("drop table custom_fields_test_item_custom_fields")
 
-      # import is making my life hard, we need to nuke this out of orbit
-      des = ActiveSupport::DescendantsTracker.class_variable_get :@@direct_descendants
-      des[ActiveRecord::Base].delete(CustomFieldsTestItem)
-      des[ActiveRecord::Base].delete(CustomFieldsTestItemCustomField)
+      # this weakref in the descendant tracker should clean up the two tests
+      # if this becomes an issue we can revisit (watch out for erratic tests)
+      Object.send(:remove_const, :CustomFieldsTestItem)
+      Object.send(:remove_const, :CustomFieldsTestItemCustomField)
     end
 
     it "simple modification of custom fields" do

spec/components/concern/has_search_data_spec.rb

@@ -21,10 +21,10 @@ describe HasSearchData do
       DB.exec("drop table model_items")
       DB.exec("drop table model_item_search_data")
 
-      # import is making my life hard, we need to nuke this out of orbit
-      des = ActiveSupport::DescendantsTracker.class_variable_get :@@direct_descendants
-      des[ActiveRecord::Base].delete(ModelItem)
-      des[ActiveRecord::Base].delete(ModelItemSearchData)
+      # this weakref in the descendant tracker should clean up the two tests
+      # if this becomes an issue we can revisit (watch out for erratic tests)
+      Object.send(:remove_const, :ModelItem)
+      Object.send(:remove_const, :ModelItemSearchData)
     end
 
     let(:item) do

spec/components/concern/positionable_spec.rb

@@ -20,9 +20,9 @@ describe Positionable do
     after do
       DB.exec("drop table test_items")
 
-      # import is making my life hard, we need to nuke this out of orbit
-      des = ActiveSupport::DescendantsTracker.class_variable_get :@@direct_descendants
-      des[ActiveRecord::Base].delete(TestItem)
+      # this weakref in the descendant tracker should clean up the two tests
+      # if this becomes an issue we can revisit (watch out for erratic tests)
+      Object.send(:remove_const, :TestItem)
     end
 
     it "can position stuff correctly" do

spec/components/concern/searchable_spec.rb

@@ -22,10 +22,10 @@ describe Searchable do
       DB.exec("drop table searchable_records")
       DB.exec("drop table searchable_record_search_data")
 
-      # import is making my life hard, we need to nuke this out of orbit
-      des = ActiveSupport::DescendantsTracker.class_variable_get :@@direct_descendants
-      des[ActiveRecord::Base].delete(SearchableRecord)
-      des[ActiveRecord::Base].delete(SearchableRecordSearchData)
+      # this weakref in the descendant tracker should clean up the two tests
+      # if this becomes an issue we can revisit (watch out for erratic tests)
+      Object.send(:remove_const, :SearchableRecord)
+      Object.send(:remove_const, :SearchableRecordSearchData)
     end
 
     let(:item) { SearchableRecord.create! }

spec/components/validators/post_validator_spec.rb

@@ -227,7 +227,7 @@ describe Validators::PostValidator do
 
       it "should add an error" do
         validator.unique_post_validator(new_post)
-        expect(new_post.errors.keys).to contain_exactly(:raw)
+        expect(new_post.errors.to_hash.keys).to contain_exactly(:raw)
       end
 
       it "should not add an error if post.skip_unique_check is true" do

spec/models/category_spec.rb

@@ -23,7 +23,7 @@ describe Category do
     category.search_priority = Searchable::PRIORITIES.values.last + 1
 
     expect(category.valid?).to eq(false)
-    expect(category.errors.keys).to contain_exactly(:search_priority)
+    expect(category.errors.to_hash.keys).to contain_exactly(:search_priority)
   end
 
   it 'validates uniqueness in case insensitive way' do

spec/models/tag_user_spec.rb

@@ -164,7 +164,7 @@ describe TagUser do
         result = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(staff), ["foo"])
         expect(result).to eq(true)
 
-        topic.errors[:base].clear
+        topic.errors.clear
 
         result = DiscourseTagging.tag_topic_by_names(topic, Guardian.new(user), [])
         expect(result).to eq(false)

spec/models/topic_timer_spec.rb

@@ -84,7 +84,7 @@ RSpec.describe TopicTimer, type: :model do
             )
 
             expect(topic_timer).to_not be_valid
-            expect(topic_timer.errors.keys).to include(:category_id)
+            expect(topic_timer.errors).to include(:category_id)
           end
         end
 

spec/requests/uploads_controller_spec.rb

@@ -240,7 +240,9 @@ describe UploadsController do
       upload = upload_file("logo.png")
       get "/uploads/#{site}/#{upload.sha1}.#{upload.extension}"
       expect(response.status).to eq(200)
-      expect(response.headers["Content-Disposition"]).to eq("attachment; filename=\"logo.png\"")
+
+      # rails 6 adds UTF-8 filename to disposition
+      expect(response.headers["Content-Disposition"]).to include("attachment; filename=\"logo.png\"")
     end
 
     it "handles image without extension" do
@@ -249,7 +251,7 @@ describe UploadsController do
 
       get "/uploads/#{site}/#{upload.sha1}.json"
       expect(response.status).to eq(200)
-      expect(response.headers["Content-Disposition"]).to eq("attachment; filename=\"image_no_extension.png\"")
+      expect(response.headers["Content-Disposition"]).to include("attachment; filename=\"image_no_extension.png\"")
     end
 
     it "handles file without extension" do
@@ -258,7 +260,7 @@ describe UploadsController do
 
       get "/uploads/#{site}/#{upload.sha1}.json"
       expect(response.status).to eq(200)
-      expect(response.headers["Content-Disposition"]).to eq("attachment; filename=\"not_an_image\"")
+      expect(response.headers["Content-Disposition"]).to include("attachment; filename=\"not_an_image\"")
     end
 
     context "prevent anons from downloading files" do

spec/serializers/current_user_serializer_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe CurrentUserSerializer do
     end
 
     it "should include correct id in top_category_ids array" do
-      category = Category.first
+      _category = Category.first
       CategoryUser.create!(user_id: user.id,
                            category_id: category1.id,
                            notification_level: CategoryUser.notification_levels[:tracking])