Permalink
Browse files

Adding methods with a generic name to a core Ruby class (and therefor…

…e all instances of that class) is not a good idea.

Crack was defining [1] `attributes` accessor methods on the core Ruby `String` class which in turn caused a very difficult to track down bug when using Mongoid, because of this [2]. This commit changes Crack so it just defines the `attributes` accessor methods on the relevant instances of `String`.

[1] https://github.com/jnunemaker/crack/blob/master/lib/crack/xml.rb#L85
[2] https://github.com/mongoid/mongoid/blob/master/lib/mongoid/relations/builder.rb#L38
  • Loading branch information...
1 parent b0c63cc commit da2fc970fc55e90622283331f475af07d3a1fdb8 @floehopper floehopper committed Mar 3, 2011
Showing with 16 additions and 2 deletions.
  1. +6 −2 lib/crack/xml.rb
  2. +10 −0 test/xml_test.rb
View
8 lib/crack/xml.rb
@@ -82,8 +82,12 @@ class << f
if @text
t = typecast_value( unnormalize_xml_entities( inner_html ) )
- t.class.send(:attr_accessor, :attributes)
- t.attributes = attributes
+ if t.is_a?(String)
+ class << t
+ attr_accessor :attributes
+ end
+ t.attributes = attributes
+ end
return { name => t }
else
#change repeating groups into an array
View
10 test/xml_test.rb
@@ -98,6 +98,16 @@ class XmlTest < Test::Unit::TestCase
should "default attributes to empty hash if not present" do
@data['opt']['user'][1].attributes.should == {}
end
+
+ should "add 'attributes' accessor methods to parsed instances of String" do
+ @data['opt']['user'][0].respond_to?(:attributes).should be(true)
+ @data['opt']['user'][0].respond_to?(:attributes=).should be(true)
+ end
+
+ should "not add 'attributes' accessor methods to all instances of String" do
+ "some-string".respond_to?(:attributes).should be(false)
+ "some-string".respond_to?(:attributes=).should be(false)
+ end
end
should "should typecast an integer" do

0 comments on commit da2fc97

Please sign in to comment.