Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix returning a new instance of the uploader on every uploader call #1860

Merged

Conversation

DarthSim
Copy link
Contributor

@DarthSim DarthSim commented Feb 1, 2016

How to reproduce:

class Picture < ActiveRecord::Base
  mount_uploader :image, ImageUploader
end

pic = Picture.new
pic.image.object_id # => 70317811568320
pic.image.object_id # => 70317821530860

First of all, this bug generates new objects every time, and this is definitely not good. Also this affects on actions with uploader. For example:

pic = Picture.new
pic.image.retrieve_from_cache!("1454349370-83816-0001-5310/image.jpg")
pic.image.blank? # => true

@mehlah
Copy link
Member

mehlah commented Feb 2, 2016

LGTM 👍

@thomasfedb
Copy link
Contributor

Looks like a sensible change @DarthSim. My only query before I can merge this is if it keeps the single and plural mounters consistent. Could you check this please?

@DarthSim
Copy link
Contributor Author

DarthSim commented Feb 2, 2016

@thomasfedb It does, why shouldn't it?

@thomasfedb
Copy link
Contributor

@DarthSim I just wanted to compare the behaviour of these two:

def mount_uploader(column, uploader=nil, options={}, &block)
  # ...

  def #{column}
    _mounter(:#{column}).uploaders[0] ||= _mounter(:#{column}).blank_uploader
  end
def mount_uploaders(column, uploader=nil, options={}, &block)
  # ...

  def #{column}
    _mounter(:#{column}).uploaders
  end

@thomasfedb
Copy link
Contributor

Clearly you changes makes this more consistent, as the later example was already returning a stable object.

thomasfedb added a commit that referenced this pull request Feb 2, 2016
Memorize uploader instance when creating a blank uploader
@thomasfedb thomasfedb merged commit 488dc4a into carrierwaveuploader:master Feb 2, 2016
mynock pushed a commit to mynock/carrierwave that referenced this pull request Aug 9, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants