diff options
| -rwxr-xr-x | actionpack/lib/action_controller/base.rb | 8 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/resources.rb | 24 | ||||
| -rw-r--r-- | actionpack/lib/action_controller/routing.rb | 25 | ||||
| -rw-r--r-- | actionpack/test/controller/resources_test.rb | 104 | ||||
| -rw-r--r-- | actionpack/test/controller/routing_test.rb | 54 |
5 files changed, 77 insertions, 138 deletions
diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index e0474ff5b3..81db0ed467 100755 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -310,6 +310,10 @@ module ActionController #:nodoc: # Turn on +ignore_missing_templates+ if you want to unit test actions without making the associated templates. cattr_accessor :ignore_missing_templates + # Controls the resource action separator + @@resource_action_separator = "/" + cattr_accessor :resource_action_separator + # Holds the request object that's primarily used to get environment variables through access like # <tt>request.env["REQUEST_URI"]</tt>. attr_internal :request @@ -605,11 +609,11 @@ module ActionController #:nodoc: def controller_path self.class.controller_path end - + def session_enabled? request.session_options && request.session_options[:disabled] != false end - + # View load paths for controller. def view_paths self.class.view_paths diff --git a/actionpack/lib/action_controller/resources.rb b/actionpack/lib/action_controller/resources.rb index 8e7dedb86f..384f5f30d7 100644 --- a/actionpack/lib/action_controller/resources.rb +++ b/actionpack/lib/action_controller/resources.rb @@ -94,6 +94,9 @@ module ActionController "#{name_prefix}#{singular}_" end + def action_separator + @action_separator ||= Base.resource_action_separator + end protected def arrange_actions @@ -431,10 +434,8 @@ module ActionController resource.collection_methods.each do |method, actions| actions.each do |action| action_options = action_options_for(action, resource, method) - - # See http://dev.rubyonrails.org/ticket/8251 - map.deprecated_named_route("#{action}_#{resource.name_prefix}#{resource.plural}", "#{resource.name_prefix}#{action}_#{resource.plural}", "#{resource.path}/#{action}", action_options) - map.deprecated_named_route("formatted_#{action}_#{resource.name_prefix}#{resource.plural}", "formatted_#{resource.name_prefix}#{action}_#{resource.plural}", "#{resource.path}/#{action}.:format", action_options) + map.named_route("#{action}_#{resource.name_prefix}#{resource.plural}", "#{resource.path}#{resource.action_separator}#{action}", action_options) + map.named_route("formatted_#{action}_#{resource.name_prefix}#{resource.plural}", "#{resource.path}#{resource.action_separator}#{action}.:format", action_options) end end end @@ -460,13 +461,11 @@ module ActionController actions.each do |action| action_options = action_options_for(action, resource, method) if action == :new - # See http://dev.rubyonrails.org/ticket/8251 - map.deprecated_named_route("new_#{resource.name_prefix}#{resource.singular}", "#{resource.name_prefix}new_#{resource.singular}", resource.new_path, action_options) - map.deprecated_named_route("formatted_new_#{resource.name_prefix}#{resource.singular}", "formatted_#{resource.name_prefix}new_#{resource.singular}", "#{resource.new_path}.:format", action_options) + map.named_route("new_#{resource.name_prefix}#{resource.singular}", resource.new_path, action_options) + map.named_route("formatted_new_#{resource.name_prefix}#{resource.singular}", "#{resource.new_path}.:format", action_options) else - # See http://dev.rubyonrails.org/ticket/8251 - map.deprecated_named_route("#{action}_new_#{resource.name_prefix}#{resource.singular}", "#{resource.name_prefix}#{action}_new_#{resource.singular}", "#{resource.new_path}/#{action}", action_options) - map.deprecated_named_route("formatted_#{action}_new_#{resource.name_prefix}#{resource.singular}", "formatted_#{resource.name_prefix}#{action}_new_#{resource.singular}", "#{resource.new_path}/#{action}.:format", action_options) + map.named_route("#{action}_new_#{resource.name_prefix}#{resource.singular}", "#{resource.new_path}#{resource.action_separator}#{action}", action_options) + map.named_route("formatted_#{action}_new_#{resource.name_prefix}#{resource.singular}", "#{resource.new_path}#{resource.action_separator}#{action}.:format", action_options) end end end @@ -476,9 +475,8 @@ module ActionController resource.member_methods.each do |method, actions| actions.each do |action| action_options = action_options_for(action, resource, method) - # See http://dev.rubyonrails.org/ticket/8251 - map.deprecated_named_route("#{action}_#{resource.name_prefix}#{resource.singular}", "#{resource.name_prefix}#{action}_#{resource.singular}", "#{resource.member_path}/#{action}", action_options) - map.deprecated_named_route("formatted_#{action}_#{resource.name_prefix}#{resource.singular}", "formatted_#{resource.name_prefix}#{action}_#{resource.singular}", "#{resource.member_path}/#{action}.:format",action_options) + map.named_route("#{action}_#{resource.name_prefix}#{resource.singular}", "#{resource.member_path}#{resource.action_separator}#{action}", action_options) + map.named_route("formatted_#{action}_#{resource.name_prefix}#{resource.singular}", "#{resource.member_path}#{resource.action_separator}#{action}.:format",action_options) end end diff --git a/actionpack/lib/action_controller/routing.rb b/actionpack/lib/action_controller/routing.rb index 2565a6dc3c..4cf9b010ba 100644 --- a/actionpack/lib/action_controller/routing.rb +++ b/actionpack/lib/action_controller/routing.rb @@ -247,7 +247,7 @@ module ActionController # end # module Routing - SEPARATORS = %w( / . ? ) + SEPARATORS = %w( / ; . , ? ) HTTP_METHODS = [:get, :head, :post, :put, :delete] @@ -548,7 +548,7 @@ module ActionController end class Segment #:nodoc: - RESERVED_PCHAR = ':@&=+$,;' + RESERVED_PCHAR = ':@&=+$' UNSAFE_PCHAR = Regexp.new("[^#{URI::REGEXP::PATTERN::UNRESERVED}#{RESERVED_PCHAR}]", false, 'N').freeze attr_accessor :is_optional @@ -561,7 +561,7 @@ module ActionController def extraction_code nil end - + # Continue generating string for the prior segments. def continue_string_structure(prior_segments) if prior_segments.empty? @@ -1020,11 +1020,6 @@ module ActionController @set.add_named_route(name, path, options) end - def deprecated_named_route(name, deprecated_name, path, options = {}) - named_route(name, path, options) - @set.add_deprecated_named_route(name, deprecated_name, path, options) unless deprecated_name == name - end - # Enables the use of resources in a module by setting the name_prefix, path_prefix, and namespace for the model. # Example: # @@ -1057,7 +1052,7 @@ module ActionController class NamedRouteCollection #:nodoc: include Enumerable - attr_reader :routes, :helpers, :deprecated_named_routes + attr_reader :routes, :helpers def initialize clear! @@ -1066,7 +1061,6 @@ module ActionController def clear! @routes = {} @helpers = [] - @deprecated_named_routes = {} @module ||= Module.new @module.instance_methods.each do |selector| @@ -1080,12 +1074,6 @@ module ActionController end def get(name) - if @deprecated_named_routes.has_key?(name.to_sym) - ActiveSupport::Deprecation.warn( - "The named route \"#{name}\" uses a format that has been deprecated. " + - "You should use \"#{@deprecated_named_routes[name]}\" instead", caller - ) - end routes[name.to_sym] end @@ -1259,11 +1247,6 @@ module ActionController named_routes[name.to_sym] = add_route(path, options) end - def add_deprecated_named_route(name, deprecated_name, path, options = {}) - add_named_route(deprecated_name, path, options) - named_routes.deprecated_named_routes[deprecated_name.to_sym] = name - 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 diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb index c3e0836205..57b956e777 100644 --- a/actionpack/test/controller/resources_test.rb +++ b/actionpack/test/controller/resources_test.rb @@ -149,7 +149,6 @@ class ResourcesTest < Test::Unit::TestCase 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_deprecated_named_route /thread_#{action}_messages/, "/threads/1/messages/#{action}", "thread_#{action}_messages_path", :action => action assert_named_route "/threads/1/messages/#{action}", "#{action}_thread_messages_path", :action => action end end @@ -168,7 +167,6 @@ class ResourcesTest < Test::Unit::TestCase 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_deprecated_named_route /formatted_thread_#{action}_messages/, "/threads/1/messages/#{action}.xml", "formatted_thread_#{action}_messages_path", :action => action, :format => 'xml' assert_named_route "/threads/1/messages/#{action}.xml", "formatted_#{action}_thread_messages_path", :action => action, :format => 'xml' end end @@ -232,7 +230,6 @@ class ResourcesTest < Test::Unit::TestCase end assert_restful_named_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options| - assert_deprecated_named_route /thread_preview_new_message/, preview_path, :thread_preview_new_message_path, preview_options assert_named_route preview_path, :preview_new_thread_message_path, preview_options end end @@ -247,7 +244,6 @@ class ResourcesTest < Test::Unit::TestCase end assert_restful_named_routes_for :messages, :path_prefix => 'threads/1/', :name_prefix => 'thread_', :options => { :thread_id => '1' } do |options| - assert_deprecated_named_route /formatted_thread_preview_new_message/, preview_path, :formatted_thread_preview_new_message_path, preview_options assert_named_route preview_path, :formatted_preview_new_thread_message_path, preview_options end end @@ -313,25 +309,17 @@ class ResourcesTest < Test::Unit::TestCase end end - # NOTE from dchelimsky: http://dev.rubyonrails.org/ticket/8251 changes some of the - # named routes. In order to play nice, I didn't delete the old ones, which means - # that this test fails because until the old ones are deprecated and/or removed, there - # must be room for two names for the same route. - # - # From what I can tell this shouldn't matter - all the other - # tests in actionpack pass without this one passing. But I could be wrong ;) - # - # def test_restful_routes_dont_generate_duplicates - # with_restful_routing :messages do - # routes = ActionController::Routing::Routes.routes - # routes.each do |route| - # routes.each do |r| - # next if route === r # skip the comparison instance - # assert distinct_routes?(route, r), "Duplicate Route: #{route}" - # end - # end - # end - # end + def test_restful_routes_dont_generate_duplicates + with_restful_routing :messages do + routes = ActionController::Routing::Routes.routes + routes.each do |route| + routes.each do |r| + next if route === r # skip the comparison instance + assert distinct_routes?(route, r), "Duplicate Route: #{route}" + end + end + end + end def test_should_create_singleton_resource_routes with_singleton_resources :account do @@ -471,6 +459,50 @@ class ResourcesTest < Test::Unit::TestCase end end + def test_resource_action_separator + with_routing do |set| + set.draw do |map| + map.resources :messages, :collection => {:search => :get}, :new => {:preview => :any}, :name_prefix => 'thread_', :path_prefix => '/threads/:thread_id' + map.resource :account, :member => {:login => :get}, :new => {:preview => :any}, :name_prefix => 'admin_', :path_prefix => '/admin' + end + + action_separator = ActionController::Base.resource_action_separator + + assert_simply_restful_for :messages, :name_prefix => 'thread_', :path_prefix => 'threads/1/', :options => { :thread_id => '1' } + assert_named_route "/threads/1/messages#{action_separator}search", "search_thread_messages_path", {} + assert_named_route "/threads/1/messages/new", "new_thread_message_path", {} + assert_named_route "/threads/1/messages/new#{action_separator}preview", "preview_new_thread_message_path", {} + assert_singleton_restful_for :account, :name_prefix => 'admin_', :path_prefix => 'admin/' + assert_named_route "/admin/account#{action_separator}login", "login_admin_account_path", {} + assert_named_route "/admin/account/new", "new_admin_account_path", {} + assert_named_route "/admin/account/new#{action_separator}preview", "preview_new_admin_account_path", {} + end + end + + def test_new_style_named_routes_for_resource + with_routing do |set| + set.draw do |map| + map.resources :messages, :collection => {:search => :get}, :new => {:preview => :any}, :name_prefix => 'thread_', :path_prefix => '/threads/:thread_id' + end + assert_simply_restful_for :messages, :name_prefix => 'thread_', :path_prefix => 'threads/1/', :options => { :thread_id => '1' } + assert_named_route "/threads/1/messages/search", "search_thread_messages_path", {} + assert_named_route "/threads/1/messages/new", "new_thread_message_path", {} + assert_named_route "/threads/1/messages/new/preview", "preview_new_thread_message_path", {} + end + end + + def test_new_style_named_routes_for_singleton_resource + with_routing do |set| + set.draw do |map| + map.resource :account, :member => {:login => :get}, :new => {:preview => :any}, :name_prefix => 'admin_', :path_prefix => '/admin' + end + assert_singleton_restful_for :account, :name_prefix => 'admin_', :path_prefix => 'admin/' + assert_named_route "/admin/account/login", "login_admin_account_path", {} + assert_named_route "/admin/account/new", "new_admin_account_path", {} + assert_named_route "/admin/account/new/preview", "preview_new_admin_account_path", {} + end + end + def test_resources_in_namespace with_routing do |set| set.draw do |map| @@ -624,11 +656,6 @@ class ResourcesTest < Test::Unit::TestCase assert_named_route "#{full_prefix}/1", "#{name_prefix}#{singular_name}_path", options[:options].merge(:id => '1') assert_named_route "#{full_prefix}/1.xml", "formatted_#{name_prefix}#{singular_name}_path", options[:options].merge(:id => '1', :format => 'xml') - assert_potentially_deprecated_named_route name_prefix, /#{name_prefix}new_#{singular_name}/, "#{full_prefix}/new", "#{name_prefix}new_#{singular_name}_path", options[:options] - assert_potentially_deprecated_named_route name_prefix, /formatted_#{name_prefix}new_#{singular_name}/, "#{full_prefix}/new.xml", "formatted_#{name_prefix}new_#{singular_name}_path", options[:options].merge( :format => 'xml') - assert_potentially_deprecated_named_route name_prefix, /#{name_prefix}edit_#{singular_name}/, "#{full_prefix}/1/edit", "#{name_prefix}edit_#{singular_name}_path", options[:options].merge(:id => '1') - assert_potentially_deprecated_named_route name_prefix, /formatted_#{name_prefix}edit_#{singular_name}/, "#{full_prefix}/1/edit.xml", "formatted_#{name_prefix}edit_#{singular_name}_path", options[:options].merge(:id => '1', :format => 'xml') - assert_named_route "#{full_prefix}/new", "new_#{name_prefix}#{singular_name}_path", options[:options] assert_named_route "#{full_prefix}/new.xml", "formatted_new_#{name_prefix}#{singular_name}_path", options[:options].merge( :format => 'xml') assert_named_route "#{full_prefix}/1/edit", "edit_#{name_prefix}#{singular_name}_path", options[:options].merge(:id => '1') @@ -686,11 +713,6 @@ class ResourcesTest < Test::Unit::TestCase 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_potentially_deprecated_named_route name_prefix, /#{name_prefix}new_#{singleton_name}/, "#{full_path}/new", "#{name_prefix}new_#{singleton_name}_path", options[:options] - assert_potentially_deprecated_named_route name_prefix, /formatted_#{name_prefix}new_#{singleton_name}/, "#{full_path}/new.xml", "formatted_#{name_prefix}new_#{singleton_name}_path", options[:options].merge(:format => 'xml') - assert_potentially_deprecated_named_route name_prefix, /#{name_prefix}edit_#{singleton_name}/, "#{full_path}/edit", "#{name_prefix}edit_#{singleton_name}_path", options[:options] - assert_potentially_deprecated_named_route name_prefix, /formatted_#{name_prefix}edit_#{singleton_name}/, "#{full_path}/edit.xml", "formatted_#{name_prefix}edit_#{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}/edit", "edit_#{name_prefix}#{singleton_name}_path", options[:options] @@ -702,22 +724,6 @@ class ResourcesTest < Test::Unit::TestCase assert_equal expected, actual, "Error on route: #{route}(#{options.inspect})" end - def assert_deprecated_named_route(message, expected, route, options) - assert_deprecated message do - assert_named_route expected, route, options - end - end - - # These are only deprecated if they have a name_prefix. - # See http://dev.rubyonrails.org/ticket/8251 - def assert_potentially_deprecated_named_route(name_prefix, message, expected, route, options) - if name_prefix - assert_deprecated_named_route(message, expected, route, options) - else - assert_named_route expected, route, options - end - end - def assert_resource_methods(expected, resource, action_method, method) assert_equal expected.length, resource.send("#{action_method}_methods")[method].size, "#{resource.send("#{action_method}_methods")[method].inspect}" expected.each do |action| diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index 858ec6f3d1..5ca63396b4 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -22,7 +22,7 @@ class UriReservedCharactersRoutingTest < Test::Unit::TestCase map.connect ':controller/:action/:variable' end - safe, unsafe = %w(: @ & = + $ , ;), %w(^ / ? # [ ]) + safe, unsafe = %w(: @ & = + $), %w(^ / ? # [ ] , ;) hex = unsafe.map { |char| '%' + char.unpack('H2').first.upcase } @segment = "#{safe}#{unsafe}".freeze @@ -1024,42 +1024,6 @@ class RouteTest < Test::Unit::TestCase end end -class MapperDeprecatedRouteTest < Test::Unit::TestCase - def setup - @set = mock("set") - @mapper = ActionController::Routing::RouteSet::Mapper.new(@set) - end - - def test_should_add_new_and_deprecated_named_routes - @set.expects(:add_named_route).with("new", "path", {:a => "b"}) - @set.expects(:add_deprecated_named_route).with("new", "old", "path", {:a => "b"}) - @mapper.deprecated_named_route("new", "old", "path", {:a => "b"}) - end - - def test_should_not_add_deprecated_named_route_if_both_are_the_same - @set.expects(:add_named_route).with("new", "path", {:a => "b"}) - @set.expects(:add_deprecated_named_route).with("new", "new", "path", {:a => "b"}).never - @mapper.deprecated_named_route("new", "new", "path", {:a => "b"}) - end -end - -class RouteSetDeprecatedRouteTest < Test::Unit::TestCase - def setup - @set = ActionController::Routing::RouteSet.new - end - - def test_should_add_deprecated_route - @set.expects(:add_named_route).with("old", "path", {:a => "b"}) - @set.add_deprecated_named_route("new", "old", "path", {:a => "b"}) - end - - def test_should_fire_deprecation_warning_on_access - @set.add_deprecated_named_route("new_outer_inner_path", "outer_new_inner_path", "/outers/:outer_id/inners/new", :controller => :inners) - ActiveSupport::Deprecation.expects(:warn) - @set.named_routes["outer_new_inner_path"] - end -end - end # uses_mocha class RouteBuilderTest < Test::Unit::TestCase @@ -1213,22 +1177,6 @@ class RouteBuilderTest < Test::Unit::TestCase assert_equal nil, builder.warn_output # should only warn on the :person segment end - def test_comma_isnt_a_route_separator - segments = builder.segments_for_route_path '/books/:id,:action' - defaults = { :action => 'show' } - assert_raise(ArgumentError) do - builder.assign_route_options(segments, defaults, {}) - end - end - - def test_semicolon_isnt_a_route_separator - segments = builder.segments_for_route_path '/books/:id;:action' - defaults = { :action => 'show' } - assert_raise(ArgumentError) do - builder.assign_route_options(segments, defaults, {}) - end - end - def test_segmentation_of_dot_path segments = builder.segments_for_route_path '/books/:action.rss' assert builder.assign_route_options(segments, {}, {}).empty? |