Use instance_accessor: false instead of instance_writer. #7000

Merged
merged 1 commit into from Aug 21, 2012

4 participants

@kennyj

stored_attributes and serialized_attributes are changed to class method only.

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

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Jul 7, 2012
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>.
@carlosantoniodasilva
Ruby on Rails member

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
kennyj added a note Jul 7, 2012

I updated it !

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

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

@kennyj

I rebased.

@rafaelfranca
Ruby on Rails member

@kennyj needs another rebase.

@kennyj

thanks @rafaelfranca ! I rebased again.

@rafaelfranca
Ruby on Rails member

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

cc/ @josevalim

@rafaelfranca
Ruby on Rails member

@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

@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
Ruby on Rails member

Yes. I think so.

@carlosantoniodasilva
Ruby on Rails member

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

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

@rafaelfranca
Ruby on Rails member

Seems good. 👍

@kennyj

I rebased this, because CHANGELOG was conflicted.

@rafaelfranca rafaelfranca merged commit ace0692 into rails:master Aug 21, 2012
@carlosantoniodasilva
Ruby on Rails member

Great, thanks @kennyj! :)

@tenderlove
Ruby on Rails member

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

Ruby on Rails member

@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.

Ruby on Rails member

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