Deprecate defining scopes with a callable (lambda, proc, etc) via the scope class method. Just define a class method yourself instead.

6 files changed, 116 insertions, 23 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 6b3d408720..93eb42a52c 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,23 @@
 *Rails 3.1.0 (unreleased)*
 
+* Passing a proc (or other object that responds to #call) to scope is deprecated. If you need your
+  scope to be lazily evaluated, or takes parameters, please define it as a normal class method
+  instead. For example, change this:
+
+    class Post < ActiveRecord::Base
+      scope :unpublished, lambda { where('published_at > ?', Time.now) }
+    end
+
+  To this:
+
+    class Post < ActiveRecord::Base
+      def self.unpublished
+        where('published_at > ?', Time.now)
+      end
+    end
+
+  [Jon Leighton]
+
 * Default scopes are now evaluated at the latest possible moment, to avoid problems where
   scopes would be created which would implicitly contain the default scope, which would then
   be impossible to get rid of via Model.unscoped.
diff --git a/activerecord/lib/active_record/named_scope.rb b/activerecord/lib/active_record/named_scope.rb
index 60840e6958..f1df04950b 100644
--- a/activerecord/lib/active_record/named_scope.rb
+++ b/activerecord/lib/active_record/named_scope.rb
@@ -51,6 +51,14 @@ module ActiveRecord
       # The above calls to <tt>scope</tt> define class methods Shirt.red and Shirt.dry_clean_only. Shirt.red,
       # in effect, represents the query <tt>Shirt.where(:color => 'red')</tt>.
       #
+      # Note that this is simply 'syntactic sugar' for defining an actual class method:
+      #
+      #   class Shirt < ActiveRecord::Base
+      #     def self.red
+      #       where(:color => 'red')
+      #     end
+      #   end
+      #
       # Unlike <tt>Shirt.find(...)</tt>, however, the object returned by Shirt.red is not an Array; it
       # resembles the association object constructed by a <tt>has_many</tt> declaration. For instance,
       # you can invoke <tt>Shirt.red.first</tt>, <tt>Shirt.red.count</tt>, <tt>Shirt.red.where(:size => 'small')</tt>.
@@ -74,14 +82,34 @@ module ActiveRecord
       # then <tt>elton.shirts.red.dry_clean_only</tt> will return all of Elton's red, dry clean
       # only shirts.
       #
-      # Named \scopes can also be procedural:
+      # If you need to pass parameters to a scope, define it as a normal method:
       #
       #   class Shirt < ActiveRecord::Base
-      #     scope :colored, lambda {|color| where(:color => color) }
+      #     def self.colored(color)
+      #       where(:color => color)
+      #     end
       #   end
       #
       # In this example, <tt>Shirt.colored('puce')</tt> finds all puce shirts.
       #
+      # Note that scopes defined with \scope will be evaluated when they are defined, rather than
+      # when they are used. For example, the following would be incorrect:
+      #
+      #   class Post < ActiveRecord::Base
+      #     scope :recent, where('published_at >= ?', Time.now - 1.week)
+      #   end
+      #
+      # The example above would be 'frozen' to the <tt>Time.now</tt> value when the <tt>Post</tt>
+      # class was defined, and so the resultant SQL query would always be the same. The correct
+      # way to do this would be via a class method, which will re-evaluate the scope each time
+      # it is called:
+      #
+      #   class Post < ActiveRecord::Base
+      #     def self.recent
+      #       where('published_at >= ?', Time.now - 1.week)
+      #     end
+      #   end
+      #
       # Named \scopes can also have extensions, just as with <tt>has_many</tt> declarations:
       #
       #   class Shirt < ActiveRecord::Base
@@ -92,6 +120,18 @@ module ActiveRecord
       #     end
       #   end
       #
+      # The above could also be written as a class method like so:
+      #
+      #   class Shirt < ActiveRecord::Base
+      #     def self.red
+      #       where(:color => 'red').extending do
+      #         def dom_id
+      #           'red_shirts'
+      #         end
+      #       end
+      #     end
+      #   end
+      #
       # Scopes can also be used while creating/building a record.
       #
       #   class Article < ActiveRecord::Base
@@ -128,6 +168,24 @@ module ActiveRecord
         valid_scope_name?(name)
         extension = Module.new(&Proc.new) if block_given?
 
+        if !scope_options.is_a?(Relation) && scope_options.respond_to?(:call)
+          ActiveSupport::Deprecation.warn <<-WARN
+Passing a proc (or other object that responds to #call) to scope is deprecated. If you need your scope to be lazily evaluated, or takes parameters, please define it as a normal class method instead. For example, change this:
+
+class Post < ActiveRecord::Base
+  scope :unpublished, lambda { where('published_at > ?', Time.now) }
+end
+
+To this:
+
+class Post < ActiveRecord::Base
+  def self.unpublished
+    where('published_at > ?', Time.now)
+  end
+end
+          WARN
+        end
+
         scope_proc = lambda do |*args|
           options = scope_options.respond_to?(:call) ? scope_options.call(*args) : scope_options
           options = scoped.apply_finder_options(options) if options.is_a?(Hash)
diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb
index 8fd1fc2577..2880fdc651 100644
--- a/activerecord/test/cases/named_scope_test.rb
+++ b/activerecord/test/cases/named_scope_test.rb
@@ -471,6 +471,12 @@ class NamedScopeTest < ActiveRecord::TestCase
       require "models/without_table"
     end
   end
+
+  def test_scopes_with_callables_are_deprecated
+    assert_deprecated do
+      Post.scope :WE_SO_EXCITED, lambda { |partyingpartyingpartying, yeah| fun!.fun!.fun! }
+    end
+  end
 end
 
 class DynamicScopeMatchTest < ActiveRecord::TestCase
diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb
index 2a4c37089a..3bd7db7834 100644
--- a/activerecord/test/models/comment.rb
+++ b/activerecord/test/models/comment.rb
@@ -1,5 +1,8 @@
 class Comment < ActiveRecord::Base
-  scope :limit_by, lambda {|l| limit(l) }
+  def self.limit_by(l)
+    limit(l)
+  end
+
   scope :containing_the_letter_e, :conditions => "comments.body LIKE '%e%'"
   scope :not_again, where("comments.body NOT LIKE '%again%'")
   scope :for_first_post, :conditions => { :post_id => 1 }
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index 632f83e5d4..7055380d85 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -7,12 +7,15 @@ class Post < ActiveRecord::Base
 
   scope :containing_the_letter_a, where("body LIKE '%a%'")
   scope :ranked_by_comments, order("comments_count DESC")
-  scope :limit_by, lambda {|l| limit(l) }
-  scope :with_authors_at_address, lambda { |address| {
-      :conditions => [ 'authors.author_address_id = ?', address.id ],
-      :joins => 'JOIN authors ON authors.id = posts.author_id'
-    }
-  }
+
+  def self.limit_by(l)
+    limit(l)
+  end
+
+  def self.with_authors_at_address(address)
+    where('authors.author_address_id = ?', address.id)
+      .joins('JOIN authors ON authors.id = posts.author_id')
+  end
 
   belongs_to :author do
     def greeting
@@ -27,9 +30,10 @@ class Post < ActiveRecord::Base
 
   scope :with_special_comments, :joins => :comments, :conditions => {:comments => {:type => 'SpecialComment'} }
   scope :with_very_special_comments, joins(:comments).where(:comments => {:type => 'VerySpecialComment'})
-  scope :with_post, lambda {|post_id|
-    { :joins => :comments, :conditions => {:comments => {:post_id => post_id} } }
-  }
+
+  def self.with_post(post_id)
+    joins(:comments).where(:comments => { :post_id => post_id })
+  end
 
   has_many   :comments do
     def find_most_recent
diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb
index 6440dbe8ab..60e750e6c4 100644
--- a/activerecord/test/models/topic.rb
+++ b/activerecord/test/models/topic.rb
@@ -1,10 +1,20 @@
 class Topic < ActiveRecord::Base
   scope :base
-  scope :written_before, lambda { |time|
-    if time
-      { :conditions => ['written_on < ?', time] }
-    end
-  }
+
+  ActiveSupport::Deprecation.silence do
+    scope :written_before, lambda { |time|
+      if time
+        { :conditions => ['written_on < ?', time] }
+      end
+    }
+
+    scope :with_object, Class.new(Struct.new(:klass)) {
+      def call
+        klass.where(:approved => true)
+      end
+    }.new(self)
+  end
+
   scope :approved, :conditions => {:approved => true}
   scope :rejected, :conditions => {:approved => false}
 
@@ -19,12 +29,6 @@ class Topic < ActiveRecord::Base
     end
   end
 
-  scope :with_object, Class.new(Struct.new(:klass)) {
-    def call
-      klass.where(:approved => true)
-    end
-  }.new(self)
-
   module NamedExtension
     def two
       2