Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Got tests to pass with some more changes.

  * request.formats is much simpler now
    * For XHRs or Accept headers with a single item, we use the Accept header
    * For other requests, we use params[:format] or fallback to HTML
    * This is primarily to work around the fact that browsers provide completely
      broken Accept headers, so we have to whitelist the few cases we can
      specifically isolate and treat other requests as coming from the browser
    * For APIs, we can support single-item Accept headers, which disambiguates
      from the browsers
  * Requests to an action that only has an XML template from the browser will
    no longer find the template. This worked previously because most browsers
    provide a catch-all */*, but this was mostly accidental behavior. If you
    want to serve XML, either use the :xml format in links, or explicitly
    specify the XML template: render "template.xml".
  • Loading branch information...
commit 1310231c15742bf7d99e2f143d88b383c32782d3 1 parent 9b552fb
@wycats wycats authored
Showing with 123 additions and 127 deletions.
  1. +15 −7 actionpack/lib/action_controller/metal/rendering_controller.rb
  2. +1 −1  actionpack/lib/action_controller/testing/process.rb
  3. +12 −24 actionpack/lib/action_dispatch/http/request.rb
  4. +12 −1 actionpack/lib/action_view/base.rb
  5. +3 −2 actionpack/lib/action_view/helpers/prototype_helper.rb
  6. +1 −1  actionpack/lib/action_view/template/resolver.rb
  7. +1 −1  actionpack/test/controller/content_type_test.rb
  8. +17 −34 actionpack/test/controller/mime_responds_test.rb
  9. +3 −3 actionpack/test/controller/render_js_test.rb
  10. +1 −1  actionpack/test/dispatch/mime_type_test.rb
  11. +30 −33 actionpack/test/dispatch/request_test.rb
  12. +0 −1  actionpack/test/fixtures/content_type/render_default_content_types_for_respond_to.rhtml
  13. +10 −10 actionpack/test/lib/fixture_template.rb
  14. +2 −2 actionpack/test/new_base/content_type_test.rb
  15. +1 −1  actionpack/test/new_base/render_layout_test.rb
  16. +3 −4 actionpack/test/new_base/render_rjs_test.rb
  17. +1 −1  actionpack/test/new_base/render_template_test.rb
  18. +4 −0 actionpack/test/template/javascript_helper_test.rb
  19. +4 −0 actionpack/test/template/prototype_helper_test.rb
  20. +2 −0  actionpack/test/template/render_test.rb
View
22 actionpack/lib/action_controller/metal/rendering_controller.rb
@@ -8,12 +8,17 @@ def self.get(klass, formats, locale)
attr_accessor :hash
def initialize(klass, formats, locale)
+ @formats, @locale = formats, locale
@hash = [formats, locale].hash
end
alias_method :eql?, :equal?
+
+ def inspect
+ "#<HashKey -- formats: #{@formats} locale: #{@locale}>"
+ end
end
-
+
module RenderingController
extend ActiveSupport::Concern
@@ -25,7 +30,7 @@ def clear_template_caches!
template_cache.clear
super
end
-
+
def template_cache
@template_cache ||= Hash.new {|h,k| h[k] = {} }
end
@@ -33,16 +38,19 @@ def template_cache
def process_action(*)
self.formats = request.formats.map {|x| x.to_sym}
- Thread.current[:format_locale_key] = HashKey.get(self.class, formats, I18n.locale)
+
+ super
+ end
+
+ def _determine_template(*)
super
end
def render(options)
+ Thread.current[:format_locale_key] = HashKey.get(self.class, formats, I18n.locale)
+
super
- self.content_type ||= begin
- mime = options[:_template].mime_type
- formats.include?(mime && mime.to_sym) || formats.include?(:all) ? mime : Mime::Type.lookup_by_extension(formats.first)
- end.to_s
+ self.content_type ||= options[:_template].mime_type.to_s
response_body
end
View
2  actionpack/lib/action_controller/testing/process.rb
@@ -148,7 +148,7 @@ def process(action, parameters = nil, session = nil, flash = nil, http_method =
def xml_http_request(request_method, action, parameters = nil, session = nil, flash = nil)
@request.env['HTTP_X_REQUESTED_WITH'] = 'XMLHttpRequest'
- @request.env['HTTP_ACCEPT'] = [Mime::JS, Mime::HTML, Mime::XML, 'text/xml', Mime::ALL].join(', ')
+ @request.env['HTTP_ACCEPT'] ||= [Mime::JS, Mime::HTML, Mime::XML, 'text/xml', Mime::ALL].join(', ')
returning __send__(request_method, action, parameters, session, flash) do
@request.env.delete 'HTTP_X_REQUESTED_WITH'
@request.env.delete 'HTTP_ACCEPT'
View
36 actionpack/lib/action_dispatch/http/request.rb
@@ -106,16 +106,10 @@ def accepts
@env["action_dispatch.request.accepts"] ||= begin
header = @env['HTTP_ACCEPT'].to_s.strip
- fallback = xhr? ? Mime::JS : Mime::HTML
-
if header.empty?
- [content_type, fallback, Mime::ALL].compact
+ [content_type]
else
- ret = Mime::Type.parse(header)
- if ret.last == Mime::ALL
- ret.insert(-2, fallback)
- end
- ret
+ Mime::Type.parse(header)
end
end
end
@@ -163,26 +157,20 @@ def fresh?(response)
# GET /posts/5 | request.format => Mime::HTML or MIME::JS, or request.accepts.first depending on the value of <tt>ActionController::Base.use_accept_header</tt>
#
def format(view_path = [])
- @env["action_dispatch.request.format"] ||=
- if parameters[:format]
- Mime[parameters[:format]]
- elsif ActionController::Base.use_accept_header && !(accepts == ONLY_ALL)
- accepts.first
- elsif xhr? then Mime::JS
- else Mime::HTML
- end
+ formats.first
end
def formats
- if ActionController::Base.use_accept_header
- if param = parameters[:format]
- Array.wrap(Mime[param])
+ accept = @env['HTTP_ACCEPT']
+
+ @env["action_dispatch.request.formats"] ||=
+ if parameters[:format]
+ [Mime[parameters[:format]]]
+ elsif xhr? || (accept && !accept.include?(?,))
+ accepts
else
- accepts.dup
+ [Mime::HTML]
end
- else
- [format]
- end
end
# Sets the \format by string extension, which can be used to force custom formats
@@ -198,7 +186,7 @@ def formats
# end
def format=(extension)
parameters[:format] = extension.to_s
- @env["action_dispatch.request.format"] = Mime::Type.lookup_by_extension(parameters[:format])
+ @env["action_dispatch.request.formats"] = [Mime::Type.lookup_by_extension(parameters[:format])]
end
# Returns a symbolized version of the <tt>:format</tt> parameter of the request.
View
13 actionpack/lib/action_view/base.rb
@@ -175,6 +175,17 @@ module Subclasses
attr_accessor :controller
attr_internal :captures
+ def reset_formats(formats)
+ @formats = formats
+
+ if defined?(ActionController)
+ # This is expensive, but we need to reset this when the format is updated,
+ # which currently only happens
+ Thread.current[:format_locale_key] =
+ ActionController::HashKey.get(self.class, formats, I18n.locale)
+ end
+ end
+
class << self
delegate :erb_trim_mode=, :to => 'ActionView::TemplateHandlers::ERB'
delegate :logger, :to => 'ActionController::Base', :allow_nil => true
@@ -240,7 +251,7 @@ def self.for_controller(controller)
end
def initialize(view_paths = [], assigns_for_first_render = {}, controller = nil, formats = nil)#:nodoc:
- @formats = formats || [:html]
+ @formats = formats
@assigns = assigns_for_first_render.each { |key, value| instance_variable_set("@#{key}", value) }
@controller = controller
@helpers = self.class.helpers || Module.new
View
5 actionpack/lib/action_view/helpers/prototype_helper.rb
@@ -991,12 +991,13 @@ def record(line)
def render(*options_for_render)
old_formats = @context && @context.formats
- @context.formats = [:html] if @context
+
+ @context.reset_formats([:html]) if @context
Hash === options_for_render.first ?
@context.render(*options_for_render) :
options_for_render.first.to_s
ensure
- @context.formats = old_formats if @context
+ @context.reset_formats(old_formats) if @context
end
def javascript_object_for(object)
View
2  actionpack/lib/action_view/template/resolver.rb
@@ -42,7 +42,7 @@ def handler_matcher
def handler_glob
@handler_glob ||= begin
- e = TemplateHandlers.extensions.map{|h| ".#{h},"}.join
+ e = TemplateHandlers.extensions.map{|h| ".#{h}"}.join(",")
"{#{e}}"
end
end
View
2  actionpack/test/controller/content_type_test.rb
@@ -46,7 +46,7 @@ def render_change_for_rxml
def render_default_content_types_for_respond_to
respond_to do |format|
format.html { render :text => "hello world!" }
- format.xml { render :action => "render_default_content_types_for_respond_to.rhtml" }
+ format.xml { render :action => "render_default_content_types_for_respond_to" }
format.js { render :text => "hello world!" }
format.rss { render :text => "hello world!", :content_type => Mime::XML }
end
View
51 actionpack/test/controller/mime_responds_test.rb
@@ -79,29 +79,20 @@ def custom_type_handling
end
end
- def custom_constant_handling
- Mime::Type.register("text/x-mobile", :mobile)
+ Mime::Type.register("text/x-mobile", :mobile)
+ def custom_constant_handling
respond_to do |type|
type.html { render :text => "HTML" }
type.mobile { render :text => "Mobile" }
end
- ensure
- Mime::SET.delete(:mobile)
- Mime.module_eval { remove_const :MOBILE if const_defined?(:MOBILE) }
end
def custom_constant_handling_without_block
- Mime::Type.register("text/x-mobile", :mobile)
-
respond_to do |type|
type.html { render :text => "HTML" }
type.mobile
end
-
- ensure
- Mime::SET.delete(:mobile)
- Mime.module_eval { remove_const :MOBILE if const_defined?(:MOBILE) }
end
def handle_any
@@ -125,32 +116,24 @@ def all_types_with_layout
end
end
+ Mime::Type.register_alias("text/html", :iphone)
+
def iphone_with_html_response_type
- Mime::Type.register_alias("text/html", :iphone)
request.format = :iphone if request.env["HTTP_ACCEPT"] == "text/iphone"
respond_to do |type|
type.html { @type = "Firefox" }
type.iphone { @type = "iPhone" }
end
-
- ensure
- Mime::SET.delete(:iphone)
- Mime.module_eval { remove_const :IPHONE if const_defined?(:IPHONE) }
end
def iphone_with_html_response_type_without_layout
- Mime::Type.register_alias("text/html", :iphone)
request.format = "iphone" if request.env["HTTP_ACCEPT"] == "text/iphone"
respond_to do |type|
type.html { @type = "Firefox"; render :action => "iphone_with_html_response_type" }
type.iphone { @type = "iPhone" ; render :action => "iphone_with_html_response_type" }
end
-
- ensure
- Mime::SET.delete(:iphone)
- Mime.module_eval { remove_const :IPHONE if const_defined?(:IPHONE) }
end
def rescue_action(e)
@@ -213,18 +196,20 @@ def test_xml
def test_js_or_html
@request.accept = "text/javascript, text/html"
- get :js_or_html
+ xhr :get, :js_or_html
assert_equal 'JS', @response.body
- get :html_or_xml
+ @request.accept = "text/javascript, text/html"
+ xhr :get, :html_or_xml
assert_equal 'HTML', @response.body
- get :just_xml
+ @request.accept = "text/javascript, text/html"
+ xhr :get, :just_xml
assert_response 406
end
def test_json_or_yaml
- get :json_or_yaml
+ xhr :get, :json_or_yaml
assert_equal 'JSON', @response.body
get :json_or_yaml, :format => 'json'
@@ -246,13 +231,13 @@ def test_json_or_yaml
def test_js_or_anything
@request.accept = "text/javascript, */*"
- get :js_or_html
+ xhr :get, :js_or_html
assert_equal 'JS', @response.body
- get :html_or_xml
+ xhr :get, :html_or_xml
assert_equal 'HTML', @response.body
- get :just_xml
+ xhr :get, :just_xml
assert_equal 'XML', @response.body
end
@@ -291,14 +276,16 @@ def test_using_defaults_with_type_list
end
def test_with_atom_content_type
+ @request.accept = ""
@request.env["CONTENT_TYPE"] = "application/atom+xml"
- get :made_for_content_type
+ xhr :get, :made_for_content_type
assert_equal "ATOM", @response.body
end
def test_with_rss_content_type
+ @request.accept = ""
@request.env["CONTENT_TYPE"] = "application/rss+xml"
- get :made_for_content_type
+ xhr :get, :made_for_content_type
assert_equal "RSS", @response.body
end
@@ -795,12 +782,8 @@ def index
protected
def with_iphone
- Mime::Type.register_alias("text/html", :iphone)
request.format = "iphone" if request.env["HTTP_ACCEPT"] == "text/iphone"
yield
- ensure
- Mime::SET.delete(:iphone)
- Mime.module_eval { remove_const :IPHONE if const_defined?(:IPHONE) }
end
end
View
6 actionpack/test/controller/render_js_test.rb
@@ -13,7 +13,7 @@ def greeting
# let's just rely on the template
end
- def partial
+ def show_partial
render :partial => 'partial'
end
end
@@ -33,7 +33,7 @@ def test_render_with_default_from_accept_header
end
def test_should_render_js_partial
- xhr :get, :partial, :format => 'js'
+ xhr :get, :show_partial, :format => 'js'
assert_equal 'partial js', @response.body
- end
+ end
end
View
2  actionpack/test/dispatch/mime_type_test.rb
@@ -56,7 +56,7 @@ class MimeTypeTest < ActiveSupport::TestCase
test "type convenience methods" do
# Don't test Mime::ALL, since it Mime::ALL#html? == true
- types = Mime::SET.symbols.uniq - [:all]
+ types = Mime::SET.symbols.uniq - [:all, :iphone]
# Remove custom Mime::Type instances set in other tests, like Mime::GIF and Mime::IPHONE
types.delete_if { |type| !Mime.const_defined?(type.to_s.upcase) }
View
63 actionpack/test/dispatch/request_test.rb
@@ -366,12 +366,12 @@ def teardown
end
test "XMLHttpRequest" do
- with_accept_header false do
- request = stub_request 'HTTP_X_REQUESTED_WITH' => 'XMLHttpRequest'
- request.expects(:parameters).at_least_once.returns({})
- assert request.xhr?
- assert_equal Mime::JS, request.format
- end
+ request = stub_request 'HTTP_X_REQUESTED_WITH' => 'XMLHttpRequest',
+ 'HTTP_ACCEPT' =>
+ [Mime::JS, Mime::HTML, Mime::XML, 'text/xml', Mime::ALL].join(",")
+ request.expects(:parameters).at_least_once.returns({})
+ assert request.xhr?
+ assert_equal Mime::JS, request.format
end
test "content type" do
@@ -420,37 +420,34 @@ def teardown
end
test "formats with accept header" do
- with_accept_header true do
- request = stub_request 'HTTP_ACCEPT' => 'text/html'
- request.expects(:parameters).at_least_once.returns({})
- assert_equal [ Mime::HTML ], request.formats
-
- request = stub_request 'CONTENT_TYPE' => 'application/xml; charset=UTF-8'
- request.expects(:parameters).at_least_once.returns({})
- assert_equal with_set(Mime::XML, Mime::HTML, Mime::ALL), request.formats
- end
+ request = stub_request 'HTTP_ACCEPT' => 'text/html'
+ request.expects(:parameters).at_least_once.returns({})
+ assert_equal [ Mime::HTML ], request.formats
- with_accept_header false do
- request = stub_request
- request.expects(:parameters).at_least_once.returns({ :format => :txt })
- assert_equal with_set(Mime::TEXT), request.formats
- end
+ request = stub_request 'CONTENT_TYPE' => 'application/xml; charset=UTF-8',
+ 'HTTP_X_REQUESTED_WITH' => "XMLHttpRequest"
+ request.expects(:parameters).at_least_once.returns({})
+ assert_equal with_set(Mime::XML), request.formats
+
+ request = stub_request
+ request.expects(:parameters).at_least_once.returns({ :format => :txt })
+ assert_equal with_set(Mime::TEXT), request.formats
end
test "negotiate_mime" do
- with_accept_header true do
- request = stub_request 'HTTP_ACCEPT' => 'text/html'
- request.expects(:parameters).at_least_once.returns({})
-
- assert_equal nil, request.negotiate_mime([Mime::XML, Mime::JSON])
- assert_equal Mime::HTML, request.negotiate_mime([Mime::XML, Mime::HTML])
- assert_equal Mime::HTML, request.negotiate_mime([Mime::XML, Mime::ALL])
-
- request = stub_request 'CONTENT_TYPE' => 'application/xml; charset=UTF-8'
- request.expects(:parameters).at_least_once.returns({})
- assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV])
- assert_equal Mime::CSV, request.negotiate_mime([Mime::CSV, Mime::YAML])
- end
+ request = stub_request 'HTTP_ACCEPT' => 'text/html',
+ 'HTTP_X_REQUESTED_WITH' => "XMLHttpRequest"
+
+ request.expects(:parameters).at_least_once.returns({})
+
+ assert_equal nil, request.negotiate_mime([Mime::XML, Mime::JSON])
+ assert_equal Mime::HTML, request.negotiate_mime([Mime::XML, Mime::HTML])
+ assert_equal Mime::HTML, request.negotiate_mime([Mime::XML, Mime::ALL])
+
+ request = stub_request 'CONTENT_TYPE' => 'application/xml; charset=UTF-8',
+ 'HTTP_X_REQUESTED_WITH' => "XMLHttpRequest"
+ request.expects(:parameters).at_least_once.returns({})
+ assert_equal Mime::XML, request.negotiate_mime([Mime::XML, Mime::CSV])
end
protected
View
1  actionpack/test/fixtures/content_type/render_default_content_types_for_respond_to.rhtml
@@ -1 +0,0 @@
-<hello>world</hello>
View
20 actionpack/test/lib/fixture_template.rb
@@ -4,7 +4,7 @@ def initialize(hash = {}, options = {})
super(options)
@hash = hash
end
-
+
def find_templates(name, details, prefix, partial)
if regexp = details_to_regexp(name, details, prefix, partial)
cached(regexp) do
@@ -16,26 +16,26 @@ def find_templates(name, details, prefix, partial)
end
end
end
-
+
private
-
+
def formats_regexp
@formats_regexp ||= begin
formats = Mime::SET.symbols
'(?:' + formats.map { |l| "\\.#{Regexp.escape(l.to_s)}" }.join('|') + ')?'
end
end
-
+
def handler_regexp
e = TemplateHandlers.extensions.map{|h| "\\.#{Regexp.escape(h.to_s)}"}.join("|")
- "(?:#{e})?"
+ "(?:#{e})"
end
-
+
def details_to_regexp(name, details, prefix, partial)
path = ""
path << "#{prefix}/" unless prefix.empty?
path << (partial ? "_#{name}" : name)
-
+
extensions = ""
[:locales, :formats].each do |k|
extensions << if exts = details[k]
@@ -47,7 +47,7 @@ def details_to_regexp(name, details, prefix, partial)
%r'^#{Regexp.escape(path)}#{extensions}#{handler_regexp}$'
end
-
+
# TODO: fix me
# :api: plugin
def path_to_details(path)
@@ -56,10 +56,10 @@ def path_to_details(path)
partial = m[1] == '_'
details = (m[2]||"").split('.').reject { |e| e.empty? }
handler = Template.handler_class_for_extension(m[3])
-
+
format = Mime[details.last] && details.pop.to_sym
locale = details.last && details.pop.to_sym
-
+
return handler, :format => format, :locale => locale, :partial => partial
end
end
View
4 actionpack/test/new_base/content_type_test.rb
@@ -75,7 +75,7 @@ class ImpliedContentTypeTest < SimpleRouteCase
end
test "sets Content-Type as application/xml when rendering *.xml.erb" do
- get "/content_type/implied/i_am_xml_erb"
+ get "/content_type/implied/i_am_xml_erb", "format" => "xml"
assert_header "Content-Type", "application/xml; charset=utf-8"
end
@@ -87,7 +87,7 @@ class ImpliedContentTypeTest < SimpleRouteCase
end
test "sets Content-Type as application/xml when rendering *.xml.builder" do
- get "/content_type/implied/i_am_xml_builder"
+ get "/content_type/implied/i_am_xml_builder", "format" => "xml"
assert_header "Content-Type", "application/xml; charset=utf-8"
end
View
2  actionpack/test/new_base/render_layout_test.rb
@@ -83,7 +83,7 @@ class MismatchFormatTest < SimpleRouteCase
testing ControllerLayouts::MismatchFormatController
test "if JS is selected, an HTML template is not also selected" do
- get :index
+ get :index, "format" => "js"
assert_response "$(\"test\").omg();"
end
View
7 actionpack/test/new_base/render_rjs_test.rb
@@ -21,24 +21,23 @@ def index
def index_locale
old_locale, I18n.locale = I18n.locale, :da
end
-
end
class TestBasic < SimpleRouteCase
testing BasicController
test "rendering a partial in an RJS template should pick the JS template over the HTML one" do
- get :index
+ get :index, "format" => "js"
assert_response("$(\"customer\").update(\"JS Partial\");")
end
test "replacing an element with a partial in an RJS template should pick the HTML template over the JS one" do
- get :index_html
+ get :index_html, "format" => "js"
assert_response("$(\"customer\").update(\"HTML Partial\");")
end
test "replacing an element with a partial in an RJS template with a locale should pick the localed HTML template" do
- get :index_locale, :format => :js
+ get :index_locale, "format" => "js"
assert_response("$(\"customer\").update(\"Danish HTML Partial\");")
end
View
2  actionpack/test/new_base/render_template_test.rb
@@ -73,7 +73,7 @@ class TestWithoutLayout < SimpleRouteCase
end
test "rendering a builder template" do
- get :builder_template
+ get :builder_template, "format" => "xml"
assert_response "<html>\n <p>Hello</p>\n</html>\n"
end
end
View
4 actionpack/test/template/javascript_helper_test.rb
@@ -7,6 +7,10 @@ def _evaluate_assigns_and_ivars() end
attr_accessor :formats, :output_buffer
+ def reset_formats(format)
+ @format = format
+ end
+
def setup
super
@template = self
View
4 actionpack/test/template/prototype_helper_test.rb
@@ -36,6 +36,10 @@ class Author::Nested < Author; end
class PrototypeHelperBaseTest < ActionView::TestCase
attr_accessor :formats, :output_buffer
+ def reset_formats(format)
+ @format = format
+ end
+
def setup
super
@template = self
View
2  actionpack/test/template/render_test.rb
@@ -190,6 +190,8 @@ def test_render_partial_and_fallback_to_layout
def test_render_missing_xml_partial_and_raise_missing_template
@view.formats = [:xml]
assert_raise(ActionView::MissingTemplate) { @view.render(:partial => "test/layout_for_partial") }
+ ensure
+ @view.formats = nil
end
def test_render_inline
Please sign in to comment.
Something went wrong with that request. Please try again.