Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Carrierwave Entity#serializable_hash doesn't take a parameter #311

Closed
lhm opened this issue Jan 11, 2013 · 4 comments
Closed

Carrierwave Entity#serializable_hash doesn't take a parameter #311

lhm opened this issue Jan 11, 2013 · 4 comments

Comments

@lhm
Copy link

lhm commented Jan 11, 2013

I just upgraded grape from 0.2.1 to 0.2.5, and now I'm getting errors from the carrierwave gem:

ArgumentError (wrong number of arguments (1 for 0)):
  carrierwave (0.8.0) lib/carrierwave/uploader/serialization.rb:11:in `serializable_hash'
  grape (0.2.5) lib/grape/entity.rb:320:in `block in serializable_hash'
  grape (0.2.5) lib/grape/entity.rb:315:in `each'
  grape (0.2.5) lib/grape/entity.rb:315:in `inject'
  grape (0.2.5) lib/grape/entity.rb:315:in `serializable_hash'

I believe that the problem is that carrierwave also implements #serializable_hash which gets included in the models that use it for image uploading. This causes this line in entity.rb to return true:

https://github.com/intridea/grape/blob/master/lib/grape/entity.rb#L319

and then calls carrierwave's #serializable_hash with an argument - which it doesn't accept:

https://github.com/jnicklas/carrierwave/blob/master/lib/carrierwave/uploader/serialization.rb#L11

I'm not entirely sure who's at fault here. If grape expects to call it's own serializable_hash method (i.e. only Entities, or Models that mixin the DSL) than it would probably be better to check for the class instead of duck-typing.

On the other hand, if it's just following rails' serializable_hash protocol, then carrierwave should probably implement the correct method signature, I think.

FWIW, the pull request that brought this line in is #181

@dblock
Copy link
Member

dblock commented Jan 11, 2013

So the real question is whether the "correct" signature for serializable_hash is with or without a parameter. What do you think it should be?

@dblock
Copy link
Member

dblock commented Jan 11, 2013

ActiveModel takes a parameter, http://api.rubyonrails.org/classes/ActiveModel/Serialization.html#method-i-serializable_hash - I think in Grape we do the right thing. Want to open an issue with Carrierwave?

You can always monkey-patch this in your model. Post a working patch here, please.

@dblock
Copy link
Member

dblock commented Jan 12, 2013

I am going to reopen this. I am not convinced Grape's recursive serializable_hash implementation is right. The ActiveModel's implementation only checks whether it's working with an Enumerable within the recursion.

@dblock dblock reopened this Jan 12, 2013
@dblock
Copy link
Member

dblock commented Jul 7, 2013

Closed via carrierwaveuploader/carrierwave#952 in Carrierwave, fix committed in carrierwaveuploader/carrierwave@afdda81

@dblock dblock closed this as completed Jul 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants