[#940] Raise error when style defined & not in path #943

Closed
wants to merge 1 commit into
from

Projects

None yet

8 participants

@jayroh
jayroh commented Jun 29, 2012
  • Add StyleTokenNotFound in Errors
  • Adjust tests to make sure :style present where not currently defined
@travisbot

This pull request fails (merged d01b07c into 029fe02).

@sikachu sikachu commented on an outdated diff Jul 4, 2012
lib/paperclip/attachment.rb
@@ -358,6 +359,16 @@ def valid_assignment? file #:nodoc:
file.nil? || (file.respond_to?(:original_filename) && file.respond_to?(:content_type))
end
+ def initialize_path
+ if style_not_found_in_custom_path
+ raise(Paperclip::Errors::StyleTokenNotFound)
+ end
+ end
+
+ def style_not_found_in_custom_path
+ @options[:path] != Paperclip::Attachment.default_options[:path] && @options[:styles].present? && !@options[:path].include?(':style')
+ end
@sikachu
sikachu Jul 4, 2012 thoughtbot, inc. member
  • Should this method ends with ?, as it return true/false?
  • Can you cut this line to be <= 100 characters long?
@sikachu
Member
sikachu commented Jul 4, 2012

I have 1 comment. Can you update that and force-push to your branch?

@sikachu sikachu was assigned Jul 4, 2012
@jayroh
jayroh commented Jul 4, 2012

Will do, Prem. I'm on it.

On Tue, Jul 3, 2012 at 8:38 PM, Prem Sichanugrist <
reply@reply.github.com

wrote:

I have 1 comment. Can you update that and force-push to your branch?


Reply to this email directly or view it on GitHub:
#943 (comment)

Joel Oliveira [#940] Raise error when style defined & not in path
* Add StyleTokenNotFound in Errors
* Adjust tests to make sure :style present where not currently defined
a5d6189
@sikachu
Member
sikachu commented Jul 4, 2012

Looks good. Merging ...

@sikachu
Member
sikachu commented Jul 4, 2012

Merged in b7b2b84. Thanks!

@sikachu sikachu closed this Jul 4, 2012
@travisbot

This pull request fails (merged a5d6189 into 0958e6f).

@sikachu
Member
sikachu commented Jul 4, 2012

That failure is a lie. I'm investigating why it's sometime fail sometimes pass now.

@jayroh
jayroh commented Jul 4, 2012

Yeah I noticed the same. Something about an example file not writing to the
fs but having no problem with subsequent runs. Really weird

On Tue, Jul 3, 2012 at 9:58 PM, Prem Sichanugrist <
reply@reply.github.com

wrote:

That failure is a lie. I'm investigating why it's sometime fail sometimes
pass now.


Reply to this email directly or view it on GitHub:
#943 (comment)

@inspire22

This seems to be saying that whenever you define a custom interpolation style, you have to use it? I have like 20 different models with paperclip attachments, not all of them use the same styles & this is breaking my code.

Argh, confusion between interpolations and styles (usually thumbnail vs. original) is frustrating too

Member

Right ... this would means if you have multiple styles, you need to have :style in the :path. I think this is a dumbproof so you won't save two styles into the same path.

Anyhow, I'll push another fix with a configuration to ignore the path check. Stay tuned.

Aah, I see. I've been using a custom interpolation that solved this:

Paperclip.interpolates :my_style do |attachment, style_name|
style_name ||= attachment.default_style
style_name == :original ? '' : "#{style_name == 'big' ? '' : '-'}#{style_name}"
end

@bhoggard

What about those of us that use obfuscated file names that do not have style in them? I use the hash_data approach, for example:

:path => "public/system/:website_id/:class/:id_partition/:hash.:extension",
:url => "/system/:website_id/:class/:id_partition/:hash.:extension",
:hash_secret => Rails.application.config.asset_hash_secret,
:hash_data => ":class/:attachment/:id/:style/:timestamp",

I now have to monkeypatch style_not_in_custom_path? to use my code that worked before this merge.

@edison edison added a commit to edison/paperclip that referenced this pull request Jul 19, 2012
@edison edison Avoid pull-request #943 9c9deb1
@edison
edison commented Jul 19, 2012

I agree with @inspire22, Im having a big problem with this error in production, because some of my models don't have :styles hash defined.

Paperclip::Errors::StyleTokenNotFound: Paperclip::Errors::StyleTokenNotFound
@MadRabbit

+1 to @bhoggard we use :hash only in the :path and this patch kinda renders our entire app dead :)

@sikachu sikachu reopened this Jul 21, 2012
@sikachu
Member
sikachu commented Jul 21, 2012

I've reverted this one in master for now. What we will need is a paperclip option which turns off this feature, so that people can still using path without :style interpolation.

Going to merge this again soon and do a minor release instead. Doing a patch release was a bad idea, sorry about that.

@travisbot

This pull request fails (merged a5d6189 into 0958e6f).

@sikachu
Member
sikachu commented Jul 21, 2012

3.1.4 is out with this patch revert. Please test it out an see if it fixed your issue.

@edison
edison commented Jul 21, 2012

Everything ok now. Thank you!

@MadRabbit

Worked for me as well. Thanks!

@bhoggard

Works for me too. Thank you.

@jyurek
Member
jyurek commented Aug 17, 2012

I'm going to close this. When we figure out what we want to do, we'll make a new one.

@jyurek jyurek closed this Aug 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment