diff options
author | Jon Moss <me@jonathanmoss.me> | 2016-12-29 10:40:47 -0500 |
---|---|---|
committer | Jon Moss <me@jonathanmoss.me> | 2016-12-29 10:40:47 -0500 |
commit | 0713265fd15f5ce0d6e86e620a2de254ff291f81 (patch) | |
tree | dfd008a1de576b048fc2a207400f7062ef409349 /actionpack | |
parent | 415e17d0b54681545d36a0f43d4cd8761de77bee (diff) | |
download | rails-0713265fd15f5ce0d6e86e620a2de254ff291f81.tar.gz rails-0713265fd15f5ce0d6e86e620a2de254ff291f81.tar.bz2 rails-0713265fd15f5ce0d6e86e620a2de254ff291f81.zip |
Use `next` instead of `break`; avoid terminating whole loop
We want to avoid terminating the whole loop here, because it will cause
parameters that should be removed to not be removed, since we are
terminating early. In this specific case, `param2` is processed before
`param1` due to the reversing of `route.parts`, and since `param2` fails
the check on this line, it would previously cause the whole loop to
fail, and `param1` would still be in `parameterized_parts`. Now, we are
simply calling `next`, which is the intended behavior.
Introduced by 8ca8a2d773b942c4ea76baabe2df502a339d05b1.
Fixes #27454.
Diffstat (limited to 'actionpack')
-rw-r--r-- | actionpack/lib/action_dispatch/journey/formatter.rb | 2 | ||||
-rw-r--r-- | actionpack/test/controller/url_for_test.rb | 21 |
2 files changed, 22 insertions, 1 deletions
diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index 1d239addf8..f3b8e82d32 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -36,7 +36,7 @@ module ActionDispatch route.parts.reverse_each do |key| break if defaults[key].nil? && parameterized_parts[key].present? - break if parameterized_parts[key].to_s != defaults[key].to_s + next if parameterized_parts[key].to_s != defaults[key].to_s break if required_parts.include?(key) parameterized_parts.delete(key) diff --git a/actionpack/test/controller/url_for_test.rb b/actionpack/test/controller/url_for_test.rb index 4b6f33c545..382b4e273d 100644 --- a/actionpack/test/controller/url_for_test.rb +++ b/actionpack/test/controller/url_for_test.rb @@ -487,6 +487,27 @@ module AbstractController end end + def test_default_params_first_empty + with_routing do |set| + set.draw do + get "(:param1)/test(/:param2)" => "index#index", + defaults: { + param1: 1, + param2: 2 + }, + constraints: { + param1: /\d*/, + param2: /\d+/ + } + end + + kls = Class.new { include set.url_helpers } + kls.default_url_options[:host] = "www.basecamphq.com" + + assert_equal "http://www.basecamphq.com/test", kls.new.url_for(controller: "index", param1: "1") + end + end + private def extract_params(url) url.split("?", 2).last.split("&").sort |