Deprecation: count class method should be called with an options hash rather than two args for conditions and joins. Closes #6287.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@5192 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

activerecord/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Deprecation: count class method should be called with an options hash rather than two args for conditions and joins.  #6287 [Bob Silva]
+
 * has_one associations with a nil target may be safely marshaled.  #6279 [norbauer, Jeremy Kemper]
 
 * Duplicate the hash provided to AR::Base#to_xml to prevent unexpected side effects [Koz]

activerecord/lib/active_record/associations/has_many_association.rb

@@ -137,7 +137,7 @@ module ActiveRecord
           elsif @reflection.options[:counter_sql]
             @reflection.klass.count_by_sql(@counter_sql)
           else
-            @reflection.klass.count(@counter_sql)
+            @reflection.klass.count(:conditions => @counter_sql)
           end
           
           @target = [] and loaded if count == 0

activerecord/lib/active_record/calculations.rb

@@ -9,7 +9,7 @@ module ActiveRecord
       # Count operates using three different approaches. 
       #
       # * Count all: By not passing any parameters to count, it will return a count of all the rows for the model.
-      # * Count by conditions or joins: For backwards compatibility, you can pass in +conditions+ and +joins+ as individual parameters.
+      # * Count by conditions or joins: This API has been deprecated and will be removed in Rails 2.0
       # * Count using options will find the row count matched by the options used.
       #
       # The last approach, count using options, accepts an option hash as the only parameter. The options are:
@@ -29,7 +29,7 @@ module ActiveRecord
       # Examples for counting all:
       #   Person.count         # returns the total count of all people
       #
-      # Examples for count by +conditions+ and +joins+ (for backwards compatibility):
+      # Examples for count by +conditions+ and +joins+ (this has been deprecated):
       #   Person.count("age > 26")  # returns the number of people older than 26
       #   Person.find("age > 26 AND job.salary > 60000", "LEFT JOIN jobs on jobs.person_id = person.id") # returns the total number of rows matching the conditions and joins fetched by SELECT COUNT(*).
       #
@@ -128,24 +128,33 @@ module ActiveRecord
         def construct_count_options_from_legacy_args(*args)
           options     = {}
           column_name = :all
-          # For backwards compatibility, we need to handle both count(conditions=nil, joins=nil) or count(options={}) or count(column_name=:all, options={}).
+
-          if args.size >= 0 && args.size <= 2
+          # We need to handle
-            if args.first.is_a?(Hash)
+          #   count()
-              options     = args.first
+          #   count(options={})
+          #   count(column_name=:all, options={})
+          #   count(conditions=nil, joins=nil)      # deprecated
+          if args.size > 2
+            raise ArgumentError, "Unexpected parameters passed to count(options={}): #{args.inspect}"
+          elsif args.size > 0
+            if args[0].is_a?(Hash)
+              options = args[0]
             elsif args[1].is_a?(Hash)
-              options     = args[1]
+              column_name, options = args
-              column_name = args.first
             else
-              # Handle legacy paramter options: def count(conditions=nil, joins=nil)
+              # Deprecated count(conditions, joins=nil)
-              options.merge!(:conditions => args[0]) if args.length > 0
+              ActiveSupport::Deprecation.warn(
-              options.merge!(:joins      => args[1]) if args.length > 1
+                "You called count(#{args[0].inspect}, #{args[1].inspect}), which is a deprecated API call. Instead you should use " +
+                "count(column_name, options). Passing the conditions and joins as string parameters will be removed in Rails 2.0."
+              )
+              options.merge!(:conditions => args[0])
+              options.merge!(:joins      => args[1]) if args[1]
             end
-          else
-            raise(ArgumentError, "Unexpected parameters passed to count(*args): expected either count(conditions=nil, joins=nil) or count(options={})")
           end
+
           [column_name, options]
         end
-      
+
         def construct_calculation_sql(operation, column_name, options) #:nodoc:
           scope           = scope(:find)
           merged_includes = merge_includes(scope ? scope[:include] : [], options[:include])

activerecord/test/associations_test.rb

@@ -395,7 +395,9 @@ class HasManyAssociationsTest < Test::Unit::TestCase
   end
 
   def test_counting_with_single_conditions
-    assert_equal 2, Firm.find(:first).plain_clients.count('1=1')
+    assert_deprecated 'count' do
+      assert_equal 2, Firm.find(:first).plain_clients.count('1=1')
+    end
   end
 
   def test_counting_with_single_hash

activerecord/test/base_test.rb

@@ -1159,7 +1159,7 @@ class BasicsTest < Test::Unit::TestCase
   def test_count_with_join
     res = Post.count_by_sql "SELECT COUNT(*) FROM posts LEFT JOIN comments ON posts.id=comments.post_id WHERE posts.#{QUOTED_TYPE} = 'Post'"
     res2 = nil
-    assert_nothing_raised do
+    assert_deprecated 'count' do
       res2 = Post.count("posts.#{QUOTED_TYPE} = 'Post'",
                         "LEFT JOIN comments ON posts.id=comments.post_id")
     end

activerecord/test/calculations_test.rb

@@ -196,4 +196,16 @@ class CalculationsTest < Test::Unit::TestCase
     assert_equal 6, Account.count(:distinct => true, :include => :firm)
     assert_equal 4, Account.count(:distinct => true, :include => :firm, :select => :credit_limit)
   end
+
+  def test_deprecated_count_with_string_parameters
+    assert_deprecated('count') { Account.count('credit_limit > 50') }
+  end
+
+  def test_count_with_no_parameters_isnt_deprecated
+    assert_not_deprecated { Account.count }
+  end
+
+  def test_count_with_too_many_parameters_raises
+    assert_raise(ArgumentError) { Account.count(1, 2, 3) }
+  end
 end

activerecord/test/deprecated_finder_test.rb

@@ -88,9 +88,9 @@ class DeprecatedFinderTest < Test::Unit::TestCase
   end
 
   def test_count
-    assert_equal(0, Entrant.count("id > 3"))
+    assert_equal(0, Entrant.count(:conditions => "id > 3"))
-    assert_equal(1, Entrant.count(["id > ?", 2]))
+    assert_equal(1, Entrant.count(:conditions => ["id > ?", 2]))
-    assert_equal(2, Entrant.count(["id > ?", 1]))
+    assert_equal(2, Entrant.count(:conditions => ["id > ?", 1]))
   end
 
   def test_count_by_sql

activerecord/test/finder_test.rb

@@ -249,9 +249,9 @@ class FinderTest < Test::Unit::TestCase
   end
 
   def test_count
-    assert_equal(0, Entrant.count("id > 3"))
+    assert_equal(0, Entrant.count(:conditions => "id > 3"))
-    assert_equal(1, Entrant.count(["id > ?", 2]))
+    assert_equal(1, Entrant.count(:conditions => ["id > ?", 2]))
-    assert_equal(2, Entrant.count(["id > ?", 1]))
+    assert_equal(2, Entrant.count(:conditions => ["id > ?", 1]))
   end
 
   def test_count_by_sql

activerecord/test/method_scoping_test.rb

@@ -57,7 +57,7 @@ class MethodScopingTest < Test::Unit::TestCase
 
     Developer.with_scope(:find => { :conditions => 'salary = 100000' }) do
       assert_equal 8, Developer.count
-      assert_equal 1, Developer.count("name LIKE 'fixture_1%'")
+      assert_equal 1, Developer.count(:conditions => "name LIKE 'fixture_1%'")
     end        
   end
 
@@ -74,7 +74,7 @@ class MethodScopingTest < Test::Unit::TestCase
   def test_scoped_count_include
     # with the include, will retrieve only developers for the given project
     Developer.with_scope(:find => { :include => :projects }) do
-      assert_equal 1, Developer.count('projects.id = 2')
+      assert_equal 1, Developer.count(:conditions => 'projects.id = 2')
     end
   end