Skip to content

Commit

Permalink
Add custom ArgumentError for invalid to: values
Browse files Browse the repository at this point in the history
Previously, it was theoretically possible to define a route with a
Symbol as a `to:` value (or at least, it would not raise a
`NoMethodError`). However, passing a Symbol broke when `/#/.match?(to)`
was [replaced][1] with `to&.include?("#")` with the assumption that `to`
was always a String.

Instead of restoring the previous error, this commit improves how the
`to:` value is checked so that it raises an `ArgumentError` for any
invalid values. The extra strictness will specifically improve the error
when a Symbol or String that doesn't include a "#" are passed since they
were effectively equivalent to passing a `nil` value, or not specifying
`to:` at all.

[1]: 5726b1d
  • Loading branch information
skipkayhil committed Jan 7, 2024
1 parent c0b5052 commit a397764
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 13 deletions.
25 changes: 13 additions & 12 deletions actionpack/lib/action_dispatch/routing/mapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,23 @@ def normalize_options!(options, path_params, modyoule)

if to.respond_to?(:action) || to.respond_to?(:call)
options
else
to_endpoint = split_to to
controller = to_endpoint[0] || default_controller
action = to_endpoint[1] || default_action
elsif to.nil?
controller = default_controller
action = default_action

controller = add_controller_module(controller, modyoule)

options.merge! check_controller_and_action(path_params, controller, action)
elsif to.is_a?(String) && to.include?("#")
to_endpoint = to.split("#").map!(&:-@)
controller = to_endpoint[0]
action = to_endpoint[1]

controller = add_controller_module(controller, modyoule)

options.merge! check_controller_and_action(path_params, controller, action)
else
raise ArgumentError, ":to must respond_to? :action or :call, or it must be a String that includes '#'"
end
end

Expand Down Expand Up @@ -308,14 +317,6 @@ def check_part(name, part, path_params, hash)
hash
end

def split_to(to)
if to&.include?("#")
to.split("#").map!(&:-@)
else
[]
end
end

def add_controller_module(controller, modyoule)
if modyoule && !controller.is_a?(Regexp)
if controller&.start_with?("/")
Expand Down
11 changes: 10 additions & 1 deletion actionpack/test/dispatch/routing_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4054,7 +4054,16 @@ def test_missing_controller_with_to
get "/foo/bar", to: "foo"
end
}
assert_match(/Missing :controller/, ex.message)
assert_match(/:to must respond_to/, ex.message)
end

def test_to_is_a_symbol
ex = assert_raises(ArgumentError) {
draw do
get "/foo/bar", to: :foo
end
}
assert_match(/:to must respond_to/, ex.message)
end

def test_missing_action_on_hash
Expand Down

0 comments on commit a397764

Please sign in to comment.