Permalink
Browse files

Added optimal formatted routes to rails, deprecating the formatted_* …

…methods, and reducing routes creation by 50% [#1359 state:committed]

Signed-off-by: David Heinemeier Hansson <david@loudthinking.com>
  • Loading branch information...
1 parent 6599dd9 commit fef6c32afe2276dffa0347e25808a86e7a101af1 @aaronbatalion aaronbatalion committed with dhh Nov 24, 2008
View
@@ -1,5 +1,7 @@
*2.3.0 [Edge]*
+* Dropped formatted_* routes in favor of just passing in :format as an option. This cuts resource routes generation in half #1359 [aaronbatalion]
+
* Remove support for old double-encoded cookies from the cookie store. These values haven't been generated since before 2.1.0, and any users who have visited the app in the intervening 6 months will have had their cookie upgraded. [Koz]
* Allow helpers directory to be overridden via ActionController::Base.helpers_dir #1424 [Sam Pohlenz]
@@ -639,10 +639,8 @@ def map_resource_routes(map, resource, action, route_path, route_name = nil, met
formatted_route_path = "#{route_path}.:format"
if route_name && @set.named_routes[route_name.to_sym].nil?
- map.named_route(route_name, route_path, action_options)
- map.named_route("formatted_#{route_name}", formatted_route_path, action_options)
+ map.named_route(route_name, formatted_route_path, action_options)
else
- map.connect(route_path, action_options)
map.connect(formatted_route_path, action_options)
end
end
@@ -34,6 +34,8 @@ def segments_for_route_path(path)
def segment_for(string)
segment =
case string
+ when /\A\.(:format)?\//
+ OptionalFormatSegment.new
when /\A:(\w+)/
key = $1.to_sym
key == :controller ? ControllerSegment.new(key) : DynamicSegment.new(key)
@@ -65,7 +65,7 @@ def applicable?
# rather than triggering the expensive logic in +url_for+.
class PositionalArguments < Optimiser
def guard_conditions
- number_of_arguments = route.segment_keys.size
+ number_of_arguments = route.required_segment_keys.size
# if they're using foo_url(:id=>2) it's one
# argument, but we don't want to generate /foos/id2
if number_of_arguments == 1
@@ -35,6 +35,11 @@ def segment_keys
segment.key if segment.respond_to? :key
end.compact
end
+
+ def required_segment_keys
+ required_segments = segments.select {|seg| (!seg.optional? && !seg.is_a?(DividerSegment)) || seg.is_a?(PathSegment) }
+ required_segments.collect { |seg| seg.key if seg.respond_to?(:key)}.compact
+ end
# Build a query string from the keys of the given hash. If +only_keys+
# is given (as an array), only the keys indicated will be used to build
@@ -185,6 +185,14 @@ def #{selector}(*args)
end
url_for(#{hash_access_method}(opts))
+
+ end
+ #Add an alias to support the now deprecated formatted_* URL.
+ def formatted_#{selector}(*args)
+ ActiveSupport::Deprecation.warn(
+ "formatted_#{selector}() has been deprecated. please pass format to the standard" +
@lukemelia

lukemelia Nov 29, 2008

Contributor

FYI, missing a space between “standard” and the route method name in this deprecation warning text.

+ "#{selector}() method instead.", caller)
+ #{selector}(*args)
end
protected :#{selector}
end_eval
@@ -361,7 +369,7 @@ def generate(options, recall = {}, method=:generate)
end
# don't use the recalled keys when determining which routes to check
- routes = routes_by_controller[controller][action][options.keys.sort_by { |x| x.object_id }]
+ routes = routes_by_controller[controller][action][options.reject {|k,v| !v}.keys.sort_by { |x| x.object_id }]
routes.each do |route|
results = route.__send__(method, options, merged, expire_on)
@@ -308,5 +308,36 @@ def self.new_escaped(strings)
end
end
end
+
+ # The OptionalFormatSegment allows for any resource route to have an optional
+ # :format, which decreases the amount of routes created by 50%.
+ class OptionalFormatSegment < DynamicSegment
+
+ def initialize(key = nil, options = {})
+ super(:format, {:optional => true}.merge(options))
+ end
+
+ def interpolation_chunk
+ "." + super
+ end
+
+ def regexp_chunk
+ '(\.[^/?\.]+)?'
+ end
+
+ def to_s
+ '(.:format)?'
+ end
+
+ #the value should not include the period (.)
+ def match_extraction(next_capture)
+ %[
+ if (m = match[#{next_capture}])
+ params[:#{key}] = URI.unescape(m.from(1))
+ end
+ ]
+ end
+ end
+
end
end
@@ -187,7 +187,7 @@ def test_with_collection_action_and_name_prefix_and_formatted
assert_restful_named_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options|
actions.keys.each do |action|
- assert_named_route "/threads/1/messages/#{action}.xml", "formatted_#{action}_thread_messages_path", :action => action, :format => 'xml'
+ assert_named_route "/threads/1/messages/#{action}.xml", "#{action}_thread_messages_path", :action => action, :format => 'xml'
end
end
end
@@ -316,7 +316,7 @@ def test_with_formatted_new_action_with_name_prefix
end
assert_restful_named_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options|
- assert_named_route preview_path, :formatted_preview_new_thread_message_path, preview_options
+ assert_named_route preview_path, :preview_new_thread_message_path, preview_options
end
end
end
@@ -1130,14 +1130,14 @@ def assert_restful_named_routes_for(controller_name, singular_name = nil, option
end
assert_named_route "#{full_path}", "#{name_prefix}#{controller_name}_path", options[:options]
- assert_named_route "#{full_path}.xml", "formatted_#{name_prefix}#{controller_name}_path", options[:options].merge(:format => 'xml')
+ assert_named_route "#{full_path}.xml", "#{name_prefix}#{controller_name}_path", options[:options].merge(:format => 'xml')
assert_named_route "#{shallow_path}/1", "#{shallow_prefix}#{singular_name}_path", options[:shallow_options].merge(:id => '1')
- assert_named_route "#{shallow_path}/1.xml", "formatted_#{shallow_prefix}#{singular_name}_path", options[:shallow_options].merge(:id => '1', :format => 'xml')
+ assert_named_route "#{shallow_path}/1.xml", "#{shallow_prefix}#{singular_name}_path", options[:shallow_options].merge(:id => '1', :format => 'xml')
assert_named_route "#{full_path}/#{new_action}", "new_#{name_prefix}#{singular_name}_path", options[:options]
- assert_named_route "#{full_path}/#{new_action}.xml", "formatted_new_#{name_prefix}#{singular_name}_path", options[:options].merge(:format => 'xml')
+ assert_named_route "#{full_path}/#{new_action}.xml", "new_#{name_prefix}#{singular_name}_path", options[:options].merge(:format => 'xml')
assert_named_route "#{shallow_path}/1/#{edit_action}", "edit_#{shallow_prefix}#{singular_name}_path", options[:shallow_options].merge(:id => '1')
- assert_named_route "#{shallow_path}/1/#{edit_action}.xml", "formatted_edit_#{shallow_prefix}#{singular_name}_path", options[:shallow_options].merge(:id => '1', :format => 'xml')
+ assert_named_route "#{shallow_path}/1/#{edit_action}.xml", "edit_#{shallow_prefix}#{singular_name}_path", options[:shallow_options].merge(:id => '1', :format => 'xml')
yield options[:options] if block_given?
end
@@ -1189,12 +1189,12 @@ def assert_singleton_named_routes_for(singleton_name, options = {})
name_prefix = options[:name_prefix]
assert_named_route "#{full_path}", "#{name_prefix}#{singleton_name}_path", options[:options]
- assert_named_route "#{full_path}.xml", "formatted_#{name_prefix}#{singleton_name}_path", options[:options].merge(:format => 'xml')
+ assert_named_route "#{full_path}.xml", "#{name_prefix}#{singleton_name}_path", options[:options].merge(:format => 'xml')
assert_named_route "#{full_path}/new", "new_#{name_prefix}#{singleton_name}_path", options[:options]
- assert_named_route "#{full_path}/new.xml", "formatted_new_#{name_prefix}#{singleton_name}_path", options[:options].merge(:format => 'xml')
+ assert_named_route "#{full_path}/new.xml", "new_#{name_prefix}#{singleton_name}_path", options[:options].merge(:format => 'xml')
assert_named_route "#{full_path}/edit", "edit_#{name_prefix}#{singleton_name}_path", options[:options]
- assert_named_route "#{full_path}/edit.xml", "formatted_edit_#{name_prefix}#{singleton_name}_path", options[:options].merge(:format => 'xml')
+ assert_named_route "#{full_path}/edit.xml", "edit_#{name_prefix}#{singleton_name}_path", options[:options].merge(:format => 'xml')
end
def assert_named_route(expected, route, options)
@@ -301,6 +301,41 @@ def test_path_generation_for_symbol_parameter_keys
assert_generates("/image", :controller=> :image)
end
+ def test_named_routes_with_nil_keys
+ add_host!
+ ActionController::Routing::Routes.draw do |map|
+ map.main '', :controller => 'posts'
+ map.resources :posts
+ map.connect ':controller/:action/:id'
+ end
+ # We need to create a new class in order to install the new named route.
+ kls = Class.new { include ActionController::UrlWriter }
+ controller = kls.new
+ params = {:action => :index, :controller => :posts, :format => :xml}
+ assert_equal("http://www.basecamphq.com/posts.xml", controller.send(:url_for, params))
+ params[:format] = nil
+ assert_equal("http://www.basecamphq.com/", controller.send(:url_for, params))
+ ensure
+ ActionController::Routing::Routes.load!
+ end
+
+ def test_formatted_url_methods_are_deprecated
+ ActionController::Routing::Routes.draw do |map|
+ map.resources :posts
+ end
+ # We need to create a new class in order to install the new named route.
+ kls = Class.new { include ActionController::UrlWriter }
+ controller = kls.new
+ params = {:id => 1, :format => :xml}
+ assert_deprecated do
+ assert_equal("/posts/1.xml", controller.send(:formatted_post_path, params))
+ end
+ assert_deprecated do
+ assert_equal("/posts/1.xml", controller.send(:formatted_post_path, 1, :xml))
+ end
+ ensure
+ ActionController::Routing::Routes.load!
+ end
private
def extract_params(url)
url.split('?', 2).last.split('&')

0 comments on commit fef6c32

Please sign in to comment.