From de79525d045b6ea2d83f325bdeeb9380510d045c Mon Sep 17 00:00:00 2001 From: wycats Date: Mon, 8 Mar 2010 21:08:31 -0800 Subject: Get rid of the instance-level URL rewriter --- actionpack/lib/action_controller/test_case.rb | 8 +++-- actionpack/lib/action_controller/url_rewriter.rb | 40 ++------------------- actionpack/test/controller/caching_test.rb | 2 +- actionpack/test/controller/url_rewriter_test.rb | 44 ++++++++---------------- actionpack/test/template/url_helper_test.rb | 2 -- 5 files changed, 25 insertions(+), 71 deletions(-) diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb index 7e0a833dfa..c43e560ac1 100644 --- a/actionpack/lib/action_controller/test_case.rb +++ b/actionpack/lib/action_controller/test_case.rb @@ -339,9 +339,13 @@ module ActionController def build_request_uri(action, parameters) unless @request.env["PATH_INFO"] options = @controller.__send__(:url_options).merge(parameters) - options.update(:only_path => true, :action => action, :relative_url_root => nil) - rewriter = ActionController::UrlRewriter.new(@request, parameters) + options.update( + :only_path => true, + :action => action, + :relative_url_root => nil, + :_path_segments => @request.symbolized_path_parameters) + rewriter = ActionController::UrlRewriter url, query_string = rewriter.rewrite(@router, options).split("?", 2) @request.env["SCRIPT_NAME"] = @controller.config.relative_url_root diff --git a/actionpack/lib/action_controller/url_rewriter.rb b/actionpack/lib/action_controller/url_rewriter.rb index 981865517f..b387d49593 100644 --- a/actionpack/lib/action_controller/url_rewriter.rb +++ b/actionpack/lib/action_controller/url_rewriter.rb @@ -2,37 +2,16 @@ require 'active_support/core_ext/hash/except' module ActionController # Rewrites URLs for Base.redirect_to and Base.url_for in the controller. - class UrlRewriter #:nodoc: + module UrlRewriter #:nodoc: RESERVED_OPTIONS = [:anchor, :params, :only_path, :host, :protocol, :port, :trailing_slash, :skip_relative_url_root] - def initialize(request, parameters) - @request, @parameters = request, parameters - end - - def rewrite(router, options = {}) - options[:host] ||= @request.host_with_port - options[:protocol] ||= @request.protocol - - self.class.rewrite(router, options, @request.symbolized_path_parameters) do |options| - process_path_options(options) - end - end - - def to_str - "#{@request.protocol}, #{@request.host_with_port}, #{@request.path}, #{@parameters[:controller]}, #{@parameters[:action]}, #{@request.parameters.inspect}" - end - - alias_method :to_s, :to_str - # ROUTES TODO: Class method code smell - def self.rewrite(router, options, path_segments=nil) + def self.rewrite(router, options) handle_positional_args(options) rewritten_url = "" - # ROUTES TODO: Fix the tests - segments = options.delete(:_path_segments) - path_segments = path_segments ? path_segments.merge(segments || {}) : segments + path_segments = options.delete(:_path_segments) unless options[:only_path] rewritten_url << (options[:protocol] || "http") @@ -81,18 +60,5 @@ module ActionController end end - # Given a Hash of options, generates a route - def process_path_options(options) - options = options.symbolize_keys - options.update(options[:params].symbolize_keys) if options[:params] - - if (overwrite = options.delete(:overwrite_params)) - options.update(@parameters.symbolize_keys) - options.update(overwrite.symbolize_keys) - end - - options - end - end end diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 11ef3cdd92..2be91893d4 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -64,7 +64,7 @@ class PageCachingTest < ActionController::TestCase @controller.cache_store = :file_store, FILE_STORE_PATH @params = {:controller => 'posts', :action => 'index', :only_path => true, :skip_relative_url_root => true} - @rewriter = ActionController::UrlRewriter.new(@request, @params) + @rewriter = ActionController::UrlRewriter FileUtils.rm_rf(File.dirname(FILE_STORE_PATH)) FileUtils.mkdir_p(FILE_STORE_PATH) diff --git a/actionpack/test/controller/url_rewriter_test.rb b/actionpack/test/controller/url_rewriter_test.rb index 199b824a7a..3bc8bec3fb 100644 --- a/actionpack/test/controller/url_rewriter_test.rb +++ b/actionpack/test/controller/url_rewriter_test.rb @@ -4,10 +4,24 @@ require 'controller/fake_controllers' ActionController::UrlRewriter class UrlRewriterTests < ActionController::TestCase + class Rewriter + def initialize(request) + @options = { + :host => request.host_with_port, + :protocol => request.protocol + } + end + + def rewrite(router, options) + options = @options.merge(options) + ActionController::UrlRewriter.rewrite(router, options) + end + end + def setup @request = ActionController::TestRequest.new @params = {} - @rewriter = ActionController::UrlRewriter.new(@request, @params) + @rewriter = Rewriter.new(@request) #.new(@request, @params) end def test_port @@ -61,34 +75,6 @@ class UrlRewriterTests < ActionController::TestCase ) end - def test_overwrite_params - @params[:controller] = 'hi' - @params[:action] = 'bye' - @params[:id] = '2' - - assert_equal '/hi/hi/2', @rewriter.rewrite(@router, :only_path => true, :overwrite_params => {:action => 'hi'}) - u = @rewriter.rewrite(@router, :only_path => false, :overwrite_params => {:action => 'hi'}) - assert_match %r(/hi/hi/2$), u - end - - def test_overwrite_removes_original - @params[:controller] = 'search' - @params[:action] = 'list' - @params[:list_page] = 1 - - assert_equal '/search/list?list_page=2', @rewriter.rewrite(@router, :only_path => true, :overwrite_params => {"list_page" => 2}) - u = @rewriter.rewrite(@router, :only_path => false, :overwrite_params => {:list_page => 2}) - assert_equal 'http://test.host/search/list?list_page=2', u - end - - def test_to_str - @params[:controller] = 'hi' - @params[:action] = 'bye' - @request.parameters[:id] = '2' - - assert_equal 'http://, test.host, /, hi, bye, {"id"=>"2"}', @rewriter.to_str - end - def test_trailing_slash options = {:controller => 'foo', :action => 'bar', :id => '3', :only_path => true} assert_equal '/foo/bar/3', @rewriter.rewrite(@router, options) diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index 38ab5521fe..165cb655da 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -122,7 +122,6 @@ class UrlHelperTest < ActionView::TestCase url = {:controller => 'weblog', :action => 'show'} @controller = ActionController::Base.new @controller.request = ActionController::TestRequest.new - @controller.url = ActionController::UrlRewriter.new(@controller.request, url) assert_dom_equal(%q{Test Link}, link_to('Test Link', url)) end @@ -131,7 +130,6 @@ class UrlHelperTest < ActionView::TestCase url = {:controller => 'weblog', :action => 'show', :host => 'www.example.com'} @controller = ActionController::Base.new @controller.request = ActionController::TestRequest.new - @controller.url = ActionController::UrlRewriter.new(@controller.request, url) assert_dom_equal(%q{Test Link}, link_to('Test Link', url)) end -- cgit v1.2.3 From 9444ac9312470696b6a5d73cd0044329211ec4c6 Mon Sep 17 00:00:00 2001 From: wycats Date: Tue, 9 Mar 2010 10:20:48 -0800 Subject: Refactor the RouteSet so it uses a Generator object instead of one huge method. --- actionpack/lib/action_controller/metal/url_for.rb | 1 - .../lib/action_dispatch/routing/route_set.rb | 196 ++++++++++++--------- 2 files changed, 109 insertions(+), 88 deletions(-) diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index f6b6cb2ff4..10c7ca9021 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -3,7 +3,6 @@ module ActionController extend ActiveSupport::Concern include ActionDispatch::Routing::UrlFor - include ActionController::RackDelegation def url_options super.reverse_merge( diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 8ebad1a9b8..e99e39269b 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -276,116 +276,138 @@ module ActionDispatch route end - def options_as_params(options) - # If an explicit :controller was given, always make :action explicit - # too, so that action expiry works as expected for things like - # - # generate({:controller => 'content'}, {:controller => 'content', :action => 'show'}) - # - # (the above is from the unit tests). In the above case, because the - # controller was explicitly given, but no action, the action is implied to - # be "index", not the recalled action of "show". - # - # great fun, eh? - - options_as_params = options.clone - options_as_params[:action] ||= 'index' if options[:controller] - options_as_params[:action] = options_as_params[:action].to_s if options_as_params[:action] - options_as_params - end + class Generator + attr_reader :options, :recall, :set, :script_name, :named_route + + def initialize(options, recall, set, extras = false) + @script_name = options.delete(:script_name) + @named_route = options.delete(:use_route) + @options = options.dup + @recall = recall.dup + @set = set + @extras = extras + + normalize_options! + normalize_controller_action_id! + use_relative_controller! + controller.sub!(%r{^/}, '') if controller + handle_nil_action! + end - def build_expiry(options, recall) - recall.inject({}) do |expiry, (key, recalled_value)| - expiry[key] = (options.key?(key) && options[key].to_param != recalled_value.to_param) - expiry + def controller + @controller ||= @options[:controller] end - end - # Generate the path indicated by the arguments, and return an array of - # the keys that were not used to generate it. - def extra_keys(options, recall={}) - generate_extras(options, recall).last - end + def current_controller + @recall[:controller] + end - def generate_extras(options, recall={}) - generate(options, recall, :generate_extras) - end + def use_recall_for(key) + if @recall[key] && (!@options.key?(key) || @options[key] == @recall[key]) + @options[key] = @recall.delete(key) + end + end - def generate(options, recall = {}, method = :generate) - options, recall = options.dup, recall.dup - script_name = options.delete(:script_name) - named_route = options.delete(:use_route) + def normalize_options! + # If an explicit :controller was given, always make :action explicit + # too, so that action expiry works as expected for things like + # + # generate({:controller => 'content'}, {:controller => 'content', :action => 'show'}) + # + # (the above is from the unit tests). In the above case, because the + # controller was explicitly given, but no action, the action is implied to + # be "index", not the recalled action of "show". - options = options_as_params(options) - expire_on = build_expiry(options, recall) + if options[:controller] + options[:action] ||= 'index' + options[:controller] = options[:controller].to_s + end - recall[:action] ||= 'index' if options[:controller] || recall[:controller] + if options[:action] + options[:action] = options[:action].to_s + end + end - if recall[:controller] && (!options.has_key?(:controller) || options[:controller] == recall[:controller]) - options[:controller] = recall.delete(:controller) + # This pulls :controller, :action, and :id out of the recall. + # The recall key is only used if there is no key in the options + # or if the key in the options is identical. If any of + # :controller, :action or :id is not found, don't pull any + # more keys from the recall. + def normalize_controller_action_id! + @recall[:action] ||= 'index' if current_controller + + use_recall_for(:controller) or return + use_recall_for(:action) or return + use_recall_for(:id) + end - if recall[:action] && (!options.has_key?(:action) || options[:action] == recall[:action]) - options[:action] = recall.delete(:action) + # if the current controller is "foo/bar/baz" and :controller => "baz/bat" + # is specified, the controller becomes "foo/baz/bat" + def use_relative_controller! + if !named_route && different_controller? + old_parts = current_controller.split('/') + size = controller.count("/") + 1 + parts = old_parts[0...-size] << controller + @controller = @options[:controller] = parts.join("/") + end + end - if recall[:id] && (!options.has_key?(:id) || options[:id] == recall[:id]) - options[:id] = recall.delete(:id) - end + # This handles the case of :action => nil being explicitly passed. + # It is identical to :action => "index" + def handle_nil_action! + if options.has_key?(:action) && options[:action].nil? + options[:action] = 'index' end + recall[:action] = options.delete(:action) if options[:action] == 'index' end - options[:controller] = options[:controller].to_s if options[:controller] + def generate + error = ActionController::RoutingError.new("No route matches #{options.inspect}") + path, params = @set.generate(:path_info, named_route, options, recall, opts) - if !named_route && expire_on[:controller] && options[:controller] && options[:controller][0] != ?/ - old_parts = recall[:controller].split('/') - new_parts = options[:controller].split('/') - parts = old_parts[0..-(new_parts.length + 1)] + new_parts - options[:controller] = parts.join('/') - end + raise error unless path - options[:controller] = options[:controller][1..-1] if options[:controller] && options[:controller][0] == ?/ + params.reject! {|k,v| !v } - merged = options.merge(recall) - if options.has_key?(:action) && options[:action].nil? - options.delete(:action) - recall[:action] = 'index' - end - recall[:action] = options.delete(:action) if options[:action] == 'index' - - opts = {} - opts[:parameterize] = lambda { |name, value| - if name == :controller - value - elsif value.is_a?(Array) - value.map { |v| Rack::Mount::Utils.escape_uri(v.to_param) }.join('/') - else - Rack::Mount::Utils.escape_uri(value.to_param) - end - } + return [path, params.keys] if @extras - unless result = @set.generate(:path_info, named_route, options, recall, opts) - raise ActionController::RoutingError, "No route matches #{options.inspect}" + path << "?#{params.to_query}" if params.any? + "#{script_name}#{path}" + rescue Rack::Mount::RoutingError + raise error end - path, params = result - params.each do |k, v| - if v - params[k] = v - else - params.delete(k) + def opts + parameterize = lambda do |name, value| + if name == :controller + value + elsif value.is_a?(Array) + value.map { |v| Rack::Mount::Utils.escape_uri(v.to_param) }.join('/') + else + Rack::Mount::Utils.escape_uri(value.to_param) + end end + {:parameterize => parameterize} end - if path && method == :generate_extras - [path, params.keys] - elsif path - path << "?#{params.to_query}" if params.any? - path = "#{script_name}#{path}" - path - else - raise ActionController::RoutingError, "No route matches #{options.inspect}" + def different_controller? + return false unless current_controller + controller.to_param != current_controller.to_param end - rescue Rack::Mount::RoutingError - raise ActionController::RoutingError, "No route matches #{options.inspect}" + end + + # Generate the path indicated by the arguments, and return an array of + # the keys that were not used to generate it. + def extra_keys(options, recall={}) + generate_extras(options, recall).last + end + + def generate_extras(options, recall={}) + generate(options, recall, true) + end + + def generate(options, recall = {}, extras = false) + Generator.new(options, recall, @set, extras).generate end def call(env) -- cgit v1.2.3 From ea4f8ef33f08a69a68f6b95f392e63ea9cc13602 Mon Sep 17 00:00:00 2001 From: Justin Ko Date: Tue, 9 Mar 2010 16:37:46 -0200 Subject: Reinstate dom_id in controllers. [#3040 state:committed] Signed-off-by: Santiago Pastorino Signed-off-by: Jeremy Kemper --- actionpack/lib/action_controller/base.rb | 1 + actionpack/test/controller/base_test.rb | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index d00afa6d4e..fcd3cb9bd3 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -29,6 +29,7 @@ module ActionController include ActionController::Verification include ActionController::RequestForgeryProtection include ActionController::Streaming + include ActionController::RecordIdentifier include ActionController::HttpAuthentication::Basic::ControllerMethods include ActionController::HttpAuthentication::Digest::ControllerMethods diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb index 3e9f8ae048..f047e7da30 100644 --- a/actionpack/test/controller/base_test.rb +++ b/actionpack/test/controller/base_test.rb @@ -76,6 +76,9 @@ class UrlOptionsController < ActionController::Base end end +class RecordIdentifierController < ActionController::Base +end + class ControllerClassTests < ActiveSupport::TestCase def test_controller_path assert_equal 'empty', EmptyController.controller_path @@ -102,6 +105,11 @@ class ControllerClassTests < ActiveSupport::TestCase assert_equal [:password], parameters end + + def test_record_identifier + assert_respond_to RecordIdentifierController.new, :dom_id + assert_respond_to RecordIdentifierController.new, :dom_class + end end class ControllerInstanceTests < Test::Unit::TestCase -- cgit v1.2.3 From a87683fb38d6cf66f39a7bd3faa6c969c63b1f46 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Tue, 9 Mar 2010 11:06:31 -0800 Subject: Disprefer JSONGem decoder since it only decodes JSON objects --- activesupport/lib/active_support/json/decoding.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activesupport/lib/active_support/json/decoding.rb b/activesupport/lib/active_support/json/decoding.rb index e357b6837a..04ff316a44 100644 --- a/activesupport/lib/active_support/json/decoding.rb +++ b/activesupport/lib/active_support/json/decoding.rb @@ -7,7 +7,7 @@ module ActiveSupport module JSON # Listed in order of preference. - DECODERS = %w(Yajl JSONGem Yaml) + DECODERS = %w(Yajl Yaml) class << self attr_reader :parse_error -- cgit v1.2.3