From c555e28a4cf3690a95b5ed108d174f39cedd19b1 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 10:12:41 -0700 Subject: [PATCH 1/8] Ignore nil in LookupContext#formats= This also removes the mutation we were performing on the values being passed in. --- actionview/lib/action_view/lookup_context.rb | 1 + actionview/test/template/lookup_context_test.rb | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 2eaf188ecf..a0a2d41948 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -280,6 +280,7 @@ module ActionView # add :html as fallback to :js. def formats=(values) if values + values = values.compact values.concat(default_formats) if values.delete "*/*" if values == [:js] values << :html diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index 537f8ee163..8b61302c32 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -65,6 +65,11 @@ class LookupContextTest < ActiveSupport::TestCase assert_equal Mime::SET.to_a, @lookup_context.formats end + test "ignores nil format" do + @lookup_context.formats = [:html, nil, :text] + assert_equal [:html, :text], @lookup_context.formats + end + test "handles explicitly defined */* formats fallback to :js" do @lookup_context.formats = [:js, Mime::ALL] assert_equal [:js, *Mime::SET.symbols], @lookup_context.formats From ffc842a09927986819cf7a66ae6a15f97bfeed76 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 10:13:35 -0700 Subject: [PATCH 2/8] Make uniq in LookupContext#formats= Having a format listed twice had no effect. This is mostly helpful to avoid an extra format when assigning [:html, "*/*"] --- actionview/lib/action_view/lookup_context.rb | 2 ++ actionview/test/template/lookup_context_test.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index a0a2d41948..41751c2963 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -282,6 +282,8 @@ module ActionView if values values = values.compact values.concat(default_formats) if values.delete "*/*" + values.uniq! + if values == [:js] values << :html @html_fallback_for_js = true diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index 8b61302c32..ce5b4c4f66 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -72,7 +72,7 @@ class LookupContextTest < ActiveSupport::TestCase test "handles explicitly defined */* formats fallback to :js" do @lookup_context.formats = [:js, Mime::ALL] - assert_equal [:js, *Mime::SET.symbols], @lookup_context.formats + assert_equal [:js, *Mime::SET.symbols].uniq, @lookup_context.formats end test "adds :html fallback to :js formats" do From 0eb9e1e51a9df554e1cf7a42ec339ca6f0120a04 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 10:18:36 -0700 Subject: [PATCH 3/8] Raise in LookupContext#formats= on invalid format This is a developer quality of life improvement, to ensure that unknown formats aren't assigned (which it would previously accept, but wouldn't work 100% correctly due to caching). --- actionview/lib/action_view/lookup_context.rb | 5 +++++ actionview/test/template/lookup_context_test.rb | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 41751c2963..8d92130957 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -284,6 +284,11 @@ module ActionView values.concat(default_formats) if values.delete "*/*" values.uniq! + invalid_types = (values - Template::Types.symbols) + unless invalid_types.empty? + raise ArgumentError, "Invalid formats: #{invalid_types.map(&:inspect).join(", ")}" + end + if values == [:js] values << :html @html_fallback_for_js = true diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index ce5b4c4f66..02a9948e38 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -80,6 +80,14 @@ class LookupContextTest < ActiveSupport::TestCase assert_equal [:js, :html], @lookup_context.formats end + test "raises on invalid format assignment" do + ex = assert_raises ArgumentError do + @lookup_context.formats = [:html, :javascript] + end + + assert_equal "Invalid formats: :javascript", ex.message + end + test "provides getters and setters for locale" do @lookup_context.locale = :pt assert_equal :pt, @lookup_context.locale From 663548845b265d97118999eea83bf05a7d41161f Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 16:06:22 -0700 Subject: [PATCH 4/8] Use symbol for mail preview format, not string --- railties/lib/rails/mailers_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/railties/lib/rails/mailers_controller.rb b/railties/lib/rails/mailers_controller.rb index 95dae3ec2d..5cffa52860 100644 --- a/railties/lib/rails/mailers_controller.rb +++ b/railties/lib/rails/mailers_controller.rb @@ -38,7 +38,7 @@ class Rails::MailersController < Rails::ApplicationController # :nodoc: end else @part = find_preferred_part(request.format, Mime[:html], Mime[:text]) - render action: "email", layout: false, formats: %w[html] + render action: "email", layout: false, formats: [:html] end else raise AbstractController::ActionNotFound, "Email '#{@email_action}' not found in #{@preview.name}" From c80b33a46123f54339dbc94c78e1bbed222cd9af Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 16:10:13 -0700 Subject: [PATCH 5/8] Rename invalid_types to invalid_values --- actionview/lib/action_view/lookup_context.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 8d92130957..336eaa0d8b 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -284,9 +284,9 @@ module ActionView values.concat(default_formats) if values.delete "*/*" values.uniq! - invalid_types = (values - Template::Types.symbols) - unless invalid_types.empty? - raise ArgumentError, "Invalid formats: #{invalid_types.map(&:inspect).join(", ")}" + invalid_values = (values - Template::Types.symbols) + unless invalid_values.empty? + raise ArgumentError, "Invalid formats: #{invalid_values.map(&:inspect).join(", ")}" end if values == [:js] From 15b315ca9c8888a7a025b61bb56049fb8fea424f Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 16:35:41 -0700 Subject: [PATCH 6/8] Improve "raises on invalid format assignment" test --- actionview/test/template/lookup_context_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index 02a9948e38..6457e45920 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -82,10 +82,10 @@ class LookupContextTest < ActiveSupport::TestCase test "raises on invalid format assignment" do ex = assert_raises ArgumentError do - @lookup_context.formats = [:html, :javascript] + @lookup_context.formats = [:html, :invalid, "also bad"] end - assert_equal "Invalid formats: :javascript", ex.message + assert_equal 'Invalid formats: :invalid, "also bad"', ex.message end test "provides getters and setters for locale" do From 31e488b901b6eb765ce9df049d536b950ad4a802 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Mon, 18 Mar 2019 16:34:16 -0700 Subject: [PATCH 7/8] Avoid assigning [nil] to formats --- actionview/lib/action_view/rendering.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionview/lib/action_view/rendering.rb b/actionview/lib/action_view/rendering.rb index ac861c44d4..5a06bd9da6 100644 --- a/actionview/lib/action_view/rendering.rb +++ b/actionview/lib/action_view/rendering.rb @@ -127,7 +127,7 @@ module ActionView # Assign the rendered format to look up context. def _process_format(format) super - lookup_context.formats = [format.to_sym] + lookup_context.formats = [format.to_sym] if format.to_sym end # Normalize args by converting render "foo" to render :action => "foo" and From 0756733d75cd4b9e115cc43f457c7f2731c25e1c Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Tue, 19 Mar 2019 16:34:12 -0700 Subject: [PATCH 8/8] Don't compact formats --- actionview/lib/action_view/lookup_context.rb | 2 +- actionview/test/template/lookup_context_test.rb | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 336eaa0d8b..fd3d025cbf 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -280,7 +280,7 @@ module ActionView # add :html as fallback to :js. def formats=(values) if values - values = values.compact + values = values.dup values.concat(default_formats) if values.delete "*/*" values.uniq! diff --git a/actionview/test/template/lookup_context_test.rb b/actionview/test/template/lookup_context_test.rb index 6457e45920..3e357fe1a7 100644 --- a/actionview/test/template/lookup_context_test.rb +++ b/actionview/test/template/lookup_context_test.rb @@ -65,11 +65,6 @@ class LookupContextTest < ActiveSupport::TestCase assert_equal Mime::SET.to_a, @lookup_context.formats end - test "ignores nil format" do - @lookup_context.formats = [:html, nil, :text] - assert_equal [:html, :text], @lookup_context.formats - end - test "handles explicitly defined */* formats fallback to :js" do @lookup_context.formats = [:js, Mime::ALL] assert_equal [:js, *Mime::SET.symbols].uniq, @lookup_context.formats