Never pass `nil` to `order`

This is part of a refactoring to make it easier to allow `order` to use
sanitize like just about everything else on relation. The deleted test
doesn't give any reasoning as to why passing `nil` to `order` needs to
be supported, and it's rather nonsensical. I can almost see allowing an
empty string being passed (though I'm tempted to just disallow it...)

5 files changed, 7 insertions, 22 deletions

diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index c43f13f3c4..e11a5cfb8a 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -134,7 +134,10 @@ module ActiveRecord
           else
             scope.joins!(reflection_scope.joins_values)
           end
-          scope.order! preload_values[:order] || values[:order]
+
+          if order_values = preload_values[:order] || values[:order]
+            scope.order!(order_values)
+          end
 
           if preload_values[:reordering] || values[:reordering]
             scope.reordering_value = true
diff --git a/activerecord/lib/active_record/associations/preloader/collection_association.rb b/activerecord/lib/active_record/associations/preloader/collection_association.rb
index 5adffcd831..9939280fa4 100644
--- a/activerecord/lib/active_record/associations/preloader/collection_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/collection_association.rb
@@ -2,13 +2,8 @@ module ActiveRecord
   module Associations
     class Preloader
       class CollectionAssociation < Association #:nodoc:
-
         private
 
-        def build_scope
-          super.order(preload_scope.values[:order] || reflection_scope.values[:order])
-        end
-
         def preload(preloader)
           associated_records_by_owner(preloader).each do |owner, records|
             association = owner.association(reflection.name)
@@ -17,7 +12,6 @@ module ActiveRecord
             records.each { |record| association.set_inverse_instance(record) }
           end
         end
-
       end
     end
   end
diff --git a/activerecord/lib/active_record/associations/preloader/has_one.rb b/activerecord/lib/active_record/associations/preloader/has_one.rb
index 24728e9f01..c4add621ca 100644
--- a/activerecord/lib/active_record/associations/preloader/has_one.rb
+++ b/activerecord/lib/active_record/associations/preloader/has_one.rb
@@ -2,7 +2,6 @@ module ActiveRecord
   module Associations
     class Preloader
       class HasOne < SingularAssociation #:nodoc:
-
         def association_key_name
           reflection.foreign_key
         end
@@ -10,13 +9,6 @@ module ActiveRecord
         def owner_key_name
           reflection.active_record_primary_key
         end
-
-        private
-
-        def build_scope
-          super.order(preload_scope.values[:order] || reflection_scope.values[:order])
-        end
-
       end
     end
   end
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index 56aa23b173..24aa47bb51 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -84,7 +84,9 @@ module ActiveRecord
             end
 
             scope.references! reflection_scope.values[:references]
-            scope = scope.order reflection_scope.values[:order] if scope.eager_loading?
+            if scope.eager_loading? && order_values = reflection_scope.values[:order]
+              scope = scope.order(order_values)
+            end
           end
 
           scope
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index bcc8455d0d..0c09713971 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -1221,12 +1221,6 @@ class EagerAssociationTest < ActiveRecord::TestCase
     end
   end
 
-  def test_join_eager_with_nil_order_should_generate_valid_sql
-    assert_nothing_raised(ActiveRecord::StatementInvalid) do
-      Post.includes(:comments).order(nil).where(:comments => {:body => "Thank you for the welcome"}).first
-    end
-  end
-
   def test_deep_including_through_habtm
     # warm up habtm cache
     posts = Post.all.merge!(:includes => {:categories => :categorizations}, :order => "posts.id").to_a