Skip to content

Conversation

mshibuya
Copy link
Member

This PR aims to make carrierwave-mongoid to work with CarrierWave 3.x, while keeping the support for older CarrierWave versions. Changes are separeted into several commits, to describe the intention better.

- Add spec/tmp to .gitignore
- Fix warning 'Invalid update document provided'
- Set files before testing behavior of #remove_uploaders= and #remove_uploader
- Make sure to reset value of #remove_previously_stored_files_after_update
As CarrierWave 3.0 introduced filename deduplication, it will prevent mistakenly removing the newly-uploaded file in a different way.
include CarrierWave3
else
include CarrierWavePre3
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file can be better understood by using ?w=1.

What I did was introducing new modules CarrierWave3 and CarrierWavePre3, and conditionally include one of them based on the CarrierWave version.

end
end
}
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carrierwave-mongoid was heavily affected by the issue in carrierwave: carrierwaveuploader/carrierwave#2770. The issue is hard to investigate due to its nature, and affected many carrierwave versions (from 3.0.0 to 3.1.0). Hence I've added a monkey patch here, to ensure carrierwave-mongoid works with flawed carrierwave versions.

@mshibuya mshibuya marked this pull request as ready for review January 13, 2025 05:23
@mshibuya mshibuya requested a review from rmm5t January 13, 2025 05:23
@mshibuya
Copy link
Member Author

@rmm5t Hi! Any chance you can take a look at this? 🙏

@trliner
Copy link
Contributor

trliner commented Apr 23, 2025

I'd love to see this PR get merged in as the ActiveModel::Dirty changes in CarrierWave 3 solve an issue I'm having with embedded document change tracking after upgrading to mongoid 8.0.

@rmm5t
Copy link
Member

rmm5t commented Apr 23, 2025

I don't have any current projects using carrierwave or mongoind at the moment, but given taht the tests are passing, I'm happy to merge here.

Worst case, you can reference branch: master in your Gemfile until a new release can get published.

@rmm5t rmm5t merged commit 0f95a6c into master Apr 23, 2025
74 checks passed
@trliner
Copy link
Contributor

trliner commented Apr 23, 2025

Thank you @rmm5t and @mshibuya!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants