Keep part when scope option has value

actionpack/CHANGELOG.md

@@ -1,3 +1,14 @@
+*   Keep part when scope option has value
+
+    When a route was defined within an optional scope, if that route didn't
+    take parameters the scope was lost when using path helpers. This commit
+    ensures scope is kept both when the route takes parameters or when it
+    doesn't.
+
+    Fixes #33219
+
+    *Alberto Almagro*
+
 *   Added `deep_transform_keys` and `deep_transform_keys!` methods to ActionController::Parameters.
 
     *Gustavo Gutierrez*

actionpack/lib/action_dispatch/journey/formatter.rb

@@ -67,7 +67,7 @@ def extract_parameterized_parts(route, options, recall, parameterize = nil)
           parameterized_parts = recall.merge(options)
 
           keys_to_keep = route.parts.reverse_each.drop_while { |part|
-            !options.key?(part) || (options[part] || recall[part]).nil?
+            !(options.key?(part) || route.scope_options.key?(part)) || (options[part] || recall[part]).nil?
           } | route.required_parts
 
           parameterized_parts.delete_if do |bad_key, _|

actionpack/lib/action_dispatch/journey/route.rb

@@ -4,9 +4,9 @@ module ActionDispatch
   # :stopdoc:
   module Journey
     class Route
-      attr_reader :app, :path, :defaults, :name, :precedence
+      attr_reader :app, :path, :defaults, :name, :precedence, :constraints,
+                  :internal, :scope_options
 
-      attr_reader :constraints, :internal
       alias :conditions :constraints
 
       module VerbMatchers
@@ -49,15 +49,10 @@ def self.verb_matcher(verb)
         end
       end
 
-      def self.build(name, app, path, constraints, required_defaults, defaults)
-        request_method_match = verb_matcher(constraints.delete(:request_method))
-        new name, app, path, constraints, required_defaults, defaults, request_method_match, 0
-      end
-
       ##
       # +path+ is a path constraint.
       # +constraints+ is a hash of constraints to be applied to this route.
-      def initialize(name, app, path, constraints, required_defaults, defaults, request_method_match, precedence, internal = false)
+      def initialize(name:, app: nil, path:, constraints: {}, required_defaults: [], defaults: {}, request_method_match: nil, precedence: 0, scope_options: {}, internal: false)
         @name        = name
         @app         = app
         @path        = path
@@ -72,6 +67,7 @@ def initialize(name, app, path, constraints, required_defaults, defaults, reques
         @decorated_ast     = nil
         @precedence        = precedence
         @path_formatter    = @path.build_formatter
+        @scope_options     = scope_options
         @internal          = internal
       end
 

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -70,17 +70,21 @@ class Mapping #:nodoc:
         ANCHOR_CHARACTERS_REGEX = %r{\A(\\A|\^)|(\\Z|\\z|\$)\Z}
         OPTIONAL_FORMAT_REGEX = %r{(?:\(\.:format\)+|\.:format|/)\Z}
 
-        attr_reader :requirements, :defaults
-        attr_reader :to, :default_controller, :default_action
-        attr_reader :required_defaults, :ast
+        attr_reader :requirements, :defaults, :to, :default_controller,
+                    :default_action, :required_defaults, :ast, :scope_options
 
         def self.build(scope, set, ast, controller, default_action, to, via, formatted, options_constraints, anchor, options)
-          options = scope[:options].merge(options) if scope[:options]
-
-          defaults = (scope[:defaults] || {}).dup
-          scope_constraints = scope[:constraints] || {}
+          scope_params = {
+            blocks: scope[:blocks] || [],
+            constraints: scope[:constraints] || {},
+            defaults: (scope[:defaults] || {}).dup,
+            module: scope[:module],
+            options: scope[:options] || {}
+          }
 
-          new set, ast, defaults, controller, default_action, scope[:module], to, formatted, scope_constraints, scope[:blocks] || [], via, options_constraints, anchor, options
+          new set: set, ast: ast, controller: controller, default_action: default_action,
+              to: to, formatted: formatted, via: via, options_constraints: options_constraints,
+              anchor: anchor, scope_params: scope_params, options: scope_params[:options].merge(options)
         end
 
         def self.check_via(via)
@@ -111,33 +115,33 @@ def self.optional_format?(path, format)
           format != false && path !~ OPTIONAL_FORMAT_REGEX
         end
 
-        def initialize(set, ast, defaults, controller, default_action, modyoule, to, formatted, scope_constraints, blocks, via, options_constraints, anchor, options)
-          @defaults = defaults
-          @set = set
-
+        def initialize(set:, ast:, controller:, default_action:, to:, formatted:, via:, options_constraints:, anchor:, scope_params:, options:)
+          @defaults           = scope_params[:defaults]
+          @set                = set
           @to                 = intern(to)
           @default_controller = intern(controller)
           @default_action     = intern(default_action)
           @ast                = ast
           @anchor             = anchor
           @via                = via
           @internal           = options.delete(:internal)
+          @scope_options      = scope_params[:options]
 
           path_params = ast.find_all(&:symbol?).map(&:to_sym)
 
           options = add_wildcard_options(options, formatted, ast)
 
-          options = normalize_options!(options, path_params, modyoule)
+          options = normalize_options!(options, path_params, scope_params[:module])
 
           split_options = constraints(options, path_params)
 
-          constraints = scope_constraints.merge Hash[split_options[:constraints] || []]
+          constraints = scope_params[:constraints].merge Hash[split_options[:constraints] || []]
 
           if options_constraints.is_a?(Hash)
             @defaults = Hash[options_constraints.find_all { |key, default|
               URL_OPTIONS.include?(key) && (String === default || Integer === default)
             }].merge @defaults
-            @blocks = blocks
+            @blocks = scope_params[:blocks]
             constraints.merge! options_constraints
           else
             @blocks = blocks(options_constraints)
@@ -160,8 +164,10 @@ def initialize(set, ast, defaults, controller, default_action, modyoule, to, for
         end
 
         def make_route(name, precedence)
-          Journey::Route.new(name, application, path, conditions, required_defaults,
-                             defaults, request_method, precedence, @internal)
+          Journey::Route.new(name: name, app: application, path: path, constraints: conditions,
+                             required_defaults: required_defaults, defaults: defaults,
+                             request_method_match: request_method, precedence: precedence,
+                             scope_options: scope_options, internal: @internal)
         end
 
         def application

actionpack/test/dispatch/routing_test.rb

@@ -4959,6 +4959,47 @@ def assert_params(params)
     end
 end
 
+class TestOptionalScopesWithOrWithoutParams < ActionDispatch::IntegrationTest
+  Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
+    app.draw do
+      scope module: "test_optional_scopes_with_or_without_params" do
+        scope "(:locale)", locale: /en|es/ do
+          get "home", to: "home#index"
+          get "with_param/:foo", to: "home#with_param", as: "with_param"
+          get "without_param", to: "home#without_param"
+        end
+      end
+    end
+  end
+
+  class HomeController < ActionController::Base
+    include Routes.url_helpers
+
+    def index
+      render inline: "<%= with_param_path(foo: 'bar') %> | <%= without_param_path %>"
+    end
+
+    def with_param; end
+    def without_param; end
+  end
+
+  APP = build_app Routes
+
+  def app
+    APP
+  end
+
+  def test_stays_unscoped_with_or_without_params
+    get "/home"
+    assert_equal "/with_param/bar | /without_param", response.body
+  end
+
+  def test_preserves_scope_with_or_without_params
+    get "/es/home"
+    assert_equal "/es/with_param/bar | /es/without_param", response.body
+  end
+end
+
 class TestPathParameters < ActionDispatch::IntegrationTest
   Routes = ActionDispatch::Routing::RouteSet.new.tap do |app|
     app.draw do

actionpack/test/journey/route_test.rb

@@ -9,57 +9,57 @@ def test_initialize
         app      = Object.new
         path     = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
         defaults = {}
-        route    = Route.build("name", app, path, {}, [], defaults)
+        route    = Route.new(name: "name", app: app, path: path, defaults: defaults)
 
         assert_equal app, route.app
         assert_equal path, route.path
         assert_same  defaults, route.defaults
       end
 
       def test_route_adds_itself_as_memo
-        app      = Object.new
-        path     = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
-        defaults = {}
-        route    = Route.build("name", app, path, {}, [], defaults)
+        app   = Object.new
+        path  = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
+        route = Route.new(name: "name", app: app, path: path)
 
         route.ast.grep(Nodes::Terminal).each do |node|
           assert_equal route, node.memo
         end
       end
 
       def test_path_requirements_override_defaults
-        path = Path::Pattern.build(":name", { name: /love/ }, "/", true)
-        defaults  = { name: "tender" }
-        route     = Route.build("name", nil, path, {}, [], defaults)
+        path     = Path::Pattern.build(":name", { name: /love/ }, "/", true)
+        defaults = { name: "tender" }
+        route    = Route.new(name: "name", path: path, defaults: defaults)
         assert_equal(/love/, route.requirements[:name])
       end
 
       def test_ip_address
         path  = Path::Pattern.from_string "/messages/:id(.:format)"
-        route = Route.build("name", nil, path, { ip: "192.168.1.1" }, [],
-                          controller: "foo", action: "bar")
+        route = Route.new(name: "name", path: path, constraints: { ip: "192.168.1.1" },
+                          defaults: { controller: "foo", action: "bar" })
         assert_equal "192.168.1.1", route.ip
       end
 
       def test_default_ip
         path  = Path::Pattern.from_string "/messages/:id(.:format)"
-        route = Route.build("name", nil, path, {}, [],
-                          controller: "foo", action: "bar")
+        route = Route.new(name: "name", path: path,
+                          defaults: { controller: "foo", action: "bar" })
         assert_equal(//, route.ip)
       end
 
       def test_format_with_star
         path  = Path::Pattern.from_string "/:controller/*extra"
-        route = Route.build("name", nil, path, {}, [],
-                          controller: "foo", action: "bar")
+        route = Route.new(name: "name", path: path,
+                          defaults: { controller: "foo", action: "bar" })
         assert_equal "/foo/himom", route.format(
           controller: "foo",
           extra: "himom")
       end
 
       def test_connects_all_match
         path  = Path::Pattern.from_string "/:controller(/:action(/:id(.:format)))"
-        route = Route.build("name", nil, path, { action: "bar" }, [], controller: "foo")
+        route = Route.new(name: "name", path: path, constraints: { action: "bar" },
+                          defaults: { controller: "foo" })
 
         assert_equal "/foo/bar/10", route.format(
           controller: "foo",
@@ -69,34 +69,33 @@ def test_connects_all_match
 
       def test_extras_are_not_included_if_optional
         path  = Path::Pattern.from_string "/page/:id(/:action)"
-        route = Route.build("name", nil, path, {}, [], action: "show")
+        route = Route.new(name: "name", path: path, defaults: { action: "show" })
 
         assert_equal "/page/10", route.format(id: 10)
       end
 
       def test_extras_are_not_included_if_optional_with_parameter
         path  = Path::Pattern.from_string "(/sections/:section)/pages/:id"
-        route = Route.build("name", nil, path, {}, [], action: "show")
+        route = Route.new(name: "name", path: path, defaults: { action: "show" })
 
         assert_equal "/pages/10", route.format(id: 10)
       end
 
       def test_extras_are_not_included_if_optional_parameter_is_nil
         path  = Path::Pattern.from_string "(/sections/:section)/pages/:id"
-        route = Route.build("name", nil, path, {}, [], action: "show")
+        route = Route.new(name: "name", path: path, defaults: { action: "show" })
 
         assert_equal "/pages/10", route.format(id: 10, section: nil)
       end
 
       def test_score
-        constraints = {}
         defaults = { controller: "pages", action: "show" }
 
         path = Path::Pattern.from_string "/page/:id(/:action)(.:format)"
-        specific = Route.build "name", nil, path, constraints, [:controller, :action], defaults
+        specific = Route.new name: "name", path: path, required_defaults: [:controller, :action], defaults: defaults
 
         path = Path::Pattern.from_string "/:controller(/:action(/:id))(.:format)"
-        generic = Route.build "name", nil, path, constraints, [], {}
+        generic = Route.new name: "name", path: path
 
         knowledge = { "id" => true, "controller" => true, "action" => true }