Merge remote-tracking branch 'upstream/master' into desc_tracker

3 files changed, 66 insertions, 10 deletions

diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb
index 2a99450a3d..be55581c66 100644
--- a/activemodel/lib/active_model/attribute_methods.rb
+++ b/activemodel/lib/active_model/attribute_methods.rb
@@ -106,11 +106,14 @@ module ActiveModel
         if block_given?
           sing.send :define_method, name, &block
         else
-          # use eval instead of a block to work around a memory leak in dev
-          # mode in fcgi
-          sing.class_eval <<-eorb, __FILE__, __LINE__ + 1
-            def #{name}; #{value.nil? ? 'nil' : value.to_s.inspect}; end
-          eorb
+          if name =~ /^[a-zA-Z_]\w*[!?=]?$/
+            sing.class_eval <<-eorb, __FILE__, __LINE__ + 1
+                def #{name}; #{value.nil? ? 'nil' : value.to_s.inspect}; end
+            eorb
+          else
+            value = value.to_s if value
+            sing.send(:define_method, name) { value }
+          end
         end
       end
 
diff --git a/activemodel/lib/active_model/lint.rb b/activemodel/lib/active_model/lint.rb
index 957d1b9d70..b71ef4b22e 100644
--- a/activemodel/lib/active_model/lint.rb
+++ b/activemodel/lib/active_model/lint.rb
@@ -23,7 +23,7 @@ module ActiveModel
       def test_to_key
         assert model.respond_to?(:to_key), "The model should respond to to_key"
         def model.persisted?() false end
-        assert model.to_key.nil?
+        assert model.to_key.nil?, "to_key should return nil when `persisted?` returns false"
       end
 
       # == Responds to <tt>to_param</tt>
@@ -40,7 +40,7 @@ module ActiveModel
         assert model.respond_to?(:to_param), "The model should respond to to_param"
         def model.to_key() [1] end
         def model.persisted?() false end
-        assert model.to_param.nil?
+        assert model.to_param.nil?, "to_param should return nil when `persisted?` returns false"
       end
 
       # == Responds to <tt>valid?</tt>
diff --git a/activemodel/test/cases/attribute_methods_test.rb b/activemodel/test/cases/attribute_methods_test.rb
index b001adb35a..022c6716bd 100644
--- a/activemodel/test/cases/attribute_methods_test.rb
+++ b/activemodel/test/cases/attribute_methods_test.rb
@@ -5,6 +5,12 @@ class ModelWithAttributes
 
   attribute_method_suffix ''
 
+  class << self
+    define_method(:bar) do
+      'original bar'
+    end
+  end
+
   def attributes
     { :foo => 'value of foo' }
   end
@@ -36,6 +42,27 @@ private
   end
 end
 
+class ModelWithWeirdNamesAttributes
+  include ActiveModel::AttributeMethods
+
+  attribute_method_suffix ''
+
+  class << self
+    define_method(:'c?d') do
+      'original c?d'
+    end
+  end
+
+  def attributes
+    { :'a?b' => 'value of a?b' }
+  end
+
+private
+  def attribute(name)
+    attributes[name.to_sym]
+  end
+end
+
 class AttributeMethodsTest < ActiveModel::TestCase
   test 'unrelated classes should not share attribute method matchers' do
     assert_not_equal ModelWithAttributes.send(:attribute_method_matchers),
@@ -49,6 +76,14 @@ class AttributeMethodsTest < ActiveModel::TestCase
     assert_equal "value of foo", ModelWithAttributes.new.foo
   end
 
+  test '#define_attribute_method generates attribute method with invalid identifier characters' do
+    ModelWithWeirdNamesAttributes.define_attribute_method(:'a?b')
+    ModelWithWeirdNamesAttributes.define_attribute_method(:'a?b')
+
+    assert_respond_to ModelWithWeirdNamesAttributes.new, :'a?b'
+    assert_equal "value of a?b", ModelWithWeirdNamesAttributes.new.send('a?b')
+  end
+
   test '#define_attribute_methods generates attribute methods' do
     ModelWithAttributes.define_attribute_methods([:foo])
 
@@ -58,15 +93,33 @@ class AttributeMethodsTest < ActiveModel::TestCase
 
   test '#define_attribute_methods generates attribute methods with spaces in their names' do
     ModelWithAttributesWithSpaces.define_attribute_methods([:'foo bar'])
-  
+
     assert_respond_to ModelWithAttributesWithSpaces.new, :'foo bar'
     assert_equal "value of foo bar", ModelWithAttributesWithSpaces.new.send(:'foo bar')
   end
-  
+
+  test '#define_attr_method generates attribute method' do
+    ModelWithAttributes.define_attr_method(:bar, 'bar')
+
+    assert_respond_to ModelWithAttributes, :bar
+    assert_equal "original bar", ModelWithAttributes.original_bar
+    assert_equal "bar", ModelWithAttributes.bar
+    ModelWithAttributes.define_attr_method(:bar)
+    assert !ModelWithAttributes.bar
+  end
+
+  test '#define_attr_method generates attribute method with invalid identifier characters' do
+    ModelWithWeirdNamesAttributes.define_attr_method(:'c?d', 'c?d')
+
+    assert_respond_to ModelWithWeirdNamesAttributes, :'c?d'
+    assert_equal "original c?d", ModelWithWeirdNamesAttributes.send('original_c?d')
+    assert_equal "c?d", ModelWithWeirdNamesAttributes.send('c?d')
+  end
+
   test '#alias_attribute works with attributes with spaces in their names' do
     ModelWithAttributesWithSpaces.define_attribute_methods([:'foo bar'])
     ModelWithAttributesWithSpaces.alias_attribute(:'foo_bar', :'foo bar')
-  
+
     assert_equal "value of foo bar", ModelWithAttributesWithSpaces.new.foo_bar
   end