Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recreate_versions!(...) ignores :from_version => ... #1164

Closed
igreg opened this issue Jul 18, 2013 · 8 comments · Fixed by #1879
Closed

recreate_versions!(...) ignores :from_version => ... #1164

igreg opened this issue Jul 18, 2013 · 8 comments · Fixed by #1879
Labels

Comments

@igreg
Copy link

igreg commented Jul 18, 2013

Having the following code:

class MyUploader < CarrierWave::Uploader::Base
  version :medium do
    ...
  end

  version :small, :from_version => :medium do
    ...
  end
end

CarrierWave works as expected when I assign the file for the first time.
It also works when I call recreate_versions! without any parameters.
But when I call recreate_versions!(:small), it ignores :from_version and use the original file that was uploaded.

I suspect the bug to be in the method recreate_versions!:

def recreate_versions!(*versions)
  # Some files could possibly not be stored on the local disk. This
  # doesn't play nicely with processing. Make sure that we're only
  # processing a cached file
  #
  # The call to store! will trigger the necessary callbacks to both
  # process this version and all sub-versions
  if versions.any?
    file = sanitized_file if !cached?
    store_versions!(file, versions)
  else
    cache! if !cached?
    store!
  end
end

https://github.com/carrierwaveuploader/carrierwave/blob/master/lib/carrierwave/uploader/versions.rb#L223

It passes sanitized_file to store_versions! which I suspect is the original file that was uploaded instead of being the cache version corresponding to :medium

@JeskTop
Copy link

JeskTop commented Sep 17, 2013

I have the same problems.But I don't know how to fix it.

@andrewcohen
Copy link

I was able to make this work by reimplementing the recreate_versions! method inside of my uploader class.

  def recreate_versions!(*versions)
    if versions.any?
      removable_versions(versions).each { |name, v| v.remove! }
      from_versions = versions.group_by { |v| self.class.versions[v][:options][:from_version] }
      from_versions.each do |from, versions|
        next if from.nil?
        file = CarrierWave::SanitizedFile.new(self.versions[from.to_sym].file)
        store_versions!(file, versions)
      end
    else
      super(*versions)
    end
  end

  def removable_versions(requested_versions)
    # versions is a method defined by carrierwave
    versions.select { |k, v| requested_versions.include?(k) }
  end

@benben
Copy link

benben commented Nov 29, 2013

I can confirm this bug. the workaround from @andrewcohen does not work for me though.

Some more info: It does not work when using recreate_versions! with or without parameters. Used carrierwave 0.9.0

@mchlfhr
Copy link

mchlfhr commented Aug 6, 2015

Just stumbled over this. Sad, it is not fixed yet (confirming issue still exists using CarrierWave 0.10.0). It would make my code much cleaner.

I saw this when I wanted to do some of my picture work over a background job - where several jobs do different manipulations (e.g. one does a resize another does some cropping stuff). With a working recreate_versions!(...), this would have been much easier.

I am currently using CarrierWaves conditional-versions for achieving my goal:

version :thumb, if: :size_job? do
  process resize_to_fill: [165, 165]
end

and then:

protected

def size_job?(file)
  model.job_processing.present? && model.job_type == "size"
end

Whereby .job_processing and .job_type are virtual attributes of the model set by the job at execution.

@thomasfedb
Copy link
Contributor

May be related to #1602.

@fwuensche
Copy link

fwuensche commented Apr 25, 2017

EDIT: This hack worked for version carrierwave (0.10.0)
As @molefrog mentioned on the following comment, it won't work for > 1.0.0


I followed more or less @andrewcohen solution. I didn't overwrite recreate_versions though. That's because I don't want other people to be surprise if their method does not work as usual.

class ImageUploader < CarrierWave::Uploader::Base
  include CarrierWave::ImageOptimizer
  include CarrierWave::RMagick

  storage :fog

  def recreate_versions_for!(*versions)
    if versions.any?
      removable_versions(versions).each { |name, v| v.remove! }
      from_versions = versions.group_by { |v| self.class.versions[v][:options][:from_version] }
      from_versions.each do |from, versions|
        next if from.nil?
        file = CarrierWave::SanitizedFile.new(self.versions[from.to_sym].file)
        store_versions!(file, versions)
      end
    end
  end

  def removable_versions(requested_versions)
    versions.select { |k, v| requested_versions.include?(k) }
  end

  # ...
end

Be careful if you have multiple images chained one after the other. For example, the code below won't work if you try to recreate :thumb.

version :xlarge do
  ...
end

version :large, from_version: :xlarge do
  ...
end

version :thumb, from_version: :large do
  ...
end

So please make sure all of them have at most one dependency, for instance:

version :xlarge do
  ...
end

version :large, from_version: :xlarge do
  ...
end

#                             vvvvvvv
version :thumb, from_version: :xlarge do
  ...
end

Than you can call:

a = Art.find(123)

# since xlarge only depends on the original file
art.image.recreate_versions!(:xlarge)

art.image.recreate_versions_regarding_from_version!(:large)
art.image.recreate_versions_regarding_from_version!(:thumb)

@molefrog
Copy link

For those who stumbled upon this issue, the hacks proposed by @fwuensche and @andrewcohen don't work starting from version 1.0.0 (didn't have chance to test it on earlier versions, sorry).
Using monkey patching is probably not the best idea for production apps, so I decided to change the code to recreate_versions! without any parameters. Also, we use fog, so the patch won't help anyway.

However, I do feel like the fix should be quite easy to implement. I started my own research by reading the source code, but seems like I don't exactly understand some core concepts like caching etc. Does anyone here have some time to guide me over the source code and maybe help me doing a PR?

Hope, I can get some attention from the team members 🙏

@graudeejs
Copy link

I've stumbled upon this as well.
While recreate_versions! for particular versions don't work well (ignores :from_version options), It works fine without arguments (in which case it recreates all versions correctly).

For me, at this moment it's good enough given that my files are on servers disk. I haven't checked if this works as expected if files are in S3 though.

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

Successfully merging a pull request may close this issue.

9 participants