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 patch
ensures scope is kept both when the route takes parameters or when it
doesn't.

Fixes #33219

5 files changed, 65 insertions, 11 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index a84f72eca9..4bcf313ecc 100644
--- a/actionpack/CHANGELOG.md
+++ b/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*
diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb
index 52396ec901..53c404ee7c 100644
--- a/actionpack/lib/action_dispatch/journey/formatter.rb
+++ b/actionpack/lib/action_dispatch/journey/formatter.rb
@@ -67,7 +67,7 @@ module ActionDispatch
           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, _|
diff --git a/actionpack/lib/action_dispatch/journey/route.rb b/actionpack/lib/action_dispatch/journey/route.rb
index 8165709a3d..587ebcb873 100644
--- a/actionpack/lib/action_dispatch/journey/route.rb
+++ b/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
@@ -51,13 +51,13 @@ module ActionDispatch
 
       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
+        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, path, constraints, required_defaults, defaults, request_method_match, precedence, scope_options, internal = false)
         @name        = name
         @app         = app
         @path        = path
@@ -72,6 +72,7 @@ module ActionDispatch
         @decorated_ast     = nil
         @precedence        = precedence
         @path_formatter    = @path.build_formatter
+        @scope_options     = scope_options
         @internal          = internal
       end
 
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index f29f66990d..695eed3adc 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -70,17 +70,17 @@ module ActionDispatch
         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_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, ast, defaults, controller, default_action, scope[:module], to, formatted, scope_constraints, scope_options, scope[:blocks] || [], via, options_constraints, anchor, options
         end
 
         def self.check_via(via)
@@ -111,7 +111,7 @@ module ActionDispatch
           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)
+        def initialize(set, ast, defaults, controller, default_action, modyoule, to, formatted, scope_constraints, scope_options, blocks, via, options_constraints, anchor, options)
           @defaults = defaults
           @set = set
 
@@ -122,6 +122,7 @@ module ActionDispatch
           @anchor             = anchor
           @via                = via
           @internal           = options.delete(:internal)
+          @scope_options      = scope_options
 
           path_params = ast.find_all(&:symbol?).map(&:to_sym)
 
@@ -161,7 +162,7 @@ module ActionDispatch
 
         def make_route(name, precedence)
           Journey::Route.new(name, application, path, conditions, required_defaults,
-                             defaults, request_method, precedence, @internal)
+                             defaults, request_method, precedence, scope_options, @internal)
         end
 
         def application
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 362488d585..0070d7af72 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -4959,6 +4959,47 @@ class TestPartialDynamicPathSegments < ActionDispatch::IntegrationTest
     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