Replace mimemagic with marcel

[ghiculescu]

Resolves #53

See also rails/marcel#30, https://weblog.rubyonrails.org/2021/3/26/marcel-upgrade-releases/, carrierwaveuploader/carrierwave#2551

Known breaking changes

• Content type validators for MS office formats may need to be updated. See https://github.com/rails/marcel/blob/48644abd771ccaa25cdaad53c0ec02beb9ca73c9/lib/marcel/tables.rb for a list of supported types. For example, validates_attachment :foo, content_type: { content_type: ["application/vnd.ms-excel"] } may need to be changed to validates_attachment :foo, content_type: { content_type: ["application/vnd.ms-excel", "application/x-tika-msoffice"] }.
• Google sheets exports of office formats may not be recognized without additional configuration. Ensure you're using the latest version of marcel which includes Magic for xlsx rails/marcel#36

[pboling]

NOTE:
mimemagic may have broader support for filetypes than marcel. The 0.3.7 release (now up to 0.3.10 & 0.4.3) of mimemagic fixed our deploys with current kt-paperclip (as well as the licensing issues).

This was our commit message, which shows how simple it is to upgrade for MacOS and Linux users:

fix: Update mimemagic gem to MIT-licensed release v0.3.7

• Depend on pre-installed shared-mime-info (GPLv2+)
• Update nokogori 1.11.1 => 1.11.2 (forced)

LOCAL DEV HAS NEW DEPENDENCY

brew install shared-mime-info
bundle install

DEPLOYED ENVS HAVE NEW DEPENDENCY

apt-get install shared-mime-info
bundle install

[ghiculescu]

If this repo is sticking with mimemagic (particularly if Rails isn't), I think some instructions on how to install it and shared-mine-info would be helpful. My preference is to replace it with something that has no extra dependencies.

[EthanZeigler]

Sadly for people using containerized builds or cloudfoundry systems, adding another system dependency can be a real pain in the butt, especially for larger organizations with lots of red tape trying to maintain legacy software

[pboling]

Would it be possible / reasonable to make it user choice on which library to use? Mostly just wondering academically, as for my personal use case it doesn't much matter, but I know there are implementations that utilize the underlying mime library as a first-class dependency, and would require much work to switch, if it became necessary.

Granted it's likely not necessary for most, as both can sit side-by-side... but for a workflow on top of kt-paperclip that reaches back down to the mime library, it could theoretically be painful.

I know, super edge case... I'll disappear again. Cheers! Thanks for keeping paperclip alive! We need it :)

[ghiculescu]

I would be open to it. Out of curiosity, are you using this gem with Rails or independently? Rails just made the same transition https://weblog.rubyonrails.org/2021/3/26/marcel-upgrade-releases/

[pboling]

Our use case is a legacy Grape (not Rails) app that does load some of the Rails gems, like ActiveRecord, ActiveSupport and ActionMailer (@ v5.2).

Also sorry for ninja edits...

[ghiculescu]

Gotcha. I think it would be possible, but would rather just keep a single dependency in this PR to keep it simple. Could you please test your app against the PR as it sits now? If it causes issues I can fix them before it's merged :)

[pboling]

Yep, I will test it soon.

[EthanZeigler]

Are there any updates on getting this merged? This is the last blocker for my team to release a fix for all this.

[pboling]

@EthanZeigler Have you tested this branch in your app to see if it works? They were waiting on feedback from an integrated environment. I was planning to test it yesterday but had to manage a different hotfix.

[pboling]

@ghiculescu is the build failure concerning?

[EthanZeigler]

I'll be able to get my team to test it tomorrow.

[ghiculescu]

@ghiculescu is the build failure concerning?

It was the same failure on master, so I don't think this PR is causing it. But 5305f6d should fix it. Edit: it improved things but there's still some failures, which don't seem related to this PR.

[ghiculescu]

I've added a list of known breaking changes we came across while testing this internally. Please let me know if there's others to add to the PR + changelog.

[geoffharcourt]

@ghiculescu thanks for opening this PR and for beginning the list of potentially breaking changes. FWIW, office documents were broken using the shared-mime-info database alone (being identified as ZIP files, probably due to the underlying structure of MS Office files as zipped XML bundles), so they are likely already broken for anyone using one of the post-yank versions of mimemagic.

[berniechiu]

we're also eager to have this merged~

[ghiculescu]

Thanks @rocket-turtle - I added it to the PR body.

[Liboul]

@ghiculescu

For anyone else subscribed to this: please test this fork in your apps - at least in CI - and let me know if it works

Our CI passed. Tested in a production environment, and all 5 of our uses for Paperclip seem to keep working fine 👌

Adding the dependency to shared-mime-info is a no go for us, not only because it is an additional external dependency, environment dependent, but also because it is increasing the time to setup the project on macOS by 30 minutes. So we will be using this branch until it is merged or another solution is released. Thanks!

[natematykiewicz]

rails/marcel#36 was merged yesterday. Be on the lookout for a version bump.

[EmmaB]

Passes CI for us (not tested in production though) saving image files and .xlsx (application/vnd.openxmlformats-officedocument.spreadsheetml.sheet) files to a bucket on S3, on Rails 6.1.3.1. Many thanks for the work!

[ghiculescu]

rails/marcel#36 was merged yesterday. Be on the lookout for a version bump.

Great. @rocket-turtle wanna see if this fixes your issues?

[johnnyshields]

At TableCheck we have also used this branch for several days with no issues processing 1,000+ uploads in various formats each day.

Looks like Marcel 1.0.1 is now out. How can we get this PR merged and released?

[rocket-turtle]

@ghiculescu looks good.

[cajuncanuck]

I've also been using this in an application handling various filetypes and haven't come across any issues.

[johnnyshields]

Guys lets get this one to the finish line... 🏁

[pankajbatra]

eagerly waiting for the merge ⏳

[johnnyshields]

@ssinghi can you handle this merge? It's a critical one. I've been running this in production now for over 3 weeks.

[CaptainAwesomeDi]

we can bump this to 1.0.1 now

[marcelcarousel]

Eager to have this merged

[agieche]

Running this on production (Rails 6.1.3.1) for several weeks now without issues. Mainly handling sound files with substantial file reprocessing. Thanks for your commitment 👍

[lucasintel]

Also running this in production (on multiple Rails services, from v5.2.5 to v6.1.3.1) for over 3 weeks. I haven't come across any issues.

[johnnyshields]

@adrianmann please merge this

[adrianmann]

@johnnyshields Merge what? I don't have privileges to merge anything anyways.

[johnnyshields]

@ssinghi ?

[mark-brooks-roostify]

Will someone be merging this? This is a pretty important update.

[ghiculescu]

For anyone else subscribed to this: please test this fork in your apps - at least in CI - and let me know if it works, or if you come across any issues worth documenting in the PR body. Here's a gemfile line to test it:

gem "kt-paperclip", git: "https://github.com/ghiculescu/kt-paperclip.git", branch: "use-marcel" # pending https://github.com/kreeti/kt-paperclip/pull/54

Bumping this for anyone who missed it. We have been running this branch in production for a few months, it's 👍 .

I think the test failures are intermittent.

[edward-ellis]

We have been running with no issues since April 4, 2021. - https://buildingenergyscore.energy.gov

[geoffharcourt]

Same here, zero issues since early April 2021.

[ssinghi]

Thank you for this PR, we'll get it reviewed soon and work on it.