Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 1 commit into from

8 participants

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

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

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 Admin
sikachu added a note
  • Should this method ends with ?, as it return true/false?
  • Can you cut this line to be <= 100 characters long?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sikachu
Admin

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

@sikachu sikachu was assigned
@jayroh
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
Admin

Looks good. Merging ...

@sikachu
Admin

Merged in b7b2b84. Thanks!

@sikachu sikachu closed this
@travisbot

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

@sikachu
Admin

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

@jayroh
@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

Admin

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 referenced this pull request from a commit in edison/paperclip
@edison edison Avoid pull-request #943 9c9deb1
@edison

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
@sikachu
Admin

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
Admin

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

@edison

Everything ok now. Thank you!

@MadRabbit

Worked for me as well. Thanks!

@bhoggard

Works for me too. Thank you.

@jyurek
Admin

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 4, 2012
  1. [#940] Raise error when style defined & not in path

    Joel Oliveira authored
    * Add StyleTokenNotFound in Errors
    * Adjust tests to make sure :style present where not currently defined
This page is out of date. Refresh to see the latest.
View
12 lib/paperclip/attachment.rb
@@ -77,6 +77,7 @@ def initialize(name, instance, options = {})
@source_file_options = options[:source_file_options]
@whiny = options[:whiny]
+ initialize_path
initialize_storage
end
@@ -358,6 +359,17 @@ def valid_assignment? file #:nodoc:
file.nil? || (file.respond_to?(:original_filename) && file.respond_to?(:content_type))
end
+ def initialize_path
+ if style_not_in_custom_path?
+ raise(Paperclip::Errors::StyleTokenNotFound)
+ end
+ end
+
+ def style_not_in_custom_path?
+ @options[:path] != Paperclip::Attachment.default_options[:path] &&
+ @options[:styles].present? && !@options[:path].include?(':style')
+ end
+
def initialize_storage #:nodoc:
storage_class_name = @options[:storage].to_s.downcase.camelize
begin
View
5 lib/paperclip/errors.rb
@@ -9,6 +9,11 @@ module Errors
class StorageMethodNotFound < Paperclip::Error
end
+ # Will be thrown when there are custom styles and those styles aren't
+ # included in the path
+ class StyleTokenNotFound < Paperclip::Error
+ end
+
# Will be thrown when a command or executable is not found.
class CommandNotFoundError < Paperclip::Error
end
View
4 test/attachment_test.rb
@@ -309,7 +309,7 @@ class AttachmentTest < Test::Unit::TestCase
context "An attachment with a default style and an extension interpolation" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => { :default => ["100x100", :png] },
:default_style => :default
@file = StringIO.new("...")
@@ -317,7 +317,7 @@ class AttachmentTest < Test::Unit::TestCase
end
should "return the right extension for the path" do
@attachment.assign(@file)
- assert_equal "file.png", @attachment.path
+ assert_equal "default_file.png", @attachment.path
end
end
View
2  test/storage/s3_test.rb
@@ -228,7 +228,7 @@ def teardown
rebuild_model :styles => { :large => ['500x500#', :jpg] },
:storage => :s3,
:bucket => "bucket",
- :path => ":attachment/:basename.:extension",
+ :path => ":attachment/:style/:basename.:extension",
:s3_credentials => {
'access_key_id' => "12345",
'secret_access_key' => "54321"
View
25 test/style_test.rb
@@ -5,7 +5,7 @@ class StyleTest < Test::Unit::TestCase
context "A style rule" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":basename_:style.:extension",
:styles => { :foo => {:geometry => "100x100#", :format => :png} },
:whiny => true
@style = @attachment.styles[:foo]
@@ -33,9 +33,18 @@ class StyleTest < Test::Unit::TestCase
end
end
+ context "A path not containing a style rule" do
+ should "generate a warning when a :style token isn't found" do
+ assert_raises(Paperclip::Errors::StyleTokenNotFound) do
+ @attachment = attachment :path => ":basename_:extension",
+ :styles => { :foo => {:geometry => "100x100#", :format => :png} }
+ end
+ end
+ end
+
context "A style rule with properties supplied as procs" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:whiny_thumbnails => true,
:processors => lambda {|a| [:test]},
:styles => {
@@ -64,7 +73,7 @@ class StyleTest < Test::Unit::TestCase
styles[:aslist] = ["100x100", :png]
styles[:ashash] = {:geometry => "100x100", :format => :png}
styles[:asstring] = "100x100"
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => styles
end
should "have the right number of styles" do
@@ -97,7 +106,7 @@ class StyleTest < Test::Unit::TestCase
context "An attachment with :convert_options" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => {:thumb => "100x100", :large => "400x400"},
:convert_options => {:all => "-do_stuff", :thumb => "-thumbnailize"}
@style = @attachment.styles[:thumb]
@@ -117,7 +126,7 @@ class StyleTest < Test::Unit::TestCase
context "An attachment with :source_file_options" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => {:thumb => "100x100", :large => "400x400"},
:source_file_options => {:all => "-density 400", :thumb => "-depth 8"}
@style = @attachment.styles[:thumb]
@@ -137,7 +146,7 @@ class StyleTest < Test::Unit::TestCase
context "A style rule with its own :processors" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => {
:foo => {
:geometry => "100x100#",
@@ -162,7 +171,7 @@ class StyleTest < Test::Unit::TestCase
context "A style rule with :processors supplied as procs" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => {
:foo => {
:geometry => "100x100#",
@@ -184,7 +193,7 @@ class StyleTest < Test::Unit::TestCase
context "An attachment with :convert_options and :source_file_options in :styles" do
setup do
- @attachment = attachment :path => ":basename.:extension",
+ @attachment = attachment :path => ":style_:basename.:extension",
:styles => {
:thumb => "100x100",
:large => {:geometry => "400x400",
Something went wrong with that request. Please try again.