Browse files

URLGenerator should disregard a ? which isn't followed by an =

In some case, we seriously don't want to prepend the timestamp with & if there wasn't a query string there. If the original URL doesn't have any query string, we should add the ?.
  • Loading branch information...
1 parent bb22be3 commit 128d664c1143d119922393fde342431c1523535e @sikachu sikachu committed Dec 12, 2011
Showing with 4 additions and 4 deletions.
  1. +2 −2 lib/paperclip/url_generator.rb
  2. +2 −2 test/url_generator_test.rb
4 lib/paperclip/url_generator.rb
@@ -38,7 +38,7 @@ def most_appropriate_url
def timestamp_as_needed(url, options)
if options[:timestamp] && timestamp_possible?
- delimiter_char = url.include?('?') ? '&' : '?'
+ delimiter_char = url.match(/\?.+=/) ? '&' : '?'
@@ -58,7 +58,7 @@ def escape_url_as_needed(url, options)
def escape_url(url)
- url.respond_to?(:escape) ? url.escape : URI.escape(url)
+ (url.respond_to?(:escape) ? url.escape : URI.escape(url)).gsub(/(\/.+)\?(.+\.)/, '\1%3F\2')

This line means I can no longer have a URL from an interpolation look like so, note the ? for the params. I have worked around the issue by defining a singleton method for #escape on the returned string. The subject of the commit seems to indicated this should not happen since indeed my returned URL does have an = after it.

Paperclip.interpolates(:my_authed_url) do |attachment, style|
  params = "?filename=#{attachment.original_filename}&key=#{attachment.instance.random_secret}"
  "{params}".tap do |url|
    def url.escape

So is this pattern what we need to do in my case of returning a self build url in an interpolation?

In fact I am wrong, even defining an escape will not work as no matter what the ? is converted. This should not be done if there is an = after it right? That line should maybe include that logic? Open an issue?

OK, figured it out. I had to pass :escape => false to my #url method. Duh :/ Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 test/url_generator_test.rb
@@ -146,8 +146,8 @@ def escape
assert_equal "#{expected}?#{updated_at}", result
- should "produce URLs with the updated_at when it exists, separated with a & if a ? already exists" do
- expected = "the?expected result"
+ should "produce URLs with the updated_at when it exists, separated with a & if a ? follow by = already exists" do
+ expected = "the?expected=result"
updated_at = 1231231234
mock_interpolator = => expected)
mock_attachment = => updated_at)

0 comments on commit 128d664

Please sign in to comment.