Merge pull request rails#52422 from Shopify/deprecate_hash_key_path_routes

Deprecate hash key path mapping

Author: gmcgibbon

actioncable/lib/action_cable/engine.rb

@@ -64,7 +64,7 @@ class Engine < Rails::Engine # :nodoc:
         config = app.config
         unless config.action_cable.mount_path.nil?
           app.routes.prepend do
-            mount ActionCable.server => config.action_cable.mount_path, internal: true, anchor: true
+            mount ActionCable.server, at: config.action_cable.mount_path, internal: true, anchor: true
           end
         end
       end

actionmailbox/config/routes.rb

@@ -2,16 +2,16 @@
 
 Rails.application.routes.draw do
   scope "/rails/action_mailbox", module: "action_mailbox/ingresses" do
-    post "/postmark/inbound_emails" => "postmark/inbound_emails#create", as: :rails_postmark_inbound_emails
-    post "/relay/inbound_emails"    => "relay/inbound_emails#create",    as: :rails_relay_inbound_emails
-    post "/sendgrid/inbound_emails" => "sendgrid/inbound_emails#create", as: :rails_sendgrid_inbound_emails
+    post "/postmark/inbound_emails", to: "postmark/inbound_emails#create", as: :rails_postmark_inbound_emails
+    post "/relay/inbound_emails",    to: "relay/inbound_emails#create",    as: :rails_relay_inbound_emails
+    post "/sendgrid/inbound_emails", to: "sendgrid/inbound_emails#create", as: :rails_sendgrid_inbound_emails
 
     # Mandrill checks for the existence of a URL with a HEAD request before it will create the webhook.
-    get "/mandrill/inbound_emails"  => "mandrill/inbound_emails#health_check", as: :rails_mandrill_inbound_health_check
-    post "/mandrill/inbound_emails" => "mandrill/inbound_emails#create",       as: :rails_mandrill_inbound_emails
+    get "/mandrill/inbound_emails",  to: "mandrill/inbound_emails#health_check", as: :rails_mandrill_inbound_health_check
+    post "/mandrill/inbound_emails", to: "mandrill/inbound_emails#create",       as: :rails_mandrill_inbound_emails
 
     # Mailgun requires that a webhook's URL end in 'mime' for it to receive the raw contents of emails.
-    post "/mailgun/inbound_emails/mime" => "mailgun/inbound_emails#create", as: :rails_mailgun_inbound_emails
+    post "/mailgun/inbound_emails/mime", to: "mailgun/inbound_emails#create", as: :rails_mailgun_inbound_emails
   end
 
   # TODO: Should these be mounted within the engine only?
@@ -20,7 +20,7 @@
     get  "inbound_emails/sources/new", to: "inbound_emails/sources#new", as: :new_rails_conductor_inbound_email_source
     post "inbound_emails/sources", to: "inbound_emails/sources#create", as: :rails_conductor_inbound_email_sources
 
-    post ":inbound_email_id/reroute" => "reroutes#create", as: :rails_conductor_inbound_email_reroute
-    post ":inbound_email_id/incinerate" => "incinerates#create", as: :rails_conductor_inbound_email_incinerate
+    post ":inbound_email_id/reroute",    to: "reroutes#create",   as: :rails_conductor_inbound_email_reroute
+    post ":inbound_email_id/incinerate", to: "incinerates#create", as: :rails_conductor_inbound_email_incinerate
   end
 end

actionmailer/lib/action_mailer/railtie.rb

@@ -84,9 +84,9 @@ class Railtie < Rails::Railtie # :nodoc:
 
       if options.show_previews
         app.routes.prepend do
-          get "/rails/mailers" => "rails/mailers#index", internal: true
-          get "/rails/mailers/download/*path" => "rails/mailers#download", internal: true
-          get "/rails/mailers/*path" => "rails/mailers#preview", internal: true
+          get "/rails/mailers", to: "rails/mailers#index", internal: true
+          get "/rails/mailers/download/*path", to: "rails/mailers#download", internal: true
+          get "/rails/mailers/*path", to: "rails/mailers#preview", internal: true
         end
       end
     end

actionmailer/test/url_test.rb

@@ -9,8 +9,8 @@ class WelcomeController < ActionController::Base
 AppRoutes = ActionDispatch::Routing::RouteSet.new
 
 AppRoutes.draw do
-  get "/welcome" => "foo#bar", as: "welcome"
-  get "/dummy_model" => "foo#baz", as: "dummy_model"
+  get "/welcome", to: "foo#bar", as: "welcome"
+  get "/dummy_model", to: "foo#baz", as: "dummy_model"
   get "/welcome/greeting", to: "welcome#greeting"
   get "/a/b(/:id)", to: "a#b"
 end

actionpack/CHANGELOG.md

@@ -1,3 +1,19 @@
+*   Deprecate drawing routes with hash key paths to make routing faster.
+
+    ```ruby
+    # Before
+    get "/users" => "users#index"
+    post "/logout" => :sessions
+    mount MyApp => "/my_app"
+
+    # After
+    get "/users", to: "users#index"
+    post "/logout", to: "sessions#logout"
+    mount MyApp, at: "/my_app"
+    ```
+
+    *Gannon McGibbon*
+
 *   Deprecate drawing routes with multiple paths to make routing faster.
     You may use `with_options` or a loop to make drawing multiple paths easier.
 

actionpack/lib/action_dispatch/routing.rb

@@ -118,9 +118,9 @@ module ActionDispatch
   #
   #     # In config/routes.rb
   #     controller :blog do
-  #       get 'blog/show'    => :list
-  #       get 'blog/delete'  => :delete
-  #       get 'blog/edit'    => :edit
+  #       get 'blog/show',   to: :list
+  #       get 'blog/delete', to: :delete
+  #       get 'blog/edit',   to: :edit
   #     end
   #
   #     # provides named routes for show, delete, and edit

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -636,6 +636,7 @@ def mount(app, options = nil)
             options = app
             app, path = options.find { |k, _| k.respond_to?(:call) }
             options.delete(app) if app
+            hash_key_app = true
           end
 
           raise ArgumentError, "A rack application must be specified" unless app.respond_to?(:call)
@@ -646,6 +647,10 @@ def mount(app, options = nil)
               or
               mount(SomeRackApp => "some_route")
           MSG
+          ActionDispatch.deprecator.warn(<<-MSG.squish) if hash_key_app
+            Mounting an engine with a hash key name is deprecated and
+            will be removed in Rails 8.1. Please use the at: option instead.
+          MSG
 
           rails_app = rails_app? app
           options[:as] ||= app_name(app, rails_app)
@@ -1682,6 +1687,12 @@ def match(path, *rest, &block)
 
             raise ArgumentError, "Route path not specified" if path.nil?
 
+            ActionDispatch.deprecator.warn(<<-MSG.squish)
+              Drawing a route with a hash key name is deprecated and
+              will be removed in Rails 8.1. Please use the to: option with
+              "controller#action" syntax instead.
+            MSG
+
             case to
             when Symbol
               options[:action] = to

actionpack/lib/action_dispatch/routing/redirection.rb

@@ -148,7 +148,7 @@ def inspect
     module Redirection
       # Redirect any path to another path:
       #
-      #     get "/stories" => redirect("/posts")
+      #     get "/stories", to: redirect("/posts")
       #
       # This will redirect the user, while ignoring certain parts of the request,
       # including query string, etc. `/stories`, `/stories?foo=bar`, etc all redirect
@@ -157,7 +157,7 @@ module Redirection
       # The redirect will use a `301 Moved Permanently` status code by default. This
       # can be overridden with the `:status` option:
       #
-      #     get "/stories" => redirect("/posts", status: 307)
+      #     get "/stories", to: redirect("/posts", status: 307)
       #
       # You can also use interpolation in the supplied redirect argument:
       #
@@ -199,7 +199,7 @@ module Redirection
       # allowing you to reuse common redirect routes. The call method must accept two
       # arguments, params and request, and return a string.
       #
-      #     get 'accounts/:name' => redirect(SubdomainRedirector.new('api'))
+      #     get 'accounts/:name', to: redirect(SubdomainRedirector.new('api'))
       #
       def redirect(*args, &block)
         options = args.extract_options!

actionpack/test/controller/base_test.rb

@@ -215,7 +215,7 @@ def setup
   def test_url_for_query_params_included
     rs = ActionDispatch::Routing::RouteSet.new
     rs.draw do
-      get "home" => "pages#home"
+      get "home", to: "pages#home"
     end
 
     options = {
@@ -316,7 +316,7 @@ class OptionalDefaultUrlOptionsControllerTest < ActionController::TestCase
   def test_default_url_options_override_missing_positional_arguments
     with_routing do |set|
       set.draw do
-        get "/things/:id(.:format)" => "things#show", :as => :thing
+        get "/things/:id(.:format)", to: "things#show", as: :thing
       end
       assert_equal "/things/1.atom", thing_path("1")
       assert_equal "/things/default-id.atom", thing_path

actionpack/test/controller/integration_test.rb

@@ -649,7 +649,7 @@ def with_test_route_set
         end
 
         set.draw do
-          get "moved" => redirect("/method")
+          get "moved", to: redirect("/method")
 
           ActionDispatch.deprecator.silence do
             match ":action", to: controller, via: [:get, :post], as: :action
@@ -784,9 +784,9 @@ def self.call(*)
     get "foo", to: "application_integration_test/test#index", as: :foo
     get "bar", to: "application_integration_test/test#index", as: :bar
 
-    mount MountedApp => "/mounted", :as => "mounted"
-    get "fooz" => proc { |env| [ 200, { ActionDispatch::Constants::X_CASCADE => "pass" }, [ "omg" ] ] },
-      :anchor => false
+    mount MountedApp, at: "/mounted", as: "mounted"
+    get "fooz", to: proc { |env| [ 200, { ActionDispatch::Constants::X_CASCADE => "pass" }, [ "omg" ] ] },
+      anchor: false
     get "fooz", to: "application_integration_test/test#index"
   end
 
@@ -890,7 +890,7 @@ def headers
   test "doesn't call controller's headers method" do
     with_routing do |routes|
       routes.draw do
-        get "/ok" => "controller_with_headers_method_integration_test/test#index"
+        get "/ok", to: "controller_with_headers_method_integration_test/test#index"
       end
 
       get "/ok"
@@ -941,10 +941,10 @@ def app
     default_url_options host: "foo.com"
 
     scope module: "url_options_integration_test" do
-      get "/foo" => "foo#index", :as => :foos
-      get "/foo/:id" => "foo#show", :as => :foo
-      get "/foo/:id/edit" => "foo#edit", :as => :edit_foo
-      get "/bar" => "bar#index", :as => :bars
+      get "/foo", to: "foo#index", as: :foos
+      get "/foo/:id", to: "foo#show", as: :foo
+      get "/foo/:id/edit", to: "foo#edit", as: :edit_foo
+      get "/bar", to: "bar#index", as: :bars
     end
   end
 
@@ -1004,7 +1004,7 @@ def app
   end
 
   routes.draw do
-    get "/foo/status" => "head_with_status_action_integration_test/foo#status"
+    get "/foo/status", to: "head_with_status_action_integration_test/foo#status"
   end
 
   test "get /foo/status with head result does not cause stack overflow error" do
@@ -1067,7 +1067,7 @@ def test_request
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          get ":action" => FooController
+          get ":action", to: FooController
         end
       end
 
@@ -1115,7 +1115,7 @@ def test_standard_json_encoding_works
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          post ":action" => FooController
+          post ":action", to: FooController
         end
       end
 
@@ -1142,7 +1142,7 @@ def test_doesnt_mangle_request_path
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          post ":action" => FooController
+          post ":action", to: FooController
         end
       end
 
@@ -1203,7 +1203,7 @@ def test_parsed_body_without_as_option
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          get ":action" => FooController
+          get ":action", to: FooController
         end
       end
 
@@ -1217,7 +1217,7 @@ def test_get_parameters_with_as_option
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          get ":action" => FooController
+          get ":action", to: FooController
         end
       end
 
@@ -1231,7 +1231,7 @@ def test_get_request_with_json_uses_method_override_and_sends_a_post_request
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          get ":action" => FooController
+          get ":action", to: FooController
         end
       end
 
@@ -1247,7 +1247,7 @@ def test_get_request_with_json_excludes_null_query_string
     with_routing do |routes|
       routes.draw do
         ActionDispatch.deprecator.silence do
-          get ":action" => FooController
+          get ":action", to: FooController
         end
       end
 
@@ -1262,7 +1262,7 @@ def post_to_foos(as:)
       with_routing do |routes|
         routes.draw do
           ActionDispatch.deprecator.silence do
-            post ":action" => FooController
+            post ":action", to: FooController
           end
         end
 
@@ -1358,8 +1358,8 @@ def remove_dumps
   end
 
   routes.draw do
-    get "/" => "page_dump_integration_test/foo#index"
-    get "/redirect" => "page_dump_integration_test/foo#redirect"
+    get "/", to: "page_dump_integration_test/foo#index"
+    get "/redirect", to: "page_dump_integration_test/foo#redirect"
   end
 
   test "save_and_open_page saves a copy of the page and call to Launchy" do