Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Use instance_accessor: false instead of instance_writer. #7000

Merged
merged 1 commit into from

4 participants

@kennyj
Collaborator

stored_attributes and serialized_attributes are changed to class method only.

Please see #6997 comments.
cc/ @carlosantoniodasilva @josevalim

guides/source/upgrading_ruby_on_rails.textile
@@ -42,6 +42,8 @@ h4(#active_record4_0). Active Record
The <tt>delete</tt> method in collection associations can now receive <tt>Fixnum</tt> or <tt>String</tt> arguments as record ids, besides records, pretty much like the <tt>destroy</tt> method does. Previously it raised <tt>ActiveRecord::AssociationTypeMismatch</tt> for such arguments. From Rails 4.0 on <tt>delete</tt> automatically tries to find the records matching the given ids before deleting them.
+Rails 4.0 changed <tt>stored_attributes</tt> and <tt>seriazlied_attributes</tt> to class methods only. Now you can't use instance methods, you must change them, e.g. <tt>self.stored_attributes</tt> to <tt>self.class.store_attributes</tt>.

There's a typo on serialized_attributes. Also, can you change the example from stored to serialized? I think that stored_attributes doesn't exist on 3-2, they were added to master only, so they're not the big deal (perhaps we could even remove the stored_attributes mention here?).

@kennyj Collaborator
kennyj added a note

I updated it !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kennyj
Collaborator

I updated it again (I removed the stored_attributes mention).
Please review it :)

@kennyj
Collaborator

I rebased.

@rafaelfranca

@kennyj needs another rebase.

@kennyj
Collaborator

thanks @rafaelfranca ! I rebased again.

@rafaelfranca

Since we are going to remove the instance accessors should not we deprecate they?

cc/ @josevalim

@rafaelfranca

@kennyj I prefer to not remove anything without deprecation. I agree with the removal but lets add deprecation warnings to 4.0 and remove only in 4.1.

WDYT?

@kennyj
Collaborator

@rafaelfranca I agree with you, but I think we confuse a little bit how to use instance_writer / instance_accessor.

$ find . -name "*.rb" -print | xargs grep instance_writer
./lib/active_record/readonly_attributes.rb:      class_attribute :_attr_readonly, instance_writer: false
./lib/active_record/timestamp.rb:      config_attribute :record_timestamps, instance_writer: true
./lib/active_record/attribute_methods/serialization.rb:        class_attribute :serialized_attributes, instance_writer: false
./lib/active_record/scoping/default.rb:        class_attribute :default_scopes, instance_writer: false
./lib/active_record/store.rb:      class_attribute :stored_attributes, instance_writer: false
./lib/active_record/model.rb:        options[:instance_writer] ||= false

I guess we also should fix this issue about readonly_attributes...

@rafaelfranca

Yes. I think so.

@carlosantoniodasilva

Hey guys, sorry to not get back to this before. Yeah, with better thinking I agree that we should add a deprecation to the instance readers, because they may be used by someone somewhere, and we could easily break it. A deprecation would make the upgrade path easier, so I think it's ok to deprecate on 4.0 and kill them on 4.1.

@kennyj
Collaborator

@rafaelfranca @carlosantoniodasilva I updated this PR. Please review it :-)

@rafaelfranca

Seems good. :+1:

@kennyj
Collaborator

I rebased this, because CHANGELOG was conflicted.

@rafaelfranca rafaelfranca merged commit ace0692 into rails:master
@carlosantoniodasilva

Great, thanks @kennyj! :)

@tenderlove
Owner

Why are we deprecating this? I'm thinking about treating hstore columns as serialized columns, and determining this at the instance level would be much nicer. /cc @rafaelfranca

@tenderlove feel free to revert, @kennyj changed the code because he thought we don't need it at the instance level for the serialized columns and I agree with him in that case because they don't differ between instances.

But in the case that we will use it for hstore I think make sense we have the instance accessor.

OK, thanks. If I end up needing this method, I'll revert it. I just wanted to make sure I wasn't missing something!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 activerecord/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* `serialized_attributes` and `_attr_readonly` become class method only. Instance reader methods are deprecated.
+
+ *kennyj*
+
* Round usec when comparing timestamp attributes in the dirty tracking.
Fixes #6975.
View
9 activerecord/lib/active_record/attribute_methods/serialization.rb
@@ -6,7 +6,7 @@ module Serialization
included do
# Returns a hash of all the attributes that have been specified for serialization as
# keys and their class restriction as values.
- class_attribute :serialized_attributes, instance_writer: false
+ class_attribute :serialized_attributes, instance_accessor: false
self.serialized_attributes = {}
end
@@ -41,6 +41,11 @@ def serialize(attr_name, class_name = Object)
end
end
+ def serialized_attributes
+ ActiveSupport::Deprecation.warn("Instance level serialized_attributes method is deprecated, please use class level method.")
+ defined?(@serialized_attributes) ? @serialized_attributes : self.class.serialized_attributes
+ end
+
class Type # :nodoc:
def initialize(column)
@column = column
@@ -114,7 +119,7 @@ def type_cast_attribute_for_write(column, value)
end
def read_attribute_before_type_cast(attr_name)
- if serialized_attributes.include?(attr_name)
+ if self.class.serialized_attributes.include?(attr_name)
super.unserialized_value
else
super
View
7 activerecord/lib/active_record/readonly_attributes.rb
@@ -4,7 +4,7 @@ module ReadonlyAttributes
extend ActiveSupport::Concern
included do
- class_attribute :_attr_readonly, instance_writer: false
+ class_attribute :_attr_readonly, instance_accessor: false
self._attr_readonly = []
end
@@ -20,5 +20,10 @@ def readonly_attributes
self._attr_readonly
end
end
+
+ def _attr_readonly
+ ActiveSupport::Deprecation.warn("Instance level _attr_readonly method is deprecated, please use class level method.")
+ defined?(@_attr_readonly) ? @_attr_readonly : self.class._attr_readonly
+ end
end
end
View
2  activerecord/lib/active_record/store.rb
@@ -41,7 +41,7 @@ module Store
extend ActiveSupport::Concern
included do
- class_attribute :stored_attributes, instance_writer: false
+ class_attribute :stored_attributes, instance_accessor: false
self.stored_attributes = {}
end
View
6 activerecord/test/cases/base_test.rb
@@ -604,6 +604,12 @@ def test_readonly_attributes
assert_equal "changed", post.body
end
+ def test_attr_readonly_is_class_level_setting
+ post = ReadonlyTitlePost.new
+ assert_raise(NoMethodError) { post._attr_readonly = [:title] }
+ assert_deprecated { post._attr_readonly }
+ end
+
def test_non_valid_identifier_column_name
weird = Weird.create('a$b' => 'value')
weird.reload
View
7 activerecord/test/cases/serialization_test.rb
@@ -53,9 +53,8 @@ def test_serialize_should_allow_attribute_except_filtering
end
def test_serialized_attributes_are_class_level_settings
- assert_raise NoMethodError do
- topic = Topic.new
- topic.serialized_attributes = []
- end
+ topic = Topic.new
+ assert_raise(NoMethodError) { topic.serialized_attributes = [] }
+ assert_deprecated { topic.serialized_attributes }
end
end
View
5 activerecord/test/cases/store_test.rb
@@ -122,9 +122,8 @@ class StoreTest < ActiveRecord::TestCase
end
test "stores_attributes are class level settings" do
- assert_raise NoMethodError do
- @john.stored_attributes = {}
- end
+ assert_raise(NoMethodError) { @john.stored_attributes = Hash.new }
+ assert_raise(NoMethodError) { @john.stored_attributes }
end
end
View
2  guides/source/upgrading_ruby_on_rails.textile
@@ -44,6 +44,8 @@ The <tt>delete</tt> method in collection associations can now receive <tt>Fixnum
Rails 4.0 has changed how orders get stacked in +ActiveRecord::Relation+. In previous versions of rails new order was applied after previous defined order. But this is no long true. Check "ActiveRecord Query guide":active_record_querying.html#ordering for more information.
+Rails 4.0 has changed <tt>serialized_attributes</tt> and <tt>_attr_readonly</tt> to class methods only. Now you shouldn't use instance methods, it's deprecated. You must change them, e.g. <tt>self.serialized_attributes</tt> to <tt>self.class.serialized_attributes</tt>.
+
h4(#active_model4_0). Active Model
Rails 4.0 has changed how errors attach with the <tt>ActiveModel::Validations::ConfirmationValidator</tt>. Now when confirmation validations fail the error will be attached to <tt>:#{attribute}_confirmation</tt> instead of <tt>attribute</tt>.
Something went wrong with that request. Please try again.