Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Updated DirtyModel's @changed_attributes hash to be symbol/string agnostic #8791

Merged
merged 1 commit into from Oct 3, 2013

Conversation

Projects
None yet
7 participants
Contributor

griffinmyers commented Jan 7, 2013

DirtyModel uses a hash to keep track of any changes made to attributes
of an instance. When using the attribute_will_change! method, you must
supply is a string and not a symbol or the *_changed? method will break
(because it is looking for the attribute name as a string in the keys
of the underlying hash). To remedy this, I simply made the underlying
hash a HashWithIndifferentAccess so it won't matter if you supply
the attribute name as a symbol or string to attribute_will_change!.

@guilleiguaran guilleiguaran commented on an outdated diff Jan 7, 2013

activemodel/CHANGELOG.md
@@ -120,4 +120,6 @@
* When `^` or `$` are used in the regular expression provided to `validates_format_of` and the :multiline option is not set to true, an exception will be raised. This is to prevent security vulnerabilities when using `validates_format_of`. The problem is described in detail in the Rails security guide *Jan Berdajs + Egor Homakov*
+* Updated the DirtyModel *_changed? method to be indifferent between using symbols and strings as keys *William Myers*
@guilleiguaran

guilleiguaran Jan 7, 2013

Owner

Changelog entry should be in top of file

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jan 7, 2013

activemodel/lib/active_model/dirty.rb
@@ -139,7 +139,7 @@ def previous_changes
# person.name = 'robert'
# person.changed_attributes # => {"name" => "bob"}
def changed_attributes
- @changed_attributes ||= {}
+ @changed_attributes ||= HashWithIndifferentAccess.new
@carlosantoniodasilva

carlosantoniodasilva Jan 7, 2013

Owner

Please namespace with ActiveSupport::

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jan 7, 2013

activemodel/test/cases/dirty_test.rb
@@ -114,4 +125,11 @@ def save
assert_equal ["Otto", "Mr. Manfredgensonton"], @model.name_change
assert_equal @model.name_was, "Otto"
end
+
+ test "using attribute_will_change with a symbol will work" do
+ @model.size = 1
+ assert @model.size_changed?
+ end
+
+
@carlosantoniodasilva

carlosantoniodasilva Jan 7, 2013

Owner

Please remove the two extra lines.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Jan 7, 2013

activemodel/test/cases/dirty_test.rb
@@ -114,4 +125,11 @@ def save
assert_equal ["Otto", "Mr. Manfredgensonton"], @model.name_change
assert_equal @model.name_was, "Otto"
end
+
+ test "using attribute_will_change with a symbol will work" do
@carlosantoniodasilva

carlosantoniodasilva Jan 7, 2013

Owner

using attribute_will_change with a symbol should be enough?

mshappe commented Feb 8, 2013

👍

Author appears to have made the requested changes, and the change itself seems reasonable. I'm not sure I can think of any reason why you wouldn't want to allow the use of symbols as well as strings here.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 8, 2013

activemodel/CHANGELOG.md
@@ -120,4 +125,5 @@
* When `^` or `$` are used in the regular expression provided to `validates_format_of` and the :multiline option is not set to true, an exception will be raised. This is to prevent security vulnerabilities when using `validates_format_of`. The problem is described in detail in the Rails security guide *Jan Berdajs + Egor Homakov*
+
@carlosantoniodasilva

carlosantoniodasilva Feb 8, 2013

Owner

Please revert this change.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 8, 2013

activemodel/test/cases/dirty_test.rb
@@ -28,6 +29,16 @@ def color=(val)
@color = val
end
+ def size
+ @size
+ end
+
+ def size=(val)
+ attribute = :size
+ attribute_will_change!(attribute) unless val == @size
@carlosantoniodasilva

carlosantoniodasilva Feb 8, 2013

Owner

I don't think you need the attribute variable, passing :size here will suffice.

Seems good, it just need a minor change and be squashed. Thanks.

Member

neerajdotname commented May 25, 2013

@griffinmyers can you squash the three commits into one. Thanks.

DirtyModel uses a hash to keep track of any changes made to attributes
of an instance. When using the attribute_will_change! method, you must
supply a string and not a symbol or the *_changed? method will break
(because it is looking for the attribute name as a string in the keys
of the underlying hash). To remedy this, I simply made the underlying
hash a HashWithIndifferentAccess so it won't matter if you supply
the attribute name as a symbol or string to attribute_will_change!.
Contributor

griffinmyers commented May 28, 2013

Sorry about that, didn't realize I was supposed to do that.

@carlosantoniodasilva Just joined Code Triage and it pointed me to this aging issue. Looks like @griffinmyers made the requested code changes and combined the commits. Sorry to be a total noob, but what is the next step to get this merged into Rails?

rafaelfranca added a commit that referenced this pull request Oct 3, 2013

Merge pull request #8791 from griffinmyers/master
Updated DirtyModel's @changed_attributes hash to be symbol/string agnostic

Conflicts:
	activemodel/CHANGELOG.md

@rafaelfranca rafaelfranca merged commit 0e65587 into rails:master Oct 3, 2013

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