Eliminate Rack::File headers deprecation warning #8812

Merged
merged 1 commit into from Jan 8, 2013

8 participants

@rubys

See http://intertwingly.net/projects/AWDwR4/checkdepot/section-6.1.html
rake test produces:
"Rack::File headers parameter replaces cache_control after Rack 1.5."

Despite what the message says, it appears that the hearders parameter change
will be effective as of Rack 1.5:

https://github.com/rack/rack/blob/rack-1.4/lib/rack/file.rb#L24
https://github.com/rack/rack/blob/master/lib/rack/file.rb#L24

@rubys rubys Eliminate Rack::File headers deprecation warning
See http://intertwingly.net/projects/AWDwR4/checkdepot/section-6.1.html
rake test produces:
   "Rack::File headers parameter replaces cache_control after Rack 1.5."

Despite what the message says, it appears that the hearders parameter change
will be effective as of Rack 1.5:

https://github.com/rack/rack/blob/rack-1.4/lib/rack/file.rb#L24
https://github.com/rack/rack/blob/master/lib/rack/file.rb#L24
9cc82b7
@carlosantoniodasilva carlosantoniodasilva merged commit 295806e into rails:master Jan 8, 2013
@carlosantoniodasilva
Ruby on Rails member

@rubys Thanks!

@josevalim
Ruby on Rails member

Is the parameter as a hash supported since which Rack version? We may need to enforce the rack version in the actionpack gemspec.

@carlosantoniodasilva
Ruby on Rails member

@josevalim checking.

@carlosantoniodasilva
Ruby on Rails member

I think I can lock rack in 1.4.3 for now? (it's probably going to be locked at 1.5 later anyway?)

@carlosantoniodasilva
Ruby on Rails member

Done in 4f002a1.

@dn

@rubys

I think this introduced a bug.

[2013-01-10 20:36:04] ERROR NoMethodError: undefined method `split' for nil:NilClass
gems/rack-1.4.3/lib/rack/handler/webrick.rb:69:in `block in service'
gems/rack-1.4.3/lib/rack/utils.rb:387:in `block in each'

https://github.com/rack/rack/blob/master/lib/rack/handler/webrick.rb#L69

This happens because of:

{"Last-Modified"=>"Thu, 10 Jan 2013 19:17:43 GMT", "Content-Type"=>"text/html", "Cache-Control"=>nil, "Content-Length"=>"82"}

Reproduce:

touch public/foo.html
curl http://localhost:3000/foo.html

By default cache_control is nil in development/production.

Ruby on Rails member

@dn it'd be great if you could produce a test case for us to fix it.

Would love to, I can only provide you with a simple example how to reproduce it.

touch public/foo.html
rails s
curl http://localhost:3000/foo.html

In https://github.com/rails/rails/blob/master/actionpack/test/dispatch/static_test.rb#L148 the cache_control is set, but even if I would nil it it will still not trigger the error, because in the test Rack::MockRequest https://github.com/rails/rails/blob/master/actionpack/test/dispatch/static_test.rb#L132 is used and because of that https://github.com/rack/rack/blob/master/lib/rack/handler/webrick.rb#L69 is never reached and the bug is not visible.

Can you reproduce the error - if so I would keep investigating how to write a simple test for it?

Maybe this explain it a bit further https://github.com/rack/rack/blob/master/lib/rack/lint.rb#L451. Rack does not expect a nil value in the header, but "Cache-Control"=>nil for a static file.

rails s
curl http://localhost:3000/robots.txt

Will also trigger the error. I'll try to write a test/fix for it.

Ruby on Rails member

@dn @carlosantoniodasilva was this issue addressed?

Ruby on Rails member

Saw that it's fixed here #8907 thanks

Ruby on Rails member

😄

@interhive

I'm getting the same error when using Webrick (Rails 4 Vanilla app). I switched to Thin in development and the error goes away.

@rubys

Reproduced: thanks! Fixed by #8907

@ike-bloomfire

This fix isn't in Rails 3.2.11 right?

@ike-bloomfire

Thanks

@lucaspiller

Are these fixes in 3.2.12? I've just upgraded and now am getting the "Rack::File headers parameter replaces cache_control after Rack 1.5" warning.

@carlosantoniodasilva
Ruby on Rails member
@ankit8898 ankit8898 referenced this pull request in fxn/rails-contributors Jul 20, 2013
Closed

Moving to 3.2.13 to fix the Rack Warning during rake test #23

@sgerrand sgerrand pushed a commit to sgerrand/rails that referenced this pull request Nov 2, 2013
@rubys rubys Fix regression introduced in pull request 8812 c692774
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment