Add custom ArgumentError for invalid to: values

[skipkayhil]

Motivation / Background

Ref #50465

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 with to&.include?("#") with the assumption that to was always a String.

Detail

This commit fixes the issue by calling to_s on the to: value before calling includes? to prevent the NoMethodError.

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.

Additional information

I considered writing a test but testing that a symbol is allowed seems wrong when it doesn't actually work (it tries to split the symbol into controller/action, and returns nil for both if symbol). Maybe alternatively we proactively raise an ArgumentError if the to value isn't a String (after the respond_to? checks)?

If accepted, this should be backported to 7-1-stable Is this still backport worthy?

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[p8]

A lot of objects return a String with a # when calling to_s:

User.new.to_s
=> "#<User:0x00000001285ec1c0>"

So I think raising an ArgumentError if the to value isn't a String would cause less confusion.
We could also allow symbols.

[skipkayhil]

So I think raising an ArgumentError if the to value isn't a String would cause less confusion.
We could also allow symbols.

The more I think about it the more I agree that we should probably be raising. I suppose the question is whether a change like that would be backportable since its a larger change in behavior.

I'm kind of thinking the ideal flow should be:

• check if respond_to? :action || respond_to? :call
• check if String and include? "#"
• else ArgumentError

Supporting Symbols or Strings that don't include a # seem pointless to me since they would end up just being ignored (split_to returns an empty array), which feels less user friendly than an ArgumentError which could indicate their uselessness. If a route was previously working but suddenly raised an ArgumentError after this change then I think the to: could just be removed? I may need to double check that the to: isn't used anywhere else.

[p8]

We should probably still support the following (mentioned in the issue), as it's documented in the API docs:

controller :blog do
  get 'blog/show',    to: :list
  get 'blog/delete',  to: :delete
  get 'blog/edit',    to: :edit
end

I'd expect we'd have a failing test for that.

[p8]

Hmm, not sure if any of this ever worked as documented:

rails/actionpack/lib/action_dispatch/routing.rb

Lines 120 to 124 in 4d50600

	# controller :blog do
	# get 'blog/show' => :list
	# get 'blog/delete' => :delete
	# get 'blog/edit' => :edit
	# end

rails/actionpack/lib/action_dispatch/routing.rb

Lines 106 to 110 in 396bb77

	# controller :blog do
	# match 'blog/show' => :list
	# match 'blog/delete' => :delete
	# match 'blog/edit/:id' => :edit
	# end

[skipkayhil]

We should probably still support the following (mentioned in the issue), as it's documented in the API docs:

That's what's interesting, those examples do not work even with the current patch due to split_to returning an empty array when to: arg does not contain a #.

Hmm, not sure if any of this ever worked as documented:

These I found actually are tested:

rails/actionpack/test/dispatch/routing_test.rb

Lines 49 to 52 in e3da4fc

	controller :sessions do
	get "login" => :new
	post "login" => :create
	end

it's just to: that appears to not work as documented.

[p8]

It seems the following commit accidentally broke some examples. “to:” was added where it didn’t belong: fc54809
The NoMethodError would still occur though.

[rafaelfranca]

Yes, let's raise a better exception.

[skipkayhil]

👍 Updated with a new ArgumentError