search

mrrooijen / mongoid-paperclip

Mongoid::Paperclip enables you to use Paperclip with the Mongoid ODM for MongoDB.
MIT License
344 stars 119 forks source link

Fix destructive modification of hook params #73

Closed tzar closed 2 years ago

tzar commented 7 years ago

Currently, the after_commit compatibility layer does not pass through options. This means that options like conditionals are not passed through, which breaks things in strange ways.

For a concrete example - devise confirmable preferences after_commit and relies on :if conditions for sending confirmation email. So a user model with an avatar will fail without this fix.

Soulou commented 7 years ago

Thanks for the patch, devise confirmation was totally broken with mongoid-paperclip, sending confirmations at each modification of the user.

tilsammans commented 2 years ago

Thank you @tzar not in the least for having patience.

I plan to release this change in an upcoming major version.

jarthod commented 2 years ago

This fix appears to be breaking for me when I call destroy_all on my model, I get

undefined method `after_destroy' for {:on=>:destroy}:Hash

The backtrace is not helpful unfortunately because it's in an AR callback (~/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/activesupport-6.1.4.4/lib/active_support/callbacks.rb:427:inblock in make_lambda'`)

I didn't dive too much into why but I verified there's no error before this commit c4364b5537ef1a74e2bcf05f9c51ea698c6c4adc, only after.

Edit: After further look, do we still even need this after_commit method? considering paperclip is already handling this case by calling after_destroy if after_commit is not defined: https://github.com/kreeti/kt-paperclip/blob/master/lib/paperclip/has_attached_file.rb#L94-L100?

It was added in 2014 without much explanation: #47, I suppose back then paperclip was not doing the conditional (#46).

tilsammans commented 2 years ago

It appears you're right, but I'm not too familiar with this repo and with no tests, it's going to be a bumpy ride for a while.

I will revert this PR and then probably release 0.1.0 with the changed paperclip dependency only.

We can discuss this fix further and schedule a 1.0.0 release once everyone's confident the code is OK. Tests and CI would be the very next thing on my wishlist.

jarthod commented 2 years ago

@tilsammans agreed, I am also not familiar enough and my usage is very limited so I don't feel confident submitting a PR to remove this method. Reverting this feels like the safest move, and then let's wait for the opinion of somebody who actually encountered this option passing problem (I did not).