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

Check for column existence before raising seemingly unrelated errors? #1747

Closed
olivierlacan opened this Issue Sep 25, 2015 · 3 comments

Comments

Projects
None yet
3 participants
@olivierlacan

olivierlacan commented Sep 25, 2015

Would you accept a PR that adds AR column checking to CarrierWave::ActiveRecord#mount_uploader or is that a bad idea?

This is a very obtuse error and I doubt I would have figured out I forgot to run rake db:migrate a few years back:

NoMethodError: undefined method `portrait_will_change!' for #<Member:0x007f92dc652f18>

PS: after a cursory Google check there seems to be a bunch of people wondering why AR dynamic methods are causing exceptions, so I think this might help out more than few people

@olivierlacan

This comment has been minimized.

Show comment
Hide comment
@olivierlacan

olivierlacan Sep 25, 2015

This is a really dumb/naive implementation of what I mean: olivierlacan@6c62250

That might help gauging how insane this is. Obviously calling connection this early might be an issue but I found that this only raises when an AR object is instantiated anyway.

olivierlacan commented Sep 25, 2015

This is a really dumb/naive implementation of what I mean: olivierlacan@6c62250

That might help gauging how insane this is. Obviously calling connection this early might be an issue but I found that this only raises when an AR object is instantiated anyway.

@eavgerinos

This comment has been minimized.

Show comment
Hide comment
@eavgerinos

eavgerinos Sep 28, 2015

Member

@olivierlacan Hello. Thanks for pointing this out.

I'm not quite sure about the check for column.

I have 3 considerations about this change:

  • Maybe some people use a virtual attribute for this by hacking the ActiveModel::Dirty.
  • What happens if the application does not use ActiveRecord models but mongoid? (Does the plugin for mongoid handle stuff like that?)
  • If we are about to check it, I believe that ArgumentError is the proper error to raise, and better inside the lib/carrierwave/orm/activerecord.rb which handles db layer stuff.

What do you think?

Member

eavgerinos commented Sep 28, 2015

@olivierlacan Hello. Thanks for pointing this out.

I'm not quite sure about the check for column.

I have 3 considerations about this change:

  • Maybe some people use a virtual attribute for this by hacking the ActiveModel::Dirty.
  • What happens if the application does not use ActiveRecord models but mongoid? (Does the plugin for mongoid handle stuff like that?)
  • If we are about to check it, I believe that ArgumentError is the proper error to raise, and better inside the lib/carrierwave/orm/activerecord.rb which handles db layer stuff.

What do you think?

@thomasfedb

This comment has been minimized.

Show comment
Hide comment
@thomasfedb

thomasfedb Nov 16, 2015

Contributor

@olivierlacan Please feel free to request this issue be reopened if you require.

Contributor

thomasfedb commented Nov 16, 2015

@olivierlacan Please feel free to request this issue be reopened if you require.

@thomasfedb thomasfedb closed this Nov 16, 2015

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