ActiveModel::Serializers::Xml should extend ActiveModel::Naming #7874

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@Omer
Omer commented Oct 8, 2012

Assuming a native Ruby class named Foo which includes only the following module ActiveModel::Serializers::Xml. If we call #to_xml on an instance of Foo we would get the following exception NoMethodError (undefined method 'model_name'...

One way to solve this issue would be to also make sure Foo also extend ActiveModel::Naming. Another way, which appear to be more robust, and more importantly, consistent with the rest of the codebase (i.e. already used in ActiveModel::Serializers::JSON) would be extend ActiveModel::Naming the moment ActiveModel::Serializers::Xml is included.

@jaggederest
Contributor

Can you add a test for this? looks like a good patch otherwise.

@frodsan
Contributor
frodsan commented Oct 26, 2012

Indeed. This is failing in master.

[16] pry(main)> class User2
[16] pry(main)*   include ActiveModel::Serializers::Xml
[16] pry(main)*   attr_accessor :name
[16] pry(main)*   
[16] pry(main)*   def attributes
[16] pry(main)*     instance_values
[16] pry(main)*   end  
[16] pry(main)* end  
=> nil
[17] pry(main)> user2 = User2.new
=> #<User2:0x007fccdaab6db0>
[18] pry(main)> user2.name = 'Francesco'
=> "Francesco"
[19] pry(main)> user2.to_xml
NoMethodError: undefined method `model_name' for User2:Class
from /Users/frodsan/Code/rails/activemodel/lib/active_model/serializers/xml.rb:78:in `serialize'

[6] pry(main)> class User
[6] pry(main)*   include ActiveModel::Serializers::JSON
[6] pry(main)*   attr_accessor :name
[6] pry(main)*   
[6] pry(main)*   def attributes
[6] pry(main)*     instance_values
[6] pry(main)*   end  
[6] pry(main)* end  
=> nil
[7] pry(main)> user = User.new
=> #<User:0x007fccdab83f90>
[8] pry(main)> user.name = 'Francesco'
=> "Francesco"
[9] pry(main)> user.to_json
=> "{\"name\":\"Francesco\"}"

@Omer please, add a test for this.

@frodsan
Contributor
frodsan commented Oct 27, 2012

This was done in 94f6b3d.

@jeremy jeremy closed this Oct 27, 2012
@Omer
Omer commented Oct 27, 2012

Sorry it took me so long to reply. The important thing is that a fix has been merged.

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