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

[Upgraded] For paperclip 3.1.2, with S3 fixes, and generic paperclip versions #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tommeier
Copy link
Collaborator

@tommeier tommeier commented Jul 2, 2012

Updated with my previous fixes to ensure it works on S3 too.

This is now working with paperclip 3.1.2, updated tests accordingly and gemfiles.

…or, tested across rails versions and added Rails 3.2 to test suite. Paperclip version bumped to 2.7.

  - Main issue - `attachment`_updated_at was not being updated, new Paperclip (> 2.4.5) assigns the updated_at of the `attachment`, when S3 took too long, or the queue takes longer than a second to pick the image up, the `attachment`_updated_at and instance.updated_at get out of sync, and the url generated (with :updated_at in the hash_data - default), will not find the newly created images.
@tommeier
Copy link
Collaborator Author

tommeier commented Jul 2, 2012

So in gemfile :

gem 'delayed_paperclip'    , '2.4.5.2', :git => 'git://github.com/tommeier/delayed_paperclip', :branch => 'fix_312'

@tommeier
Copy link
Collaborator Author

tommeier commented Jul 2, 2012

Good point @timols, i'll update now.

@tommeier
Copy link
Collaborator Author

tommeier commented Jul 2, 2012

Removed unnecessary line and made sure a test checks the resulting value in case paperclip changes their API.

@bastien
Copy link

bastien commented Jul 10, 2012

The redefinition of the most_appropriate_url doesn't work for me, it's still the original Paperclip::UrlGenerator one that is called. Also why not just creating a new UrlGenerator inheriting from Paperclip::UrlGenerator? You could just modify the default options to use it then. Here's what I did in my app:

module Paperclip
  class MyUrlGenerator < UrlGenerator

    def for(style_name, options)
      escape_url_as_needed(
        timestamp_as_needed(
          @attachment_options[:interpolator].interpolate(most_appropriate_url(options), @attachment, style_name),
          options
      ), options)
    end

    private

    def most_appropriate_url(options)
      if @attachment.original_filename.nil? || (!options[:without_processing] && @attachment.delayed_default_url?)
        default_url
      else
        @attachment_options[:url]
      end
    end
  end
end

And then in my model

Class Image < ActiveRecord::Base
    has_attached_file(:resource, :url_generator => Paperclip::MyUrlGenerator, ... )
    ...
end

@bastien
Copy link

bastien commented Jul 10, 2012

Oh, I also had another problem with your patch, you can't call reprocess! directly anymore, it won't do anything, you need to call process_delayed!. Maybe more a note than an issue as it's not something you want to call too often.

@tommeier
Copy link
Collaborator Author

@bastien good idea, just want to work out a cleaner way to make it apply across the board when using delayed_paperclip, didn't have time to sort it out, so the quick hack won in the end unfortunately.

Calling reprocess! is working fine in prod for us?

@bastien
Copy link

bastien commented Jul 11, 2012

@tommeier is it because you set the attachment post_processing to true manually?

jrgifford referenced this pull request in jrgifford/delayed_paperclip Oct 7, 2012
Changelog:
 - Merge in upstream #69.
 - Various odds and ends in the readme.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants