Carrierwave does not delete old file?

[Christian Fazzini]

Lets say user has a profile photo that he uploads. The user decides to
change profile photo. Do you notice that the old profile photo still
remains on disk? Is this normal behavior for carrierwave?

Should I be explicit about this in the code? i.e. check if a file
already exists for this user, if so delete it first? Or shouldn't
carrierwave do this automatically for me instead?

PS. I am using active record and pgsql

[Jonas Nicklas]

This is a known issue, and whilemany people (including me) have tried
to solve it, no one has succeeded yet.

/Jonas

> --
> You received this message because you are subscribed to the Google Groups "carrierwave" group.
> To post to this group, send email to carri...@googlegroups.com.
> To unsubscribe from this group, send email to carrierwave...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/carrierwave?hl=en.
>
>

[Christian Fazzini]

No workaround? Must we delete the image manually then? i.e. check if
image exists, if so delete it first before uploading a new one?

On Jan 27, 4:08 pm, Jonas Nicklas <jonas.nick...@gmail.com> wrote:
> This is a known issue, and whilemany people (including me) have tried
> to solve it, no one has succeeded yet.
>
> /Jonas
>
> On Wed, Jan 26, 2011 at 6:31 PM, Christian Fazzini
>

[Jonas Nicklas]

I'm not aware of any workaround, but do search the mailing list, this
topic has come up a number of times.

/Jonas

[Samuel Lown]

If I recall correctly, this only happens when you replace an image, destroying the model should delete the file. Assuming this is correct, you might want to try creating a seperate model for your profile photos: if you want to replace an image, delete the old profile photo object and create a new one in its place.

Alternatively, you can force the filename to something specific so that the old will always be replaced:

    def filename

      'avatar.png' unless original_filename.blank?

    end

Cheers,

sam

--
www.samlown.com
www.autofiscal.com
www.planetaki.com/sam

[Victor Stan]

I made a module to take care of this for my own use. It assumes you are storing files somewhere in the public folder

In the Model use

  # assumed root of all images in within the public folder

  # reveals a method: delete_all_other_files_except(file) to class

  include DeleteAllOtherFiles

the module delete_all_other_file.rb is in the modules directory, but you can obviously put it wherever as long as you can include it as a module in the model.

module DeleteAllOtherFiles

  def delete_all_other_files_except(file)

   raise "File can't be blank, for delete_all_other_files_except method."  if file.nil?

    main_path = Rails.root.to_s+"/public"

    location = main_path+'/'+file.store_dir.to_s

    Rails.logger.debug('This is the path from within the module: '+main_path)

   

    all_files = []

    Find.find(location) do |f|

      all_files << f

    end

    skip_me = []

    skip_me << file.to_s

    file.versions.each do |key, value|

      skip_me << value.to_s

    end

    Rails.logger.debug(skip_me.to_yaml)

    delete_me = all_files - skip_me.collect { |v| main_path+v }

    delete_me.each { |f| if File.stat(f).file? then File.delete(f) end }

    Rails.logger.debug(delete_me.to_yaml)

  end

end

and in the controller I do this:

def update

    @home = Home.find(params[:id])

    if @home.update_attributes(params[:home])

      # delete images we don't need

      @home.delete_all_other_files_except(@home.image)

      flash[:notice] = "Successfully updated feature."

      redirect_to @home

    else

      render :action => 'edit'

    end

  end

critiques welcome.

[Christian Fazzini]

Samuel, althought that might seem like a clean workaround. It isn't
really. i.e. if the user decides to upload a file with a different
extension.

Jonas Nicklas, have you taken Victor Stan's suggestion into
consideration? IMO, this is quite a crucial feature to have in
Carrierwave. Leaving uploaded files unattended only consumes server
space

[Christian Fazzini]

The last comment made by ctide sorta works. It removes the file from
the file system but does not remove the object attached from Active
Record.

https://github.com/jnicklas/carrierwave/issues/issue/72

My temporary solution for this is. In def update, check if an
attachment has been uploaded via the params and if so, then remove
attachment from the user object:

def update
@user = User.find(params[:id])

# Carrierwave bug - Carrierwave does not remove old files when new
files are updated on the same model
# https://github.com/jnicklas/carrierwave/issues/issue/72
if params[:user][:attachment]
# Only remove the old file from the file system
@user.attachment.remove!
end
end

On Jan 28, 4:05 pm, Christian Fazzini <christian.fazz...@gmail.com>
wrote:

[Samuel Lown]

Samuel, althought that might seem like a clean workaround. It isn't
really. i.e. if the user decides to upload a file with a different
extension.

If you don't store the original image, or at least resize it, everything should be converted to the format you specify. This is the way MiniMagick of RMagick work. Granted, its not perfect, but if it is really important that you store the original, stick it in another model and be done with it :-)

 

Jonas Nicklas, have you taken Victor Stan's suggestion into
consideration? IMO, this is quite a crucial feature to have in
Carrierwave. Leaving uploaded files unattended only consumes server
space

That solution, as Victor said, will only work on local storage and would be a bit in-efficient with a remote service where 'stats' take time to complete.

I guess this is a bit like the old Active Record "has_and_belongs_to_many" debate, its convenient, but you know you should be using a real model for the relationship :-)

Cheers,

sam

 

--
You received this message because you are subscribed to the Google Groups "carrierwave" group.
To post to this group, send email to carri...@googlegroups.com.
To unsubscribe from this group, send email to carrierwave...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/carrierwave?hl=en.

--
www.samlown.com
www.autofiscal.com
www.planetaki.com/sam

[Christian Fazzini]

Yea but sticking with another model is an resourceful. Having a second
model just to succumb to this bug is unjustifiable :-P The bug should
be fixed. Have been looking around google. Seems like this bug has
been around for a while. Does the developer of the gem not want to fix
this?

> > carrierwave...@googlegroups.com<carrierwave%2Bunsu...@googlegroups.com>
> > .

[Samuel Lown]

Yea but sticking with another model is an resourceful. Having a second
model just to succumb to this bug is unjustifiable :-P The bug should
be fixed. Have been looking around google. Seems like this bug has
been around for a while. Does the developer of the gem not want to fix
this?

I can see why you might find it frustrating, but I'm sure if you sat down and tried to think of how to solve this problem you'd run into the same road-blocks as everyone else. Its just simply not an easy problem to solve, otherwise it would have been fixed already, I'm sure you can appreciate that.

Personally, I think adding another model is quite an elegant solution. Mix it in with accepts_nested_attributes_for and its not too complicated to implement either.

Cheers,

sam

 

To unsubscribe from this group, send email to carrierwave...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/carrierwave?hl=en.

--
www.samlown.com
www.autofiscal.com
www.planetaki.com/sam

[Trevor Turk]

On Friday, January 28, 2011 10:53:16 AM UTC, Sam Lown wrote:

I can see why you might find it frustrating, but I'm sure if you sat down and tried to think of how to solve this problem you'd run into the same road-blocks as everyone else. Its just simply not an easy problem to solve, otherwise it would have been fixed already, I'm sure you can appreciate that.

Personally, I think adding another model is quite an elegant solution. Mix it in with accepts_nested_attributes_for and its not too complicated to implement either.

I've been trying to work through some of the existing issues on Github (I'm drafting another post to the group about this now) and I've made some major updates to the wiki page. One of the people that's been bothered by this issue has added a "known issues" area that describes this problem, and suggests a workaround. 

https://github.com/jnicklas/carrierwave/wiki

If any of you have time, please consider updating that wiki page to include more detail, more possible workarounds, etc. I'm trying to move us toward having an area where we clearly document "known issue" like this that may never be solved. This way, we have something to link people to if/when they ask, and hopefully some kind of work around that might help them out. 

Thanks!

[Jeroen R.]

I just added the following to the wiki and as a comment in the issue
tracker:

For GridFS there is a very easy fix. MongoDB Ruby driver allows you to
specify if the old files should be deleted or not. Default behavior is
that GridFS keeps old versions when you store a new version.

File: lib/carrierwave/storage/grid_fs.rb
line: 64

change:

grid.open(@uploader.store_path, 'w', :content_type =>
file.content_type) do |f|

to:
grid.open(@uploader.store_path, 'w', :content_type =>
file.content_type,:delete_old=>true) do |f|
That should do the trick for GridFS.

Jeroen

[Jonas Nicklas]

that's only going to work as long as the store_path is the same.

/Jonas

[Jeroen R.]

Correct. In my case where I store user avatars, I keep the storepath
the same for all uploads and it works fine.

If you are using different store_paths then yes it does not work.

[Swaathi Kakarla]

So I ran into the same problem, and this is how I solved it. Maybe you'll find it useful :)

In your uploader, use a before filter to perform actions before saving the new image.

before :save, :delete_old_file


def delete_old_file
    FileUtils.rm_rf(model.profile_picture.url)
end

[Paul Osetinsky]

nice fix Swaathi - worked for me