Restore to: option in routes with an implicit controller

[etiennebarrie]

Ref #50465

In #50466, routes that use the to: option without providing a controller (e.g. to: "action") started to raise an ArgumentError. Before that it was possible to use the to: option without a controller as long as it was nested within a controller, resource or resources scope.

This broke one of our apps between 7.1.2 and 7.1.3, here's a repro script:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails", ARGV.first || "7.1.3"
end

require "action_controller/railtie"
require "minitest/autorun"

class FoosController < ActionController::Base
end

class BugTest < ActionController::TestCase
  def test_routing
    with_routing do |routes|
      routes.draw do
        resources :foos, only: [] do
          get "bar", to: "bar"
        end
      end

      assert_routing "/foos/1/bar", controller: "foos", action: "bar", foo_id: "1"
    end
  end
end

This fails using 7.1.3 and passes using 7.1.2:

$ ruby route-implicit-controller.rb |tail -1
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
$ ruby route-implicit-controller.rb 7.1.2 |tail -1                                                                                                                                                                                 
1 runs, 3 assertions, 0 failures, 0 errors, 0 skips

This PR fixes this and also allows passing a Symbol in those cases (e.g. to: :action).

The :to option for routes can once again be a Symbol or a String without a controller if the controller is implicitly provided by a nesting controller or resources call.

With these changes, the documentation changes from #50523 would not have been necessary, though it's fine to keep the documentation as it is now.

cc @skipkayhil @p8

[etiennebarrie]

This used to fail in check_controller_and_action, whereas now it fails in the checks for the to: option, which seems correct to me.

[etiennebarrie]

This lets us use routing assertions directly without having to use with_routing.

[etiennebarrie]

I avoided using action.present? here, but maybe I could just require the core ext.

[etiennebarrie]

Specifying the numbers of splits here makes sure we handle the "controller#" case correctly (and that we don't assume controller is the action).

[skipkayhil]

Can you also add a CHANGELOG entry since this fixes a regression in 7.1.3?

[skipkayhil]

If the goal is to backport this to 7-1-stable I'm not sure we should be adding support for Symbol since that didn't work even before #50466

[skipkayhil]

I feel like this new code is much harder to follow than before #50466.

What do you think of this diff? It also passes the reproduction script and I think is a bit simpler:

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 724bf72e5c..b152de0ea7 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -231,10 +231,15 @@ def normalize_options!(options, path_params, modyoule)
               if to.nil?
                 controller = default_controller
                 action = default_action
-              elsif to.is_a?(String) && to.include?("#")
-                to_endpoint = to.split("#").map!(&:-@)
-                controller  = to_endpoint[0]
-                action      = to_endpoint[1]
+              elsif to.is_a?(String)
+                if to.include?("#")
+                  to_endpoint = to.split("#").map!(&:-@)
+                  controller  = to_endpoint[0]
+                  action      = to_endpoint[1]
+                else
+                  controller = default_controller
+                  action = default_action
+                end
               else
                 raise ArgumentError, ":to must respond to `action` or `call`, or it must be a String that includes '#'"
               end

[etiennebarrie]

Yeah it's nicer. I had to use to instead of default_action in the new branch handling the string when it's missing the #, otherwise the repro script wasn't passing.

[etiennebarrie]

Added a changelog entry but it's a bit weird since the previous change in 7.1.3 hadn't been mentioned in the first place.