Fix send_file :disposition compat issues [#128]

compat/streaming_test.rb

@@ -105,7 +105,7 @@ context "SendData" do
   end
 
   # Deprecated. The Content-Disposition is no longer handled by sendfile.
-  specify "should include a Content-Disposition header" do
+  xspecify "should include a Content-Disposition header" do
     get '/' do
       send_file File.dirname(__FILE__) + '/public/foo.xml'
     end
@@ -118,4 +118,16 @@ context "SendData" do
     headers['Content-Transfer-Encoding'].should.equal 'binary'
   end
 
+  specify "should include a Content-Disposition header when :disposition set to attachment" do
+    get '/' do
+      send_file File.dirname(__FILE__) + '/public/foo.xml',
+        :disposition => 'attachment'
+    end
+
+    get_it '/'
+
+    should.be.ok
+    headers['Content-Disposition'].should.not.be.nil
+    headers['Content-Disposition'].should.equal 'attachment; filename="foo.xml"'
+  end
 end

lib/sinatra/base.rb

@@ -125,15 +125,24 @@ module Sinatra
       end
     end
 
-    # Use the contents of the file as the response body and attempt to
+    # Use the contents of the file at +path+ as the response body.
     def send_file(path, opts={})
       stat = File.stat(path)
       last_modified stat.mtime
+
       content_type media_type(opts[:type]) ||
         media_type(File.extname(path)) ||
         response['Content-Type'] ||
         'application/octet-stream'
+
       response['Content-Length'] ||= (opts[:length] || stat.size).to_s
+
+      if opts[:disposition] == 'attachment' || opts[:filename]
+        attachment opts[:filename] || path
+      elsif opts[:disposition] == 'inline'
+        response['Content-Disposition'] = 'inline'
+      end
+
       halt StaticFile.open(path, 'rb')
     rescue Errno::ENOENT
       not_found

lib/sinatra/compat.rb

@@ -104,17 +104,6 @@ module Sinatra
       etag(*args, &block)
     end
 
-    # The :disposition option is deprecated; use: #attachment. This method
-    # setting the Content-Transfer-Encoding header is deprecated.
-    #--
-    # TODO deprecation warning for :disposition argument.
-    def send_file(path, opts={})
-      opts[:disposition] = 'attachment' if !opts.key?(:disposition)
-      attachment opts[:filename] || path if opts[:filename] || opts[:disposition]
-      response['Content-Transfer-Encoding'] = 'binary' if opts[:disposition]
-      super(path, opts)
-    end
-
     # Throwing halt with a Symbol and the to_result convention are
     # deprecated. Override the invoke method to detect those types of return
     # values.

test/helpers_test.rb

@@ -240,11 +240,11 @@ describe 'Helpers#send_file' do
     @file = nil
   end
 
-  def send_file_app
+  def send_file_app(opts={})
     path = @file
     mock_app {
       get '/file.txt' do
-        send_file path
+        send_file path, opts
       end
     }
   end
@@ -283,6 +283,24 @@ describe 'Helpers#send_file' do
     get '/'
     assert not_found?
   end
+
+  it "does not set the Content-Disposition header by default" do
+    send_file_app
+    get '/file.txt'
+    assert_nil response['Content-Disposition']
+  end
+
+  it "sets the Content-Disposition header when :disposition set to 'attachment'" do
+    send_file_app :disposition => 'attachment'
+    get '/file.txt'
+    assert_equal 'attachment; filename="file.txt"', response['Content-Disposition']
+  end
+
+  it "sets the Content-Disposition header when :filename provided" do
+    send_file_app :filename => 'foo.txt'
+    get '/file.txt'
+    assert_equal 'attachment; filename="foo.txt"', response['Content-Disposition']
+  end
 end
 
 describe 'Helpers#last_modified' do