minimagick / minimagick

mini replacement for RMagick
https://rubydoc.info/gems/mini_magick
MIT License
2.83k stars 347 forks source link

v4.3 incompatible with carrierwave uploader #337

Closed mfkp closed 9 years ago

mfkp commented 9 years ago

Might be related to #336, but upgrading from 4.2.10 to 4.3.1 (or anything after) results in an error when trying to upload a file generated by mini_magick with carrierwave.

Stacktrace:

MiniMagick::Error: `mogrify -original-filename /var/folders/m8/q271_j211tj_2ykl7hjl48f80000gn/T/mini_magick20151011-43809-9v4qqo.pdf` failed with error:
mogrify: unrecognized option `-original-filename' @ error/mogrify.c/MogrifyImageCommand/5487.

    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/mini_magick-4.3.1/lib/mini_magick/shell.rb:18:in `run'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/mini_magick-4.3.1/lib/mini_magick/tool.rb:79:in `call'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/mini_magick-4.3.1/lib/mini_magick/tool.rb:40:in `new'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/mini_magick-4.3.1/lib/mini_magick/image.rb:497:in `mogrify'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/mini_magick-4.3.1/lib/mini_magick/image.rb:389:in `method_missing'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/sanitized_file.rb:43:in `original_filename'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/sanitized_file.rb:57:in `filename'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/sanitized_file.rb:82:in `extension'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/uploader/extension_whitelist.rb:41:in `check_whitelist!'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/uploader/callbacks.rb:16:in `block in with_callbacks'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/uploader/callbacks.rb:16:in `each'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/uploader/callbacks.rb:16:in `with_callbacks'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/uploader/cache.rb:122:in `cache!'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/mount.rb:329:in `cache'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/mount.rb:163:in `rec_pdf='
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/carrierwave-0.10.0/lib/carrierwave/orm/activerecord.rb:39:in `rec_pdf='
... 40 levels...
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activesupport-4.2.4/lib/active_support/callbacks.rb:497:in `block in around'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activesupport-4.2.4/lib/active_support/callbacks.rb:505:in `call'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activesupport-4.2.4/lib/active_support/callbacks.rb:505:in `call'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activesupport-4.2.4/lib/active_support/callbacks.rb:92:in `__run_callbacks__'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activesupport-4.2.4/lib/active_support/callbacks.rb:778:in `_run_perform_callbacks'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activesupport-4.2.4/lib/active_support/callbacks.rb:81:in `run_callbacks'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activejob-4.2.4/lib/active_job/execution.rb:31:in `perform_now'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/activejob-4.2.4/lib/active_job/execution.rb:16:in `perform_now'
    from (irb):4
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/railties-4.2.4/lib/rails/commands/console.rb:110:in `start'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/railties-4.2.4/lib/rails/commands/console.rb:9:in `start'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/railties-4.2.4/lib/rails/commands/commands_tasks.rb:68:in `console'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/railties-4.2.4/lib/rails/commands/commands_tasks.rb:39:in `run_command!'
    from /Users/kyle/.rbenv/versions/2.2.0/lib/ruby/gems/2.2.0/gems/railties-4.2.4/lib/rails/commands.rb:17:in `<top (required)>'
    from bin/rails:4:in `require'
    from bin/rails:4:in `<main>'

The only instance of "original filename" I can find is in the carrierwave uploader file:

def filename
  "#{secure_token}.#{file.extension}" if original_filename.present?
end

I'm not doing any post-processing of the file in the uploader. In fact, the include CarrierWave::MiniMagick is commented out in the uploader.

Any ideas on what could be causing this?

janko commented 9 years ago

Thanks for reporting this, I released a new version 4.3.6 with the fix.

Not long ago MiniMagick was autogenerating methods for tools, so you could get MiniMagick::Tool::Mogrify.instance_methods, and it was parsed from the mogrify -help page. However, this was causing thread safety issues with autoloading, so I removed this feature. However, if you would ask MiniMagick::Image before if it responded to some method, it would in turn ask MiniMagick::Tool::Mogrify (because that's what it calls when you call a processing method on an image object). However, since we're not autogenerating methods anymore, I decided that "#respond_to?" will now always return true, to try to keep some backwards compatibility. That was a very bad decision, I don't know what I was thinking. Now this commit should restore the backwards compatibility by autogenerating methods when respond_to? is called.

CarrierWave was apparently initializing its CarrierWave::SanitizedFile with an instance of MiniMagick::Image, so when it asked here if the underlying file responds to #original_filename, MiniMagick::Image happily said yes (because it's always returning true). Sorry about that. :sweat_smile:

mfkp commented 9 years ago

That makes sense, thanks for the quick fix and explanation!

vanboom commented 8 years ago

Unfortunately it appears this issue still exists in 4.3.6.

 Validation failed: MyImage Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: `mogrify -to-ary /tmp/mini_magick20151121-16601-16ikfml.png` failed with error:
       mogrify: unrecognized option `-to-ary' @ error/mogrify.c/MogrifyImageCommand/6248.
janko commented 8 years ago

Could you post a stack trace? When I search for "to_ary" on CarrierWave master, it doesn't yield any results, so I don't know where it's coming from. Which version/commit are you using CarrierWave with?

vanboom commented 8 years ago

I am using Carrierwave 0.11.0, mini_magick 4.5.1

Upload Document Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: mogrify -to-ary /tmp/mini_magick20160404-5965-u21tcw.jpg failed with error: mogrify: unrecognized option `-to-ary' @ error/mogrify.c/MogrifyImageCommand/6248.

mini_magick 4.0.1 works OK

janko commented 8 years ago

@vanboom How does your uploader look like? Are you doing something like https://github.com/minimagick/minimagick/issues/338#issuecomment-185513690?

vanboom commented 8 years ago

Yes, that was it. Many thanks!!