Fix dup with multiple mounters or not cached

carrierwaveuploader / carrierwave

Classier solution for file uploads for Rails, Sinatra and other Ruby web frameworks

https://github.com/carrierwaveuploader/carrierwave

8.78k stars 1.66k forks source link

Fix dup with multiple mounters or not cached #2645

Closed BrianHawley closed 1 year ago

BrianHawley commented 1 year ago

Don't call each if @_mounters is unitialized or nil. It will be nil for calls to later mounters, or if not referenced or saved.
Don't interpolate column when it should be a local variable.
Don't define global functions in specs. Make them local to the block.
Cover more missing scenarios.
Fix some rubocop failures.

[Fixes #1962]

mshibuya commented 1 year ago

Thanks for the PR. Where's the fix for this point?

The initialize_dup is defined in the wrong context, as it refers to the overall mounters collection for the object, but is defined in a per-mounter module.

https://github.com/carrierwaveuploader/carrierwave/issues/1962#issuecomment-1370308875

BrianHawley commented 1 year ago

@mshibuya I've used this fix in my app at work (to fix after updating carrierwave yesterday) and it solves the issues I ran into: NoMethodError on nil, fortunately caught by our test suite. I verified it there before making the PR here.

BrianHawley commented 1 year ago

If we were to solve the 'wrong context' issue, the change will look like this. What do you think?

Sure, that will work. Updated with your changes, @mshibuya.

mshibuya commented 1 year ago

Thank you!