Skip to content

Commit

Permalink
Allow ActionController::Base#default_url_options to have a default op…
Browse files Browse the repository at this point in the history
…tions argument of nil.

This fixes a bug introduced in [6a6b439] which was breaking routing in ActionController::UrlWriter.
  • Loading branch information
chuyeow authored and rick committed May 6, 2008
1 parent 04d8554 commit ee1d508
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 5 deletions.
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/base.rb
Expand Up @@ -998,7 +998,7 @@ def rewrite_options(options) #:nodoc:
# As you can infer from the example, this is mostly useful for situations where you want to centralize dynamic decisions about the
# urls as they stem from the business domain. Please note that any individual url_for call can always override the defaults set
# by this method.
def default_url_options(options) #:doc:
def default_url_options(options = nil)
end

# Redirects the browser to the target specified in +options+. This parameter can take one of three forms:
Expand Down
6 changes: 3 additions & 3 deletions actionpack/lib/action_controller/routing/optimisations.rb
Expand Up @@ -61,9 +61,9 @@ def guard_condition
# 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
"(!defined?(default_url_options) || default_url_options(nil).blank?) && defined?(request) && request && args.size == 1 && !args.first.is_a?(Hash)"
"(!defined?(default_url_options) || default_url_options.blank?) && defined?(request) && request && args.size == 1 && !args.first.is_a?(Hash)"
else
"(!defined?(default_url_options) || default_url_options(nil).blank?) && defined?(request) && request && args.size == #{number_of_arguments}"
"(!defined?(default_url_options) || default_url_options.blank?) && defined?(request) && request && args.size == #{number_of_arguments}"
end
end

Expand Down Expand Up @@ -98,7 +98,7 @@ def generation_code
# argument
class PositionalArgumentsWithAdditionalParams < PositionalArguments
def guard_condition
"(!defined?(default_url_options) || default_url_options(nil).blank?) && defined?(request) && request && args.size == #{route.segment_keys.size + 1} && !args.last.has_key?(:anchor) && !args.last.has_key?(:port) && !args.last.has_key?(:host)"
"(!defined?(default_url_options) || default_url_options.blank?) && defined?(request) && request && args.size == #{route.segment_keys.size + 1} && !args.last.has_key?(:anchor) && !args.last.has_key?(:port) && !args.last.has_key?(:host)"
end

# This case uses almost the same code as positional arguments,
Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/controller/base_test.rb
Expand Up @@ -52,7 +52,7 @@ class DefaultUrlOptionsController < ActionController::Base
def default_url_options_action
end

def default_url_options(options)
def default_url_options(options = nil)
{ :host => 'www.override.com', :action => 'new', :bacon => 'chunky' }
end
end
Expand Down

3 comments on commit ee1d508

@samlown
Copy link

@samlown samlown commented on ee1d508 Jun 9, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

This change has caused me a bit of a headache to try and figure why some of my redirects to named routes had stopped working after an upgrade to Rails 2.1. I’m probably not the only one to be affected by this either, as changing the definition of a method which is frequently “duck-typed” is really not a good idea, especially when the error message is so cryptic.

For the record the error I received was this:

(eval):2:in `default_url_options’ (eval):2:in`admin_pages_url’

Where admin_pages_url is my named route. The code in question in the controller was:

redirect_to admin_pages_url

Ensuring all definitions of default_url_options contain the options = nil part fixes this problem, but I really do not think this change was necessary.

Perhaps this can be reverted to the first version of this change here:

http://github.com/rails/rails/commit/6a6b4392c16c665eb713705f2b38e959a658eeef

Thanks, Sam Lown

@samlown
Copy link

@samlown samlown commented on ee1d508 Jun 9, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

This change has caused me a bit of a headache to try and figure why some of my redirects to named routes had stopped working after an upgrade to Rails 2.1. I’m probably not the only one to be affected by this either, as changing the definition of a method which is frequently “duck-typed” is really not a good idea, especially when the error message is so cryptic.

For the record the error I received was this:

(eval):2:in `default_url_options’ (eval):2:in`admin_pages_url’

Where admin_pages_url is my named route. The code in question in the controller was:

redirect_to admin_pages_url

Ensuring all definitions of default_url_options contain the options = nil part fixes this problem, but I really do not think this change was necessary.

Perhaps this can be reverted to the first version of this change here:

http://github.com/rails/rails/commit/6a6b4392c16c665eb713705f2b38e959a658eeef

Thanks, Sam Lown

@samlown
Copy link

@samlown samlown commented on ee1d508 Jun 9, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

This change has caused me a bit of a headache to try and figure why some of my redirects to named routes had stopped working after an upgrade to Rails 2.1. I’m probably not the only one to be affected by this either, as changing the definition of a method which is frequently “duck-typed” is really not a good idea, especially when the error message is so cryptic.

For the record the error I received was this:

(eval):2:in `default_url_options’ (eval):2:in`admin_pages_url’

Where admin_pages_url is my named route. The code in question in the controller was:

redirect_to admin_pages_url

Ensuring all definitions of default_url_options contain the options = nil part fixes this problem, but I really do not think this change was necessary.

Perhaps this can be reverted to the first version of this change here:

http://github.com/rails/rails/commit/6a6b4392c16c665eb713705f2b38e959a658eeef

Thanks, Sam Lown

Please sign in to comment.